caventa - receiveETH could spend fee

[sherlock-admin]

caventa

high

receiveETH could spend fee

Summary

The receiveETH function in the AvailBridge contract currently allows the possibility of send fees, which contradicts the intended behavior. The absence of proper fee checking can result in users obtaining the entire value sent to receiveETH, leading to an accumulation of fees that cannot be utilized.

Vulnerability Detail

Contrary to the expected behavior, the receiveETH function lacks sufficient checks to prevent the spending of fees. Users could potentially receive the full value of the Ether sent to receiveETH, even if there are associated fees. This oversight poses a risk to the proper functioning of fee management within the contract.

Impact

The impact of this vulnerability is that fees could be collected through the receiveETH function, but the corresponding withdrawFees mechanism might be unable to utilize these fees. This situation could lead to a discrepancy between the accumulated fees and their actual usability, impacting the expected financial flow within the AvailBridge contract.

Code Snippet

https://github.com/sherlock-audit/2023-12-avail/blob/main/contracts/src/AvailBridge.sol#L171-L179 https://github.com/sherlock-audit/2023-12-avail/blob/main/contracts/src/AvailBridge.sol#L239-L263

Tool used

Manual Review and wrote a simple test

Recommendation

Change receiveEth

function receiveETH(Message calldata message, MerkleProofInput calldata input)
        external
        whenNotPaused
        onlySupportedDomain(message.originDomain, message.destinationDomain)
        onlyTokenTransfer(message.messageType)
        nonReentrant
    {
        (bytes32 assetId, uint256 value) = abi.decode(message.data, (bytes32, uint256));
        if (assetId != ETH_ASSET_ID) {
            revert InvalidAssetId();
        }

+++ require(address(this).balance - value >= fees);

        _checkBridgeLeaf(message, input);

        // downcast SCALE-encoded bytes to an Ethereum address
        address dest = address(bytes20(message.to));

        emit MessageReceived(message.from, dest, message.messageId);

        // slither-disable-next-line arbitrary-send-eth,missing-zero-check,low-level-calls
        (bool success,) = dest.call{value: value}("");
        if (!success) {
            revert UnlockFailed();
        }
    }

[sherlock-admin]

2 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

Invalid

takarez commented:

invalid because { invalid: should provide a POC}

[sherlock-admin]

Escalate

Below is the foundry test added to the existing AvailBridgeTest.t.sol, which comprises two scenarios:

1st Scenario : Executing sendMessage followed by withdrawFees => no problem 2nd Scenario: Executing sendMessage then receiveEth, and then withdrawFees => fail because the fee is moved by receiveEth.

 function test_withdrawFees() external {

        // Scenario 1

        {
            // Initiate variables
            bytes32 rangeHash = 0x0100000000000000000000000000000000000000000000000000000000000000;
            uint64 messageId = 1;
            bytes32 to = 0x0100000000000000000000000000000000000000000000000000000000000000;
            bytes memory data = new bytes(1);
            data[0] = 0x01;
            uint32 feePerByte = 1;
            uint248 amount = 1_000;

            address from = makeAddr("from");

            // Check balance
            assertEq(address(bridge).balance, 0); // Balance is 0
            assertEq(bridge.fees(), 0); // Fee is 0

            // Send message
            {
                vm.prank(owner);
                bridge.updateFeePerByte(feePerByte);
                IAvailBridge.Message memory message = IAvailBridge.Message(0x01, bytes32(bytes20(from)), to, 2, 1, data, 0);
                vm.prank(from);
                vm.deal(from, amount);
                bridge.sendMessage{value: amount}(to, data);
            }

            // Check balance and fee
            assertEq(address(bridge).balance, 1_000); // Balance is 1_000
            assertEq(bridge.fees(), 1_000); // Fee is 1_000

            // Withdraw fees => no problem
            bridge.withdrawFees();

            // Check balance and fee again
            assertEq(address(bridge).balance, 0); // Balance is 0
            assertEq(bridge.fees(), 0); // Fee is 0
        }

        // Scenario 2

        {
            // Initiate variables
            bytes32 rangeHash = 0x0100000000000000000000000000000000000000000000000000000000000000;
            uint64 messageId = 1;
            bytes32 to = 0x0100000000000000000000000000000000000000000000000000000000000000;
            bytes memory data = new bytes(1);
            data[0] = 0x01;
            uint32 feePerByte = 1;
            uint248 amount = 1_000;

            address from = makeAddr("from");

            // Check balance
            assertEq(address(bridge).balance, 0); // Balance is 0
            assertEq(bridge.fees(), 0); // Fee is 0

            // Send message
            {
                vm.prank(owner);
                bridge.updateFeePerByte(feePerByte);
                address from = makeAddr("from");
                IAvailBridge.Message memory message = IAvailBridge.Message(0x01, bytes32(bytes20(from)), to, 2, 1, data, 0);
                vm.prank(from);
                vm.deal(from, amount);
                bridge.sendMessage{value: amount}(to, data);
            }

            // Check balance and fee
            assertEq(address(bridge).balance, 1_000); // Balance is 1_000
            assertEq(bridge.fees(), 1_000); // Fee is 1_000

            // receiveETH (This function call does not exist in scenario 1)
            {
                address to = makeAddr("to");
                IAvailBridge.Message memory message = IAvailBridge.Message(
                    0x02,
                    bytes32(bytes20(from)),
                    bytes32(bytes20(to)),
                    1,
                    2,
                    abi.encode(0x4554480000000000000000000000000000000000000000000000000000000000, amount),
                    messageId
                );
                bytes32 messageHash = keccak256(abi.encode(message));
                bytes32 dataRoot = keccak256(abi.encode(bytes32(0), messageHash));

                vectorx.set(rangeHash, dataRoot);

                bytes32[] memory emptyArr;
                IAvailBridge.MerkleProofInput memory input =
                IAvailBridge.MerkleProofInput(emptyArr, emptyArr, rangeHash, 0, bytes32(0), messageHash, messageHash, 0);

                uint256 balance = to.balance;
                bridge.receiveETH(message, input);
            }

            // Check balance and fee
            assertEq(address(bridge).balance, 0); // Balance is 0
            assertEq(bridge.fees(), 1_000); // Fee is 1_000

            // Withdraw fees (Should fail)
            vm.expectRevert(IAvailBridge.WithdrawFailed.selector);
            bridge.withdrawFees();

            // Check balance and fee again
            assertEq(address(bridge).balance, 0); // Balance is 0
            assertEq(bridge.fees(), 1_000); // Fee is 1000
        }
    }

The fee remains at 1,000, but it cannot be transferred due to the contract having a zero balance. This balance was transferred out by the receiveETH function.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[QEDK]

I think there is a small misconception as to how the bridge itself works, so let me break it down. All fees accrued is accounted for separately, which means that address(AvailBridge).balance - fees = ETH supply on Avail. What this also means is that msg.value sent via sendETH() is always minted on Avail and vice-versa, all ETH (except fees) sent from AvailBridge corresponds to the same amount of ETH burnt on Avail. The additional function call that you have put would not be possible because there would be no corresponding ETH burnt on Avail that emits the valid token bridging message.

[jingyi2811]

Yeah, makes sense.

[Evert0x]

Result: Invalid Unique