`sendRequest()` and `sendRequestNative()` does not follow CEI pattern

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x5ece34905b39312928cf3e97eafbb67694ec7f17a4f7b3d5bc2f13840ae1804c Severity: minor

Description: Description\

In Most.sol, Both sendRequest() and sendRequestNative() does not follow Checks Effects Interaction pattern which is considered as anti reentrancy pattern to prevent functions from reentrancy kind of attacks.

See here @audit tags to get the issue

    function sendRequest(
        bytes32 srcTokenAddress,
        uint256 amount,
        bytes32 destReceiverAddress
    ) external whenNotPaused {
        if (amount == 0) revert ZeroAmount();

        IERC20 token = IERC20(bytes32ToAddress(srcTokenAddress));

        bytes32 destTokenAddress = supportedPairs[srcTokenAddress];
        if (destTokenAddress == 0x0) revert UnsupportedPair();

        // lock tokens in this contract
        // message sender needs to give approval else this tx will revert
        token.safeTransferFrom(msg.sender, address(this), amount);     . . . . .@audit // This external call should be at last of function call

        emit CrosschainTransferRequest(
            committeeId,
            destTokenAddress,
            amount,
            destReceiverAddress,
            requestNonce
        );

        ++requestNonce;     . . . . .  .  @audit // Effect should be before interaction i.e external call function
    }

    function sendRequestNative(
        bytes32 destReceiverAddress
    ) external payable whenNotPaused {
        uint256 amount = msg.value;
        if (amount == 0) revert ZeroAmount();

        bytes32 destTokenAddress = supportedPairs[
            addressToBytes32(wethAddress)
        ];

        if (destTokenAddress == 0x0) revert UnsupportedPair();

        (bool success, ) = wethAddress.call{value: amount}(            . . . . .@audit // This should be at last of function call
            abi.encodeCall(IWETH9.deposit, ())
        );

        if (!success) revert WrappingEth();

        emit CrosschainTransferRequest(
            committeeId,
            destTokenAddress,
            amount,
            destReceiverAddress,
            requestNonce
        );

        ++requestNonce;        . . . . .  .  @audit // Effect should be before interaction i.e external call function
    }

More info about CEI pattern can be checked here.

The issue here is both function does not protect against reentrancy as the functions follows anti pattern which is not recommended.

The call function should be called at last of function and requestNonce should be incremented before the external call function. This makes in compliance with CEI pattern.

POC:

Its not applicable as the issue is about anti smart contract coding pattern. To get the issue better, i would highlight that CEI pattern is followed in ink based Most contract send_request() function in which Effects i.erequest_Nonce` is incremented before the transfer of tokens i.e external call. This can be checked here

Recommendation to fix\ Follow CEI pattern in both function to protect against reentrancy kind of attacks.

    function sendRequest(
        bytes32 srcTokenAddress,
        uint256 amount,
        bytes32 destReceiverAddress
    ) external whenNotPaused {
        if (amount == 0) revert ZeroAmount();

        IERC20 token = IERC20(bytes32ToAddress(srcTokenAddress));

        bytes32 destTokenAddress = supportedPairs[srcTokenAddress];
        if (destTokenAddress == 0x0) revert UnsupportedPair();

+        ++requestNonce;

        // lock tokens in this contract
        // message sender needs to give approval else this tx will revert
        token.safeTransferFrom(msg.sender, address(this), amount);

        emit CrosschainTransferRequest(
            committeeId,
            destTokenAddress,
            amount,
            destReceiverAddress,
            requestNonce
        );

-        ++requestNonce;     
    }

    function sendRequestNative(
        bytes32 destReceiverAddress
    ) external payable whenNotPaused {
        uint256 amount = msg.value;
        if (amount == 0) revert ZeroAmount();

        bytes32 destTokenAddress = supportedPairs[
            addressToBytes32(wethAddress)
        ];

        if (destTokenAddress == 0x0) revert UnsupportedPair();

+        ++requestNonce;

        (bool success, ) = wethAddress.call{value: amount}(
            abi.encodeCall(IWETH9.deposit, ())
        );

        if (!success) revert WrappingEth();

        emit CrosschainTransferRequest(
            committeeId,
            destTokenAddress,
            amount,
            destReceiverAddress,
            requestNonce
        );

-        ++requestNonce;        
    }

[krzysztofziobro]

Doesn't qualify for minor: "negative impact on user experience, or putting one of the contracts in an unexpected state"