Add inner transaction fee and clear state transaction check issues to Algorand not-so-smart-contracts

SUMMARY.md

@@ -27,6 +27,8 @@
     - [Access Controls](./not-so-smart-contracts/algorand/access_controls/README.md)
     - [Asset Id Check](./not-so-smart-contracts/algorand/asset_id_check/README.md)
     - [Denial of Service](./not-so-smart-contracts/algorand/denial_of_service/README.md)
+    - [Inner Transaction Fee](./not-so-smart-contracts/algorand/inner_transaction_fee/README.md)
+    - [Clear State Transaction Check](./not-so-smart-contracts/algorand/clear_state_transaction_check/README.md)
   - [Cairo](./not-so-smart-contracts/cairo/README.md)
     - [Improper access controls](./not-so-smart-contracts/cairo/access_controls/README.md)
     - [Integer division errors](./not-so-smart-contracts/cairo/integer_division/README.md)

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

@@ -14,17 +14,19 @@ Each _Not So Smart Contract_ includes a standard set of 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 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                           |
+| [Inner Transaction Fee](inner_transaction_fee)                 | Inner transaction fee should be set to zero                                    | no                             | yes                           |
+| [Clear State Transaction Check](clear_state_transaction_check) | Contract does not check OnComplete field of an Application Call                | yes                            | yes                           |
 
 ## Credits
 

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

@@ -0,0 +1,40 @@
+# Clear State Transaction Check
+
+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.
+
+## Description
+
+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.
+
+## Exploit Scenario
+
+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.
+
+```py
+@router.method(no_op=CallConfig.CALL)
+def take_flash_loan(offset: abi.Uint64, amount: abi.Uint64) -> Expr:
+    return Seq([
+        # Ensure the pay_flash_loan method is called
+        Assert(And(
+            Gtxn[Txn.group_index() + offset.get()].type_enum == TxnType.ApplicationCall,
+            Gtxn[Txn.group_index() + offset.get()].application_id() == Global.current_application_id(),
+            Gtxn[Txn.group_index() + offset.get()].application_args[0] == MethodSignature("pay_flash_loan(uint64)")
+        )),
+        # Perform other validations, transfer assets to the user, update the global state
+        # [...]
+    ])
+
+@router.method(no_op=CallConfig.CALL)
+def pay_flash_loan(offset: abi.Uint64) -> Expr:
+    return Seq([
+        # Validate the "take_flash_loan" transaction at `Txn.group_index() - offset.get()`
+        # Ensure the user has returned the funds to the pool along with the fee. Fail the transaction otherwise
+        # [...]
+    ])
+```
+
+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.
+
+## Recommendations
+
+Validate the OnComplete field of the ApplicationCall transactions.

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

@@ -0,0 +1,38 @@
+# Inner Transaction Fee
+
+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.
+
+## Description
+
+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.
+
+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.
+
+## Exploit Scenarios
+
+```py
+@router.method(no_op=CallConfig.CALL)
+def mint(pay: abi.PaymentTransaction) -> Expr:
+    return Seq([
+            # perform validations and other operations
+            # [...]
+            # mint wrapped-asset id
+            InnerTxnBuilder.Begin(),
+            InnerTxnBuilder.SetFields(
+                {
+                    TxnField.type_enum: TxnType.AssetTransfer,
+                    TxnField.asset_receiver: Txn.sender(),
+                    TxnField.xfer_asset: wrapped_algo_asset_id,
+                    TxnField.asset_amount: pay.get().amount(),
+                }
+            ),
+            InnerTxnBuilder.Submit(),
+            # [...]
+    ])
+```
+
+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.
+
+## Recommendations
+
+Explicitly set the inner transaction fees to zero and use the fee pooling feature of the Algorand.