code-423n4 / 2024-03-zksync-findings

1 stars 1 forks source link

Denial of Service when PathLength of Merkle Root is Within a Valid Value #14

Closed c4-bot-10 closed 4 months ago

c4-bot-10 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/main/code/contracts/ethereum/contracts/state-transition/libraries/Merkle.sol#L25

Vulnerability details

Impact

Denial of Service when Path Length of Merkle Root is exactly 256 which should be a valid length value but would revert due to wrong implementation in Merkle Root Calculation

Proof of Concept

 function calculateRoot(
        bytes32[] calldata _path,
        uint256 _index,
        bytes32 _itemHash
    ) internal pure returns (bytes32) {
        uint256 pathLength = _path.length;
        require(pathLength > 0, "xc");
>>>        require(pathLength < 256, "bt");
        require(_index < (1 << pathLength), "px");

        bytes32 currentHash = _itemHash;
        for (uint256 i; i < pathLength; i = i.uncheckedInc()) {
            currentHash = (_index % 2 == 0)
                ? _efficientHash(currentHash, _path[i])
                : _efficientHash(_path[i], currentHash);
            _index /= 2;
        }

        return currentHash;
    }

The code above shows how Merkle Root is calculated in the Merkle contract, however the code reverts when Path length is 256, 256 is still within the allowed size, reverting it would cause Denial of Service in the Merkle Contract

Tools Used

Manual Review

Recommended Mitigation Steps

As corrected below path length of 256 should also be allowed without reversion

 function calculateRoot(
        bytes32[] calldata _path,
        uint256 _index,
        bytes32 _itemHash
    ) internal pure returns (bytes32) {
        uint256 pathLength = _path.length;
        require(pathLength > 0, "xc");
---        require(pathLength < 256, "bt");
+++        require(pathLength <= 256, "bt");
        require(_index < (1 << pathLength), "px");

        bytes32 currentHash = _itemHash;
        for (uint256 i; i < pathLength; i = i.uncheckedInc()) {
            currentHash = (_index % 2 == 0)
                ? _efficientHash(currentHash, _path[i])
                : _efficientHash(_path[i], currentHash);
            _index /= 2;
        }

        return currentHash;
    }

Assessed type

DoS

c4-sponsor commented 5 months ago

saxenism (sponsor) disputed

saxenism commented 5 months ago

The allowed max path length is in fact 255, so the implicit assumption of the warden is inaccurate.

Not an issue.

alex-ppg commented 4 months ago

The Warden specifies that a Merkle root path of 256 elements should be permitted, however, there is no evidence that denotes this to be the case. As there is no further evidence to substantiate the claim that 256 elements should be permitted, and the Sponsor claims that 255 is the correct limitation, I am inclined to consider this exhibit as invalid.

c4-judge commented 4 months ago

alex-ppg marked the issue as unsatisfactory: Invalid