Skip to content

Commit

Permalink
Merge pull request #172 from crytic/update-not-so-algorand
Browse files Browse the repository at this point in the history
Update not-so-algorand
montyly authored Nov 29, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
2 parents 7bfbb7e + 0c4b5c6 commit b9d3b8f
Showing 4 changed files with 17 additions and 12 deletions.
22 changes: 11 additions & 11 deletions not-so-smart-contracts/algorand/README.md
Original file line number Diff line number Diff line change
@@ -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

2 changes: 2 additions & 0 deletions not-so-smart-contracts/algorand/group_size_check/README.md
Original file line number Diff line number Diff line change
@@ -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.
3 changes: 3 additions & 0 deletions not-so-smart-contracts/algorand/rekeying/README.md
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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`.

0 comments on commit b9d3b8f

Please sign in to comment.