Opus - jasonxiale's results

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L1379-L1397

Vulnerability details

Impact

shrine.withdraw_helper updates trove's deposit incorrectly, which might cause trove lose some asset

Proof of Concept

In shrine.withdraw_helper, the function will first read trove's deposit - withdraw amount, and finally updates the trove's deposit

1378         // Withdraw a specified amount of a Yang from a Trove
1379         fn withdraw_helper(ref self: ContractState, yang: ContractAddress, trove_id: u64, amount: Wad) {
1380             let yang_id: u32 = self.get_valid_yang_id(yang);
1381
1382             // Fails if amount > amount of yang deposited in the given trove
1383             let trove_balance: Wad = self.deposits.read((yang_id, trove_id)); <<<--- Here read user's deposit
1384             assert(trove_balance >= amount, 'SH: Insufficient yang balance');
1385
1386             let new_trove_balance: Wad = trove_balance - amount;
1387             let new_total: Wad = self.yang_total.read(yang_id) - amount;
1388
1389             self.charge(trove_id);
1390
1391             self.yang_total.write(yang_id, new_total);
1392             self.deposits.write((yang_id, trove_id), new_trove_balance); <<<--- Here write user's deposits
1393
1394             // Emit events
1395             self.emit(YangTotalUpdated { yang, total: new_total });
1396             self.emit(DepositUpdated { yang, trove_id, amount: new_trove_balance });
1397         }

The issue is that between self.deposits.read and self.deposits.write, self.charge(trove_id); is called And in shrine.charge, the trove's deposit might be updated in shrine.cairo#L1426, in such case, the trove's deposit will be incorrect

1400         fn charge(ref self: ContractState, trove_id: u64) {
    ...
1419 
1420             // If there was any exceptional redistribution, write updated yang amounts to trove
1421             if updated_trove_yang_balances.is_some() {
1422                 let mut updated_trove_yang_balances = updated_trove_yang_balances.unwrap();
1423                 loop {
1424                     match updated_trove_yang_balances.pop_front() {
1425                         Option::Some(yang_balance) => {
1426                             self.deposits.write((*yang_balance.yang_id, trove_id), *yang_balance.amount); <<<--- Here the trove's deposit will be updated
1427                         },
1428                         Option::None => { break; },
1429                     };
1430                 };
1431             }
    ...
1458         }

Tools Used

VIM

Recommended Mitigation Steps

diff --git a/src/core/shrine.cairo b/src/core/shrine.cairo
index 0236e95..4b47823 100644
--- a/src/core/shrine.cairo
+++ b/src/core/shrine.cairo
@@ -1379,6 +1379,8 @@ mod shrine {
         fn withdraw_helper(ref self: ContractState, yang: ContractAddress, trove_id: u64, amount: Wad) {
             let yang_id: u32 = self.get_valid_yang_id(yang);
 
+            self.charge(trove_id);
+
             // Fails if amount > amount of yang deposited in the given trove
             let trove_balance: Wad = self.deposits.read((yang_id, trove_id));
             assert(trove_balance >= amount, 'SH: Insufficient yang balance');
@@ -1386,8 +1388,6 @@ mod shrine {
             let new_trove_balance: Wad = trove_balance - amount;
             let new_total: Wad = self.yang_total.read(yang_id) - amount;
 
-            self.charge(trove_id);
-
             self.yang_total.write(yang_id, new_total);
             self.deposits.write((yang_id, trove_id), new_trove_balance);

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/gate.cairo#L191-L223 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/absorber.cairo#L667-L705

Vulnerability details

Impact

Both absorber and gate use the same mitigation for ERC4626 first depositor front-running vulnerability, but current implementation is not sufficient. By abusing the flaw, even though malicious attacker can't benefit from the mitigation, he can cause other normal users lose asset.

Proof of Concept

Because of absorber and gate use the same mitigation, I will take gate as example.

Suppose the yang's decimals is 18

When sentinel.add_yang is called to add yang to shrine, initial_yang_amt is passed to shrine.add_yang as mitigation to the inflat issue. And initial_yang_amt is set as INITIAL_DEPOSIT_AMT which is const INITIAL_DEPOSIT_AMT: u128 = 1000;

In sentinel.cairo#L204-L206, yang_erc20.transfer_from is called to transfer 1000 wei yang_erc from caller to gate

And then the code flow will fall into shrine.add_yang, when the function is called, initial_yang_amt is still 1000

In shrine.add_yang, the yang_total will be set to 1000 in shrine.cairo#L591

So when the admin(which is the first depositor) calls sentinel.add_yang, he will transfer 1000 wei yang_asset and he will recevie 1000 yang_amt yang. After that, when the second user calls abbot.open_trove, gate.convert_to_yang_helper will calculate his yang_amt by gate.cairo#L220

        fn convert_to_yang_helper(self: @ContractState, asset_amt: u128) -> Wad {
            let asset: IERC20Dispatcher = self.asset.read();
            let total_yang: Wad = self.get_total_yang_helper(asset.contract_address);

            if total_yang.is_zero() {
                let decimals: u8 = asset.decimals();
                // Otherwise, scale `asset_amt` up by the difference to match `Wad`
                // precision of yang. If asset is of `Wad` precision, then the same
                // value is returned
                fixed_point_to_wad(asset_amt, decimals)
            } else {
                (asset_amt.into() * total_yang) / get_total_assets_helper(asset).into() <<<--- the second user calcuates the `yang_amout` using this code
            }
        }

And gate.get_total_assets_helper is using asset.balance_of.

    #[inline(always)]
    fn get_total_assets_helper(asset: IERC20Dispatcher) -> u128 {
        asset.balance_of(get_contract_address()).try_into().unwrap() <<<--- balance_of is used here
    }

So to sum up, in current implementation:

1. the first depositor(which is also the admin) will depost 1000 wei ERC, and will receive 1000 yang_amt
2. the second depostor will get his yang_amt based of (asset_amt.into() * total_yang) / get_total_assets_helper(asset).into()

Suppose in a case that the asset is DAI(18 decimals)

1. when yang_dai is created, the admin calls add_yang to add DAI into the protocol, and he will transfer 1000 wei DAI to gate, and will receive 1000 yang_amt
2. a malicious tranfers (100000 * 1e18 - 1000 wei) DAI(which is worth 100000 USD) to gate
3. Alice a normal user deposit 199* 1e18 DAI(which is worth 199 USD) by calling abbot.open_trove, based on gate.cairo#L220, Alice will get 199 * 1e18 * 1000 / (100000 * 1e18) = 1 wei
4. If Alice wants to close her trove, gate.convert_to_assets will be used to calculate the asset amount, according to gate.cairo#L191-L204, gate.cairo#L202 will be used: ((yang_amt * get_total_assets_helper(asset).into()) / total_yang).val, because alice has 1 wei yang_amout, and get_total_assets_helper() is (100000 + 199) * 1e18, total_yang will be 1001 So Alice will get 1 * (100000 * 1e18 + 199*1e18) / (1000 + 1) = 100.0989010989011 * 1e18, which is 100 USD. And the remaining 99*1e18 DAI will be in the gate.

In the above case, there will be two issue:

1. Alice will lose her asset
2. After Alice's close her trove, the system's balance will be (100000 + 99)*1e18, which is becoming larger. And the balance will keep growing with time going, which will cause users losing more asset.

Tools Used

VIM

Recommended Mitigation Steps

In gate.get_total_assets_helper, don't use balance_of to calculate the amount, instead, define a new variables and record the deposited asset amount by the variables

Assessed type

ERC4626