Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GPT Review not-so-smart-contracts/algorand #278

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion not-so-smart-contracts/README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# (Not So) Smart Contracts

This repository contains examples of common smart contract vulnerabilities, including code from real smart contracts. Use Not So Smart Contracts to learn about vulnerabilities, as a reference when performing security reviews, and as a benchmark for security and analysis tools:
This repository contains examples of common smart contract vulnerabilities, including code taken from actual smart contracts. Use Not So Smart Contracts as a learning resource for understanding vulnerabilities, as a reference during security reviews, and as a benchmark for security and analysis tools:

- [Algorand](./algorand/README.md)
- [Cairo](./cairo/README.md)
Expand Down
30 changes: 15 additions & 15 deletions not-so-smart-contracts/algorand/README.md
Original file line number Diff line number Diff line change
@@ -1,33 +1,33 @@
# (Not So) Smart Contracts

This repository contains examples of common Algorand smart contract vulnerabilities, including code from real smart contracts. Use Not So Smart Contracts to learn about Algorand vulnerabilities, as a reference when performing security reviews, and as a benchmark for security and analysis tools.
This repository contains examples of common Algorand smart contract vulnerabilities, including code from real smart contracts. Use the Not So Smart Contracts to learn about Algorand vulnerabilities, as a reference when performing security reviews, and as a benchmark for security and analysis tools.

## Features

Each _Not So Smart Contract_ includes a standard set of information:

- Description of the vulnerability type
- Attack scenarios to exploit the vulnerability
- Recommendations to eliminate or mitigate the vulnerability
- Real-world contracts that exhibit the flaw
- Recommendations for eliminating or mitigating the vulnerability
- Real-world contracts exhibiting the flaw
- References to third-party resources with more information

## Vulnerabilities

| 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 |
| 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 the 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 enforce access controls for updating and deleting applications | 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 an asset | yes | yes |

## Credits

These examples are developed and maintained by [Trail of Bits](https://www.trailofbits.com/).

If you have questions, problems, or just want to learn more, then join the #ethereum channel on the [Empire Hacking Slack](https://empireslacking.herokuapp.com/) or [contact us](https://www.trailofbits.com/contact/) directly.
If you have questions, issues, or simply want to learn more, join the #ethereum channel on the [Empire Hacking Slack](https://empireslacking.herokuapp.com/) or [contact us](https://www.trailofbits.com/contact/) directly.
11 changes: 5 additions & 6 deletions not-so-smart-contracts/algorand/access_controls/README.md
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
# Access Controls

Lack of appropriate checks for application calls of type UpdateApplication and DeleteApplication allows attackers to update applications code or delete an application entirely.
Inadequate checks for application calls of type UpdateApplication and DeleteApplication can allow attackers to alter an application's code or delete it entirely.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably stick with previously used update instead of alter an (?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 , update is more appropriated here


## Description

When an application call is successful, additional operations are executed based on the OnComplete field. If the OnComplete field is set to UpdateApplication the approval and clear programs of the application are replaced with the programs specified in the transaction. Similarly, if the OnComplete field is set to DeleteApplication, application parameters are deleted.
This allows attackers to update or delete the application if proper access controls are not enforced in the application.
When an application call is successful, further operations are carried out based on the OnComplete field. If the OnComplete field is set to UpdateApplication, the approval and clear programs of the application get replaced with the programs specified in the transaction. Likewise, if the OnComplete field is set to DeleteApplication, the application parameters get deleted. This can let attackers update or delete the application if proper access controls are not in place.

## Exploit Scenarios

A stateful contract serves as a liquidity pool for a pair of tokens. Users can deposit the tokens to get the liquidity tokens and can get back their funds with rewards through a burn operation. The contract does not enforce restrictions for UpdateApplication type application calls. Attacker updates the approval program with a malicious program that transfers all assets in the pool to the attacker's address.
A stateful contract functions as a liquidity pool for two tokens. Users can deposit these tokens to acquire liquidity tokens and can retrieve their funds along with rewards via a burn operation. The contract, however, does not enforce any restrictions for UpdateApplication type application calls. An attacker can update the approval program with a malicious program that transfers all the assets in the pool to their address.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a stateful contract functions should be change, as function as a specific meaning.


## Recommendations

- Set proper access controls and apply various checks before approving applications calls of type UpdateApplication and DeleteApplication.
- Implement appropriate access controls and conduct various checks before approving application calls of type UpdateApplication and DeleteApplication.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conduct should be reverted I think


- Use [Tealer](https://github.com/crytic/tealer) to detect this issue.
- Utilize [Tealer](https://github.com/crytic/tealer) to identify this issue.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be reverted I think

12 changes: 6 additions & 6 deletions not-so-smart-contracts/algorand/asset_id_check/README.md
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
# Asset Id Check
# Asset ID Check

Lack of verification of asset id in the contract allows attackers to transfer a different asset in place of the expected asset and mislead the application.
Failing to verify the asset ID in a contract can allow attackers to transfer a different asset instead of the expected one, potentially misleading the application.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lack of verification was better I think.


## Description

Contracts accepting and doing operations based on the assets transferred to the contract must verify that the transferred asset is the expected asset by checking the asset Id. Absence of check for expected asset Id could allow attackers to manipulate contracts logic by transferring a fake, less or more valuable asset instead of the correct asset.
Contracts that accept and perform operations based on assets transferred to them must verify that the transferred asset is indeed the expected asset by checking the asset ID. Neglecting to check the expected asset ID could enable attackers to manipulate the contract's logic by transferring a fake, less valuable, or more valuable asset instead of the correct one.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neglecting should be reverted


## Exploit Scenarios

- A liquidity pool contract mints liquidity tokens on deposit of two tokens. Contract does not check that the asset Ids in the two asset transfer transactions are correct. Attacker deposits the same less valuable asset in the two transactions and withdraws both tokens by burning the pool tokens.
- User creates a delegate signature that allows recurring transfers of a certain asset. Attacker creates a valid asset transfer transaction of more valuable assets.
- A liquidity pool contract mints liquidity tokens upon the deposit of two tokens. The contract does not verify that the asset IDs in the two asset transfer transactions are correct. The attacker deposits the same, less valuable asset in both transactions and withdraws both tokens by burning the pool tokens.
- A user creates a delegate signature that permits recurring transfers of a certain asset. The attacker then generates a valid asset transfer transaction involving more valuable assets.

## Examples

Expand All @@ -35,4 +35,4 @@ def withdraw_asset(

## Recommendations

Verify the asset id to be expected asset for all asset related operations in the contract.
Ensure that the asset ID is verified as the expected asset for all asset-related operations in the contract.
14 changes: 7 additions & 7 deletions not-so-smart-contracts/algorand/closing_account/README.md
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
# Closing Account
# Closing Accounts

Lack of check for CloseRemainderTo transaction field in smart signatures allows attackers to transfer entire funds of the contract account or the delegators account to their account.
A lack of checks for the CloseRemainderTo transaction field in smart signatures allows attackers to transfer the entire funds of a contract account or a delegator's account to their own account.

## Description

Algorand accounts must satisfy minimum balance requirement and protocol rejects transactions whose execution results in account balance lower than the required minimum. In order to transfer the entire balance and close the account, users should use the CloseRemainderTo field of a payment transaction. Setting the CloseRemainderTo field transfers the entire account balance remaining after transaction execution to the specified address.
Algorand accounts must meet the minimum balance requirement, and the protocol rejects transactions if their execution would result in an account balance lower than the required minimum. To transfer the entire balance and close an account, users should use the CloseRemainderTo field of a payment transaction. Setting the CloseRemainderTo field transfers the remaining account balance after transaction execution to the specified address.

Any user with access to the smart signature may construct and submit the transactions using the smart signature. The smart signatures approving payment transactions have to ensure that the CloseRemainderTo field is set to the ZeroAddress or any other specific address to avoid unintended transfer of funds.
Any user with access to the smart signature can construct and submit transactions using that smart signature. To avoid unintended fund transfers, smart signatures approving payment transactions must ensure that the CloseRemainderTo field is set to the ZeroAddress or another specific address.

## Exploit Scenarios

A user creates a delegate signature for recurring payments. Attacker creates a valid transaction and sets the CloseRemainderTo field to their address.
A user creates a delegate signature for recurring payments. An attacker creates a valid transaction and sets the CloseRemainderTo field to their own address.

## Examples

Note: This code contains several other vulnerabilities, see [Rekeying](../rekeying), [Unchecked Transaction Fees](../unchecked_transaction_fee), [Time-based Replay Attack](../time_based_replay_attack).
Note: This code contains several other vulnerabilities. See [Rekeying](../rekeying), [Unchecked Transaction Fees](../unchecked_transaction_fee), [Time-based Replay Attack](../time_based_replay_attack).

```py
def withdraw(
Expand All @@ -36,4 +36,4 @@ def withdraw(

## Recommendations

Verify that the CloseRemainderTo field is set to the ZeroAddress or to any intended address before approving the transaction in the Teal contract.
Before approving a transaction in the Teal contract, verify that the CloseRemainderTo field is set to the ZeroAddress or another intended address.
12 changes: 6 additions & 6 deletions not-so-smart-contracts/algorand/closing_asset/README.md
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
# Closing Asset

Lack of check for AssetCloseTo transaction field in smart signatures allows attackers to transfer the entire asset balance of the contract account or the delegators account to their account.
A lack of checks for the AssetCloseTo transaction field in smart signatures enables attackers to transfer the entire asset balance of the contract account or the delegator's account to their account.

## Description

Algorand supports Fungible and Non Fungible Tokens using Algorand Standard Assets(ASA). An Algorand account must first opti-in to the asset before that account can receive any tokens. Opting to an asset increases the minimum balance requirement of the account. Users can opt-out of the asset and decrease the minimum balance requirement using the AssetCloseTo field of Asset Transfer transaction. Setting the AssetCloseTo field transfers the accounts entire token balance remaining after transaction execution to the specified address.
Algorand supports Fungible and Non-Fungible Tokens using Algorand Standard Assets (ASA). An Algorand account must first opt-in to the asset before it can receive any tokens. Opting-in to an asset increases the minimum balance requirement of the account. Users can opt-out of the asset and decrease the minimum balance requirement by using the AssetCloseTo field of the Asset Transfer transaction. Setting the AssetCloseTo field transfers the account's entire token balance remaining after the transaction execution to the specified address.

Any user with access to the smart signature may construct and submit the transactions using the smart signature. The smart signatures approving asset transfer transactions have to ensure that the AssetCloseTo field is set to the ZeroAddress or any other specific address to avoid unintended transfer of tokens.
Any user with access to the smart signature may construct and submit transactions using the smart signature. Smart signatures approving asset transfer transactions must ensure that the AssetCloseTo field is set to the ZeroAddress or any other specific address to prevent unintended token transfers.

## Exploit Scenarios

User creates a delegate signature that allows recurring transfers of a certain asset. Attacker creates a valid asset transfer transaction with AssetCloseTo field set to their address.
A user creates a delegate signature that allows recurring transfers of a certain asset. An attacker then creates a valid asset transfer transaction with the AssetCloseTo field set to their address.

## Examples

Note: This code contains several other vulnerabilities, see [Rekeying](../rekeying), [Unchecked Transaction Fees](../unchecked_transaction_fee), [Closing Asset](../closing_asset), [Time-based Replay Attack](../time_based_replay_attack), [Asset Id Check](../asset_id_check).
Note: This code contains several other vulnerabilities, such as [Rekeying](../rekeying), [Unchecked Transaction Fees](../unchecked_transaction_fee), [Closing Asset](../closing_asset), [Time-based Replay Attack](../time_based_replay_attack), and [Asset ID Check](../asset_id_check).

```py
def withdraw_asset(
Expand All @@ -36,4 +36,4 @@ def withdraw_asset(

## Recommendations

Verify that the AssetCloseTo field is set to the ZeroAddress or to the intended address before approving the transaction in the Teal contract.
Before approving the transaction in the Teal contract, make sure to verify that the AssetCloseTo field is set to the ZeroAddress or the intended address.
Loading