_performFallbackCall can revert sliently when refundee is not capable of taking ETH refund from layerzero side

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L788 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L712 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L938

Vulnerability details

Impact

_performFallbackCall can revert sliently when refundee is not capable of taking ETH refund from layerzero side

Proof of Concept

In RootBridgeAgent.sol when the has fall back toggle flag is on, the smart contract aim to perform a fallback call to notify the dest chain the failure to deliver the message

for example, the relevent code is here

//Update tx state if execution failed
if (!success) {
    //Read the fallback flag.
    if (_hasFallbackToggled) {
        // Update tx state as retrieve only
        executionState[_srcChainId][_depositNonce] = STATUS_RETRIEVE;
        // Perform the fallback call
        _performFallbackCall(payable(_refundee), _depositNonce, _srcChainId);
    } else {
        // No fallback is requested revert allowing for retry.
        revert ExecutionFailure();
    }
}

and here when retrieve the deposit

    //Check if deposit is in retrieve mode
    if (executionState[_srcChainId][nonce] == STATUS_DONE) {
        revert AlreadyExecutedTransaction();
    } else {
        //Set settlement to retrieve mode, if not already set.
        if (executionState[_srcChainId][nonce] == STATUS_READY) {
            executionState[_srcChainId][nonce] = STATUS_RETRIEVE;
        }
        //Trigger fallback/Retry failed fallback
        _performFallbackCall(
            payable(address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED])))), nonce, _srcChainId
        );
    }

the logic of the _performFallbackCall is here

function _performFallbackCall(address payable _refundee, uint32 _depositNonce, uint16 _dstChainId) internal {
    //Sends message to LayerZero messaging layer
    ILayerZeroEndpoint(lzEndpointAddress).send{value: address(this).balance}(
        _dstChainId,
        getBranchBridgeAgentPath[_dstChainId],
        abi.encodePacked(bytes1(0x04), _depositNonce),
        payable(_refundee),
        address(0),
        ""
    );
}

the code forward all ETH (address(this).balance) and aim to use the ETH to pay for the layerzero fee

the problem is that such fallback is wrapped by the excessivelySafeCall

to not blocking other passing messaging via layerzero endpoint, the excessivelySafeCall will sliently revert but does not block transaction confirm

basically this means if there are revert (refundee cannot take the ETH) inside the excessivelySafeCall, fall back is never executed and even user toggle the fall back flag, asset can still be lost

which line of code will revert when refundee is not capable of taking ETH refund from layerzero side

after the message is sent via the endpoint, the layerzero endpoint foward the message to UltraLightNodeV2

the code quote the fee and refund the excessive fee to the refundee address

but if the refundee address is not capable of taking the ETH refund, transaction revert

transaction revert in this line of code

// assert the user has attached enough native token for this address
require(totalNativeFee <= msg.value, "LayerZero: not enough native for fees");
// refund if they send too much
uint amount = msg.value.sub(totalNativeFee);
if (amount > 0) {
    (bool success, ) = _refundAddress.call{value: amount}("");
    require(success, "LayerZero: failed to refund");
}

can add this test into the BranchBridgeAgentTest.t.sol to prove that the transaction revert when the refund address is not capable of taking the ETH refund

we need to add a smart contract code in the same file first

contract NoEth {

    // Constructor and other functions go here

    // This function gets executed when someone sends Ether to the contract directly
    receive() external payable {
        revert("Contract does not accept ETH");
    }

}

then add

      function testRefundTakesNoETH() public {

         // layerzero endpoint in arbitrum
        address LZEndpointArb = 0x3c2269811836af69497E5F486A85D7316753cf62;

        bytes memory fallbackData = abi.encodePacked(bytes1(0x04), uint32(1));

        // payload

        address user = vm.addr(5201134);
        deal(user, 1 ether);

        address remoteAddress = address(1);
        address localAddress = address(1);

        bytes memory remoteAndLocalAddresses = abi.encodePacked(remoteAddress, address(user));

        NoEth no_eth = new NoEth();

        vm.prank(user);
        ILayerZeroEndpoint(LZEndpointArb).send{value: 1 ether}(
            101,
            remoteAndLocalAddresses,
            fallbackData,
            payable(address(no_eth)),
            address(0),
            ""
        );

    }

and we can run the fork test

forge test -vvvv --match-test "testRefundTakesNoETH" --fork-url https://arb1.arbitrum.io/rpc

the transaction revert and we can see the error message:

  │   │   │   └─ ← 0x0000000000000000000000000000000000000000000000000005ca1dd1a70d94
    │   │   ├─ [2488] 0x3773E1E9Deb273fCdf9f80bc88bB387B1e6Ce34d::getFees(false, 6795147159134519 [6.795e15], 1629604303801748 [1.629e15]) [staticcall]
    │   │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000000
    │   │   ├─ [138] NoEth::receive{value: 991575248537063733}() 
    │   │   │   └─ ← "Contract does not accept ETH"
    │   │   └─ ← "LayerZero: failed to refund"
    │   └─ ← "LayerZero: failed to refund"
    └─ ← "LayerZero: failed to refund"

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 6.71s

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/ulysses-omnichain/BranchBridgeAgentTest.t.sol:BranchBridgeAgentTest
[FAIL. Reason: LayerZero: failed to refund] testRefundTakesNoETH() (gas: 271138)

note, the BranchBridgeAgent has the same problem when executing fallback call

submitting as medium because it may require user error but

it is very natural maybe for a smart contract that implement some check inside the receive function (only receive ETH from WETH contract)

and if that case, if such address is the refundee, perform fall back sliently revert

Tools Used

Manual Review, foundry

Recommended Mitigation Steps

call estimate fee before sending the fallback request to not send too much fee to avoid layerzero fee refund

also if the fallback revert, save the payload and allow user retry the fallback call later

Assessed type

DoS

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #887

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-judge]

alcueca marked the issue as not a duplicate

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a

[alcueca]

User error, as the warden points out.