`BranchBridgeAgent.retrieveDeposit` doesn't check if the deposit is in FAILED state

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L422-L432

Vulnerability details

Impact

The BranchBridgeAgent.retrieveDeposit function is supposed to be called for retrieving a failed deposit in order to get funds back to branch chain, but the function doesn't check that the deposit being used is really in FAILED state, so a malicious user can call the function to retrieve a successful deposit, this will allow him to get funds on both the branche and root chains.

Proof of Concept

The issue occurs in the BranchBridgeAgent.retrieveDeposit function below :

function retrieveDeposit(
    uint32 _depositNonce,
    GasParams calldata _gParams
) external payable override lock {
    // Check if the deposit belongs to the message sender
    if (getDeposit[_depositNonce].owner != msg.sender)
        revert NotDepositOwner();
    //@audit not checking deposit status is FAILED

    //Encode Data for cross-chain call.
    bytes memory payload = abi.encodePacked(
        bytes1(0x08),
        msg.sender,
        _depositNonce
    );

    //Update State and Perform Call
    _performCall(payable(msg.sender), payload, _gParams);
}

As you can see in the function doesn't at all check the deposit status (if it's FAILED or SUCCESS), even though the function comments say that it should only be called for Failed deposit :

 /**
 * @notice External function to request tokens back to branch chain after failing omnichain environment interaction.
 *    @param depositNonce Identifier for user deposit to retrieve.
 *    @param gasParams gas parameters for the cross-chain call.
 *    @dev DEPOSIT ID: 8
 *
 */
function retrieveDeposit(uint32 depositNonce, GasParams calldata gasParams) external payable;

Because there is no check on the deposit state a malicious user can creates a deposit (for example using callOutAndBridge) and after the deposit has succeeded and the assets are transferred to the user on to the root chain, he will be able to call the retrieveDeposit function to change the deposit state to FAILED.

After this the malicious user can now call the redeemDeposit function which is callable when a deposit has failed, and this function will give his back his assets on the branche chain. In the end the user will have his assets back on the branche chain and will get new assets in the Root chain.

Tools Used

Manual review

Recommended Mitigation Steps

The BranchBridgeAgent.retrieveDeposit function should check that the deposit status is FAILED :

function retrieveDeposit(
    uint32 _depositNonce,
    GasParams calldata _gParams
) external payable override lock {
    // Check if the deposit belongs to the message sender
    if (getDeposit[_depositNonce].owner != msg.sender)
        revert NotDepositOwner();
    //@audit Check if deposit has failed
    if (getDeposit[_depositNonce].status != == STATUS_FAILED)
        revert();

    //Encode Data for cross-chain call.
    bytes memory payload = abi.encodePacked(
        bytes1(0x08),
        msg.sender,
        _depositNonce
    );

    //Update State and Perform Call
    _performCall(payable(msg.sender), payload, _gParams);
}

Assessed type

Invalid Validation

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #701

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid