Honest users could lose funds due to the current implementation of `executeProposal()`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-axelar/blob/2f9b234bb8222d5fbe934beafede56bfb4522641/contracts/cgp/governance/InterchainGovernance.sol#L68-L79

Vulnerability details

Impact

In the InterChainGovernance.sol contract, the executeProposal function lacks an explicit check to ensure that the msg.value provided with the function call is greater than or equal to the nativeValue specified.

After an extensive discussion with the sponsors, we came to an conclusion that

If less amount is provided in executeProposal() and for some reason there is still enough balance in the contract then the function will not revert. This allows for the msg.value to be sent along with the caller, or for some other entity to pay for the call by simply funding the contract right before the caller executes the proposal. Note the payable receive function found at the bottom of the contract.

This potential oversight easily leads to possible front-running attacks, where an attacker can observe pending transactions and then scoop up the funds in the contract to execute their transactions ahead of honest users, see POC.

Proof of Concept

Take a look at InterchainGovernance.sol#L68-L79

    /**
     * @notice Executes a proposal
     * @dev The proposal is executed by calling the target contract with calldata. Native value is
     * transferred with the call to the target contract.
     * @param target The target address of the contract to call
     * @param callData The data containing the function and arguments for the contract to call
     * @param nativeValue The amount of native token to send to the target contract
     //@audit there is no explicit checks to ensure that the msg.value provided with the function call is >= the nativeValue specified
     */
    function executeProposal(
        address target,
        bytes calldata callData,
        uint256 nativeValue
    ) external payable {
        bytes32 proposalHash = keccak256(abi.encodePacked(target, callData, nativeValue));

        _finalizeTimeLock(proposalHash);
        _call(target, callData, nativeValue);

        emit ProposalExecuted(proposalHash, target, callData, nativeValue, block.timestamp);
    }

Here is a hypothetical PoC scenario:

1. A legitimate user (Alice) funds the contract.
2. Alice then tries to call the executeProposal() function to forward their native value to a target contract.
3. An attacker (Bob) is watching the mempool for any queries to this function.
4. Seeing Alice's transaction, Bob also calls the function with a higher gas fee and a new target address. This allows Bob's transaction to be mined before Alice's.
5. As a result, Bob can take the native values Alice intended to use for her proposal, leading to a loss for Alice.

As seen the executeProposal() function, doesn't prevent this scenario, note that this issue is also compounded by the presence of the payable receive() function, making it possible for funds to be present in the contract:

receive() external payable {}

Tool Used

Manual Audit

Recommended Mitigation Steps

As concluded by the discussion with the sponsors

I think it would always be safest to pass the native value as msg.value from the caller that would execute the function.

So a fix should be to include an explicit check in the executeProposal() function to ensure that the msg.value provided is equal to or exceeds the nativeValue specified. This addition will prevent an attacker from front-running a legitimate call to the function with a higher gas fee. since they have to provide the amount of native value they would like to get to the target contract, code implemntation of the fix could be the below:

require(msg.value >= nativeValue, "Provided value is less than the required native value");

Additionally I'd recommended to ensure a mechanism exists for securely withdrawing any funds sent to the contract via the payable fallback function, incase a user mistakenly sends them to the contracts, that's asides via complex executions, access to this withdrawal function should be strictly controlled, e.g., by allowing only the contract owner to perform the action.

Assessed type

Other

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

deanamiel marked the issue as sponsor disputed

[deanamiel]

Only Axelar is meant to send nativeValue to the contract, and nothing can be executed without the Axelarnet governance process of voting and approval.

[c4-judge]

berndartmueller marked the issue as unsatisfactory: Invalid