handler's `receive_cross_chain_callback()` will always set the tx_status to `SETTLED` on source chain & burn the tokens (MintBurn Mode) even when the msg fails on destination

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-08-chakra/blob/main/cairo/handler/src/handler_erc20.cairo#L148-L161

Vulnerability details

When a validator invokes Settlement.cairo's receive_cross_chain_callback(), they will pass the cross_chain_msg_status as an input and base on that the msg status should be set on the source chain. Means, if the cross_chain_msg_status is given as SUCCESS then the msg status on the source chain will be set as CrossChainTxStatus::SETTLED and if it is not SUCCESS then it should simply set the status to CrossChainTxStatus::FAILED but in the cairo implementation, the cross_chain_msg_status is completely ignored:

        fn receive_cross_chain_callback(ref self: ContractState, cross_chain_msg_id: felt252, from_chain: felt252, to_chain: felt252,
        from_handler: u256, to_handler: ContractAddress, cross_chain_msg_status: u8) -> bool{
            assert(to_handler == get_contract_address(),'error to_handler');

            assert(self.settlement_address.read() == get_caller_address(), 'not settlement');

            assert(self.support_handler.read((from_chain, from_handler)) && 
                    self.support_handler.read((to_chain, contract_address_to_u256(to_handler))), 'not support handler');

            let erc20 = IERC20MintDispatcher{contract_address: self.token_address.read()};
            if self.mode.read() == SettlementMode::MintBurn{
                erc20.burn_from(get_contract_address(), self.created_tx.read(cross_chain_msg_id).amount);
            }
            let created_tx = self.created_tx.read(cross_chain_msg_id);
            ///@audit-issue M cross_chain_msg_status not checked like in the solidity instance, this will always set tx_status as settled and return true and never Failed!
            self.created_tx.write(cross_chain_msg_id, CreatedCrossChainTx{
                tx_id: created_tx.tx_id,
                from_chain: created_tx.from_chain,
                to_chain: created_tx.to_chain,
                from:created_tx.from,
                to:created_tx.to,
                from_token: created_tx.from_token,
                to_token: created_tx.to_token,
                amount: created_tx.amount,
@>              tx_status: CrossChainTxStatus::SETTLED
            });

            return true;
        }

The tx_status is directly set as SETTLED, which means it will be always get marked as SETTLED even when the Msg gets failed on the destination chain.

Also, as it doesn't check the cross_chain_msg_status, this will always burn the tokens (when the MODE is MintBurn), which is not the case when we look at the solidity instance

            if self.mode.read() == SettlementMode::MintBurn{
                erc20.burn_from(get_contract_address(), self.created_tx.read(cross_chain_msg_id).amount);
            }

Impact

• Wrong state update and tokens burning

Proof of Concept

In the solidity instance both the status is checked and tokens only get burned when the status gets marked as SETTLED:

        if (status == CrossChainMsgStatus.Success) {
            if (mode == SettlementMode.MintBurn) {
                _erc20_burn(address(this), create_cross_txs[txid].amount);
            }

            create_cross_txs[txid].status = CrossChainTxStatus.Settled;
        }

        if (status == CrossChainMsgStatus.Failed) {
            create_cross_txs[txid].status = CrossChainTxStatus.Failed;
        }

Tools Used

Shaheen's Vision

Recommended Mitigation Steps

Make sure to check the given status and mark the tx_status base on that.

Assessed type

Context

[alex-ppg]

The submission details how the callback leg of a MintBurn mode bridge will always burn the tokens regardless of whether the transaction actually succeeded on the destination chain.

This indicates a high-risk issue within the codebase that would lead to fund loss if the destination chain's execution fails.

[c4-judge]

alex-ppg marked the issue as satisfactory