c4-bot-10 commented 8 months ago

Lines of code

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

Vulnerability details

when user call abbot.withdraw() -> shrine.withdraw() ->shrine.withdraw_helper()

The main logic is withdraw_helper()

        fn withdraw_helper(ref self: ContractState, yang: ContractAddress, trove_id: u64, amount: Wad) {
            let yang_id: u32 = self.get_valid_yang_id(yang);

            // 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');

            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);

            // Emit events
            self.emit(YangTotalUpdated { yang, total: new_total });
            self.emit(DepositUpdated { yang, trove_id, amount: new_trove_balance });
        }

Execution steps:

Calculate let new_trove_balance = trove_balance - amount;
Execute self.charge(trove_id);
self.deposits.write((yang_id, trove_id), new_trove_balance);

However, self.charge() may execute pull_redistributed_debt_and_yangs() -> self.deposits.write(), retrieving redistributed yang. These retrieved redistributed yang will be overwritten by the temporary variable new_trove_balance in the third step. This part of the redistributed yang will be lost.

Impact

redistributed yang will lost

Recommended Mitigation

charge first

        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');

            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);

            // Emit events
            self.emit(YangTotalUpdated { yang, total: new_total });
            self.emit(DepositUpdated { yang, trove_id, amount: new_trove_balance });
        }