Skip to content

Commit 84eada3

Browse files
authored
Merge pull request #294 from crytic/algorand-fee-clear
Add inner transaction fee and clear state transaction check issues to Algorand not-so-smart-contracts
2 parents a193204 + f4e83f2 commit 84eada3

File tree

4 files changed

+93
-11
lines changed

4 files changed

+93
-11
lines changed

SUMMARY.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@
2727
- [Access Controls](./not-so-smart-contracts/algorand/access_controls/README.md)
2828
- [Asset Id Check](./not-so-smart-contracts/algorand/asset_id_check/README.md)
2929
- [Denial of Service](./not-so-smart-contracts/algorand/denial_of_service/README.md)
30+
- [Inner Transaction Fee](./not-so-smart-contracts/algorand/inner_transaction_fee/README.md)
31+
- [Clear State Transaction Check](./not-so-smart-contracts/algorand/clear_state_transaction_check/README.md)
3032
- [Cairo](./not-so-smart-contracts/cairo/README.md)
3133
- [Improper access controls](./not-so-smart-contracts/cairo/access_controls/README.md)
3234
- [Integer division errors](./not-so-smart-contracts/cairo/integer_division/README.md)

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

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,17 +14,19 @@ Each _Not So Smart Contract_ includes a standard set of information:
1414

1515
## Vulnerabilities
1616

17-
| Not So Smart Contract | Description | Applicable to smart signatures | Applicable to smart contracts |
18-
| ------------------------------------------------------- | ------------------------------------------------------------------------------ | ------------------------------ | ----------------------------- |
19-
| [Rekeying](rekeying) | Attacker rekeys an account | yes | yes |
20-
| [Unchecked Transaction Fees](unchecked_transaction_fee) | Attacker sets excessive fees for smart signature transactions | yes | no |
21-
| [Closing Account](closing_account) | Attacker closes smart signature accounts | yes | no |
22-
| [Closing Asset](closing_asset) | Attacker transfers entire asset balance of a smart signature | yes | no |
23-
| [Group Size Check](group_size_check) | Contract does not check transaction group size | yes | yes |
24-
| [Time-based Replay Attack](time_based_replay_attack) | Contract does not use lease for periodic payments | yes | no |
25-
| [Access Controls](access_controls) | Contract does not enfore access controls for updating and deleting application | no | yes |
26-
| [Asset Id Check](asset_id_check) | Contract does not check asset id for asset transfer operations | yes | yes |
27-
| [Denial of Service](denial_of_service) | Attacker stalls contract execution by opting out of a asset | yes | yes |
17+
| Not So Smart Contract | Description | Applicable to smart signatures | Applicable to smart contracts |
18+
| -------------------------------------------------------------- | ------------------------------------------------------------------------------ | ------------------------------ | ----------------------------- |
19+
| [Rekeying](rekeying) | Attacker rekeys an account | yes | yes |
20+
| [Unchecked Transaction Fees](unchecked_transaction_fee) | Attacker sets excessive fees for smart signature transactions | yes | no |
21+
| [Closing Account](closing_account) | Attacker closes smart signature accounts | yes | no |
22+
| [Closing Asset](closing_asset) | Attacker transfers entire asset balance of a smart signature | yes | no |
23+
| [Group Size Check](group_size_check) | Contract does not check transaction group size | yes | yes |
24+
| [Time-based Replay Attack](time_based_replay_attack) | Contract does not use lease for periodic payments | yes | no |
25+
| [Access Controls](access_controls) | Contract does not enfore access controls for updating and deleting application | no | yes |
26+
| [Asset Id Check](asset_id_check) | Contract does not check asset id for asset transfer operations | yes | yes |
27+
| [Denial of Service](denial_of_service) | Attacker stalls contract execution by opting out of a asset | yes | yes |
28+
| [Inner Transaction Fee](inner_transaction_fee) | Inner transaction fee should be set to zero | no | yes |
29+
| [Clear State Transaction Check](clear_state_transaction_check) | Contract does not check OnComplete field of an Application Call | yes | yes |
2830

2931
## Credits
3032

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
# Clear State Transaction Check
2+
3+
The lack of checks on the OnComplete field of the application calls might allow an attacker to execute the clear state program instead of the approval program, breaking core validations.
4+
5+
## Description
6+
7+
Algorand applications make use of group transactions to realize operations that may not be possible using a single transaction model. Some operations require that other transactions in the group call certain methods and applications. These requirements are asserted by validating that the transactions are ApplicationCall transactions. However, the OnComplete field of these transactions is not always validated, allowing an attacker to submit ClearState ApplicationCall transactions. The ClearState transaction invokes the clear state program instead of the intended approval program of the application.
8+
9+
## Exploit Scenario
10+
11+
A protocol offers flash loans from a liquidity pool. The flash loan operation is implemented using two methods: `take_flash_loan` and `pay_flash_loan`. `take_flash_loan` method transfers the assets to the user and `pay_flash_loan` verifies that the user has returned the borrowed assets. `take_flash_loan` verifies that a later transaction in the group calls the `pay_flash_loan` method. However, It does not validate the OnComplete field.
12+
13+
```py
14+
@router.method(no_op=CallConfig.CALL)
15+
def take_flash_loan(offset: abi.Uint64, amount: abi.Uint64) -> Expr:
16+
return Seq([
17+
# Ensure the pay_flash_loan method is called
18+
Assert(And(
19+
Gtxn[Txn.group_index() + offset.get()].type_enum == TxnType.ApplicationCall,
20+
Gtxn[Txn.group_index() + offset.get()].application_id() == Global.current_application_id(),
21+
Gtxn[Txn.group_index() + offset.get()].application_args[0] == MethodSignature("pay_flash_loan(uint64)")
22+
)),
23+
# Perform other validations, transfer assets to the user, update the global state
24+
# [...]
25+
])
26+
27+
@router.method(no_op=CallConfig.CALL)
28+
def pay_flash_loan(offset: abi.Uint64) -> Expr:
29+
return Seq([
30+
# Validate the "take_flash_loan" transaction at `Txn.group_index() - offset.get()`
31+
# Ensure the user has returned the funds to the pool along with the fee. Fail the transaction otherwise
32+
# [...]
33+
])
34+
```
35+
36+
An attacker constructs a valid group transaction for flash loan but sets the OnComplete field of `pay_flash_loan` call to ClearState. The clear state program is executed for complete_flash_loan call, which does not validate that the attacker has returned the funds. The attacker steals all the assets in the pool.
37+
38+
## Recommendations
39+
40+
Validate the OnComplete field of the ApplicationCall transactions.
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
# Inner Transaction Fee
2+
3+
Inner transaction fees are by default set to an estimated amount which are deducted from the application account if it is the Sender. An attacker can perform operations executing inner transactions and drain system funds, making it under-collateralized.
4+
5+
## Description
6+
7+
Inner transactions are initialized with Sender set to the application account and Fee to the minimum allowable, taking into account MinTxnFee and credit from overpaying in earlier transactions. The inner transaction fee depends on the transaction fee paid by the user. As a result, the user controls, to some extent, the fee paid by the application.
8+
9+
If the application does not explicitly set the Fee to zero, an attacker can burn the application’s balance in the form of fees. This also becomes an issue if the application implements internal bookkeeping to track the application balance and does not account for fees.
10+
11+
## Exploit Scenarios
12+
13+
```py
14+
@router.method(no_op=CallConfig.CALL)
15+
def mint(pay: abi.PaymentTransaction) -> Expr:
16+
return Seq([
17+
# perform validations and other operations
18+
# [...]
19+
# mint wrapped-asset id
20+
InnerTxnBuilder.Begin(),
21+
InnerTxnBuilder.SetFields(
22+
{
23+
TxnField.type_enum: TxnType.AssetTransfer,
24+
TxnField.asset_receiver: Txn.sender(),
25+
TxnField.xfer_asset: wrapped_algo_asset_id,
26+
TxnField.asset_amount: pay.get().amount(),
27+
}
28+
),
29+
InnerTxnBuilder.Submit(),
30+
# [...]
31+
])
32+
```
33+
34+
The application does not explicitly set the inner transaction fee to zero. When user mints wrapped-algo, some of the ALGO is burned in the form of fees. The amount of wrapped-algo in circulation will be greater than the application ALGO balance. The system will be under-collateralized.
35+
36+
## Recommendations
37+
38+
Explicitly set the inner transaction fees to zero and use the fee pooling feature of the Algorand.

0 commit comments

Comments
 (0)