verification of bridge leaf will always fail due to unhashed leaf input argument

Summary

Vulnerability Detail

AvailBridge.verifyBridgeLeaf() ia used to verify the bridge leaf which takes a Merkle tree proof of inclusion for a bridge leaf and verifies it. The issue here is, it takes a input.leaf as one of argument which is passed as without keccak256 hashed. The Natspec specifically says to pass the leaf as hash of keccak.

440        // leaf must be keccak(message)

and verifyBridgeLeaf() is given as below,

    function verifyBridgeLeaf(MerkleProofInput calldata input) public view returns (bool) {
        if (input.bridgeRoot == 0x0) {
            revert BridgeRootEmpty();
        }
        _checkDataRoot(input);
        // leaf must be keccak(message)                 @audit // comment is not incorporated 
        // we don't need to check that the leaf is non-zero because we check that the root is non-zero
        return input.leafProof.verify(input.bridgeRoot, input.leafIndex, input.leaf);          @audit // input leaf argument is not hashed
    }

While verifying the bridge leaf, the verification will always fail as input.leaf is passed as bytes32 argument instead of keccak256(abi.encode(input.leaf).

Let's consider a example in remix, consider a input leaf in bytes32 argument, input.leaf = 0x111122223333444455556666777788889999AAAABBBBCCCCDDDDEEEEFFFFCCCC;

If this bytes32 argument is hashed via keccak(abi.encode(input.leaf) then the result would be 0x276b28b3bf3c9bd84af7f4ce06088d3c0df1ff732522ed263e466b1a63504ad6

This is completely different and the same can be applied while verification of bridge leaf and it will always fail due to wrong input argument.

verifyBridgeLeaf() is used in private function _checkBridgeLeaf()

    function _checkBridgeLeaf(Message calldata message, MerkleProofInput calldata input) private {
        bytes32 leaf = keccak256(abi.encode(message));
        if (isBridged[leaf]) {
            revert AlreadyBridged();
        }
        // validate that the leaf being proved is indeed the message hash!
        if (input.leaf != leaf) {
            revert InvalidLeaf();
        }
        // check proof of inclusion
        if (!verifyBridgeLeaf(input)) {             @audit // will always revert as bridge leaf verification will always fail.
            revert InvalidMerkleProof();
        }
        // mark as spent
        isBridged[leaf] = true;
    }

_checkBridgeLeaf() is further used in following funcions,

    function receiveMessage(Message calldata message, MerkleProofInput calldata input)

   . . . some code

        _checkBridgeLeaf(message, input);         @audit // bridge verification will fail due to issue in internal used functions

   . . . some code
}

    function receiveAVAIL(Message calldata message, MerkleProofInput calldata input)

   . . . some code

        _checkBridgeLeaf(message, input);

   . . . some code
}

    function receiveETH(Message calldata message, MerkleProofInput calldata input)

   . . . some code

        _checkBridgeLeaf(message, input);
   . . . some code
}

    function receiveERC20(Message calldata message, MerkleProofInput calldata input)

   . . . some code

        _checkBridgeLeaf(message, input);

   . . . some code
}

All these functions would revert during the verification of bridge leaf. The major contract functionality would be bricked.

Impact

Failure of bridge leaf verification would lead overall brick of contract functionality and the above functions would revert due to error while passing the input.leaf argument during bridge leaf verification. In addition, it does not incorporate the natspec design consideration while passing input.leaf argument.

Code Snippet

https://github.com/sherlock-audit/2023-12-avail/blob/1afb56b8d4dfbf5d3f21bed0ddf80a04730204b5/contracts/src/AvailBridge.sol#L442

Tool used

Manual Review

Recommendation

Consider below change to mitigate the issue. This is also inline with Natspec design for leaf argument for bridge leaf verification.

    function verifyBridgeLeaf(MerkleProofInput calldata input) public view returns (bool) {
        if (input.bridgeRoot == 0x0) {
            revert BridgeRootEmpty();
        }
        _checkDataRoot(input);
        // leaf must be keccak(message)
        // we don't need to check that the leaf is non-zero because we check that the root is non-zero
-        return input.leafProof.verify(input.bridgeRoot, input.leafIndex, input.leaf);
+        return input.leafProof.verify(input.bridgeRoot, input.leafIndex, keccak256(abi.encode(input.leaf)));
    }

The code is already designed to work as intended, the hashed leaf should be provided for Merkle proof verification. // leaf must be keccak(message) means that the leaf being provided into the input.leaf param should already be hashed.

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

takarez commented:

invalid because {invalid: it means that it must be KECCAKed during the function call not return}

Hi @QEDK,

// leaf must be keccak(message) means that the leaf being provided into the input.leaf param should already be hashed.

With due respect, i don't agree with this comment. input.leaf has to be keccak hashed after passing the argument while calling the verifyBridgeLeaf().

I would like to highlight similar functionality where input.leaf is hashed within function and not passed as hashed argument, see verifyBlobLeaf()


    function verifyBlobLeaf(MerkleProofInput calldata input) external view returns (bool) {
        if (input.blobRoot == 0x0) {
            revert BlobRootEmpty();
        }
        _checkDataRoot(input);
        // leaf must be keccak(blob)
        // we don't need to check that the leaf is non-zero because we hash the pre-image here
        return input.leafProof.verify(input.blobRoot, input.leafIndex, keccak256(abi.encode(input.leaf)));   @audit // input.leaf is keccak hash here, however it is missed in verifyBridgeLeaf()
    }

It can be seen input.leaf is keccak hashed while calling verifyBlobLeaf(). In addition, similar design natspec states,

        // leaf must be keccak(blob)

The recommendation in issue says to keccak hash the input.leaf similar to as done in verifyBlobLeaf. Therefore, verifyBridgeLeaf() should be corrected as below,

    function verifyBridgeLeaf(MerkleProofInput calldata input) public view returns (bool) {
        if (input.bridgeRoot == 0x0) {
            revert BridgeRootEmpty();
        }
        _checkDataRoot(input);
        // leaf must be keccak(message)
        // we don't need to check that the leaf is non-zero because we check that the root is non-zero
-        return input.leafProof.verify(input.bridgeRoot, input.leafIndex, input.leaf);
+        return input.leafProof.verify(input.bridgeRoot, input.leafIndex, keccak256(abi.encode(input.leaf)));
    }

Correct me, if i am missing something.

Hi @QEDK,

// leaf must be keccak(message) means that the leaf being provided into the input.leaf param should already be hashed.

With due respect, i don't agree with this comment. input.leaf has to be keccak hashed after passing the argument while calling the verifyBridgeLeaf().

I would like to highlight similar functionality where input.leaf is hashed within function and not passed as hashed argument, see verifyBlobLeaf()
    function verifyBlobLeaf(MerkleProofInput calldata input) external view returns (bool) {
        if (input.blobRoot == 0x0) {
            revert BlobRootEmpty();
        }
        _checkDataRoot(input);
        // leaf must be keccak(blob)
        // we don't need to check that the leaf is non-zero because we hash the pre-image here
        return input.leafProof.verify(input.blobRoot, input.leafIndex, keccak256(abi.encode(input.leaf)));   @audit // input.leaf is keccak hash here, however it is missed in verifyBridgeLeaf()
    }
It can be seen input.leaf is keccak hashed while calling verifyBlobLeaf(). In addition, similar design natspec states,
        // leaf must be keccak(blob)
The recommendation in issue says to keccak hash the input.leaf similar to as done in verifyBlobLeaf. Therefore, verifyBridgeLeaf() should be corrected as below,
    function verifyBridgeLeaf(MerkleProofInput calldata input) public view returns (bool) {
        if (input.bridgeRoot == 0x0) {
            revert BridgeRootEmpty();
        }
        _checkDataRoot(input);
        // leaf must be keccak(message)
        // we don't need to check that the leaf is non-zero because we check that the root is non-zero
-        return input.leafProof.verify(input.bridgeRoot, input.leafIndex, input.leaf);
+        return input.leafProof.verify(input.bridgeRoot, input.leafIndex, keccak256(abi.encode(input.leaf)));
    }
Correct me, if i am missing something.

That is because blob leaves are double-hashed, but bridge leaves are single-hashed because they require a reveal on the contract, so both inputs have to be hashed once, making the comment and implementation correct, and for the blobs, we re-hash it per the implementation detail. I recommend going through the supplementary bridge document that was shared.

sherlock-audit / 2023-12-avail-judging

0xMR0 - verification of bridge leaf will always fail due to unhashed leaf input argument #130

verification of bridge leaf will always fail due to unhashed leaf input argument

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation