Merge pull request #172 from crytic/update-not-so-algorand

Update not-so-algorand

Author: montyly

not-so-smart-contracts/algorand/README.md

@@ -14,17 +14,17 @@ Each _Not So Smart Contract_ includes a standard set of information:
 
 ## Vulnerabilities
 
-| Not So Smart Contract | Description |
-| --- | --- |
-| [Rekeying](rekeying) | Smart signatures are rekeyable |
-| [Unchecked Transaction Fees](unchecked_transaction_fee) | Attacker sets excessive fees for smart signature transactions |
-| [Closing Account](closing_account) | Attacker closes smart signature accounts |
-| [Closing Asset](closing_asset) | Attacker transfers entire asset balance of a smart signature |
-| [Group Size Check](group_size_check) | Contract does not check transaction group size |
-| [Time-based Replay Attack](time_based_replay_attack) | Contract does not use lease for periodic payments |
-| [Access Controls](access_controls) | Contract does not enfore access controls for updating and deleting application | 
-| [Asset Id Check](asset_id_check) | Contract does not check asset id for asset transfer operations |
-| [Denial of Service](denial_of_service) | Attacker stalls contract execution by opting out of a asset |
+| Not So Smart Contract | Description | Applicable to smart signatures | Applicable to smart contracts |
+| --- | --- | --- | --- |
+| [Rekeying](rekeying) | Attacker rekeys an account | yes | yes |
+| [Unchecked Transaction Fees](unchecked_transaction_fee) | Attacker sets excessive fees for smart signature transactions | yes | no |
+| [Closing Account](closing_account) | Attacker closes smart signature accounts | yes | no |
+| [Closing Asset](closing_asset) | Attacker transfers entire asset balance of a smart signature | yes | no |
+| [Group Size Check](group_size_check) | Contract does not check transaction group size | yes | yes |
+| [Time-based Replay Attack](time_based_replay_attack) | Contract does not use lease for periodic payments | yes | no |
+| [Access Controls](access_controls) | Contract does not enfore access controls for updating and deleting application | no | yes |
+| [Asset Id Check](asset_id_check) | Contract does not check asset id for asset transfer operations | yes | yes |
+| [Denial of Service](denial_of_service) | Attacker stalls contract execution by opting out of a asset | yes | yes |
 
 ## Credits
 

not-so-smart-contracts/algorand/group_size_check/README.md

@@ -40,3 +40,5 @@ def split_and_withdraw(
 - Verify that the group size of an atomic transfer is the intended size in the contracts.
 
 - Use [Tealer](https://github.com/crytic/tealer) to detect this issue.
+
+- Favor using ABI for smart contracts and relative indexes to verify the group transaction.

not-so-smart-contracts/algorand/rekeying/README.md

@@ -13,6 +13,7 @@ Contract accounts are accounts which are derived from the Teal code that is in c
 
 Similar issue affects the accounts that created a delegate signature by signing a Teal program. Delegator is only needed to sign the contract and any user with access to delegate signature can construct and submit transactions. Because of this, a malicious user can rekey the sender’s account if the Teal logic accepts a transaction with the rekey-to field set to the user controlled address.
 
+Note: From Teal v6, Applications can also be rekeyed by executing an inner transaction with "RekeyTo" field set to a non-zero address. Rekeying an application allows to bypass the application logic and directly transfer Algos and assets of the application account.
 ## Exploit Scenarios
 
 A user creates a delegate signature for recurring payments. Attacker rekeys the sender’s account by specifying the rekey-to field in a valid payment transaction.
@@ -43,3 +44,5 @@ def withdraw(
 - For the Teal programs written in Teal version 2 or greater, either used as delegate signature or contract account, include a check in the program that verifies rekey-to field to be equal to ZeroAddress or any intended address. Teal contracts written in Teal version 1 are not affected by this issue. Rekeying feature is introduced in version 2 and Algorand rejects transactions that use features introduced in the versions later than the executed Teal program version.
 
 - Use [Tealer](https://github.com/crytic/tealer) to detect this issue.
+
+- For Applications, verify that user provided value is not used for `RekeyTo` field of a inner transaction. Additionally, avoid rekeying an application to admin controlled address. This allows for the possibility of "rug pull" by a malicious admin.

not-so-smart-contracts/algorand/unchecked_transaction_fee/README.md

@@ -36,4 +36,4 @@ def withdraw(
 
 ## Recommendations
 
-- Verify that transaction fees are under reasonable bounds before approving the transaction in the Teal contract.
+- Force the transaction fee to be `0` and use fee pooling. If the users should be able to call the smart signature outside of a group, force the transaction fee to be minimum transaction fee:  `global MinTxnFee`.