search

sherlock-audit / 2024-02-optimism-2024-judging

6 stars 4 forks source link

MiloTruck - `uint64` is too small to hold `indexAtDepth` for nodes below a depth of 64 #222

Closed sherlock-admin3 closed 7 months ago

sherlock-admin3 commented 7 months ago

MiloTruck

medium

uint64 is too small to hold indexAtDepth for nodes below a depth of 64

Summary

As uint64 is used to represent the index of nodes, calling step() with nodes below a depth of 64 may return an incorrect result.

Vulnerability Detail

LibPosition.indexAtDepth() returns indexAtDepth_ as a uint64:

    function indexAtDepth(Position _position) internal pure returns (uint64 indexAtDepth_) {
        // Return bits p_{msb-1}...p_{0}. This effectively pulls the 2^{depth} out of the gindex,
        // leaving only the `indexAtDepth`.
        uint256 msb = depth(_position);
        assembly {
            indexAtDepth_ := sub(_position, shl(msb, 1))
        }
    }

However, uint64 is too small to contain the index of nodes at a depth of 64 and below.

Impact

indexAtDepth() is used in step() to find the depth of nodes. As such, calling step() on nodes at a depth of 64 and below may result in an incorrect result.

Code Snippet

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/f216b0d3ad08c1a0ead557ea74691aaefd5fd489/optimism/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L177

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/f216b0d3ad08c1a0ead557ea74691aaefd5fd489/optimism/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L825

Tool used

Manual Review

Recommendation

In indexAtDepth(), return indexAtDepth_ as a uint128 instead.

smartcontracts commented 7 months ago

This is valid and we will fix it. It should be noted that this would not impact the system as currently specified as the configured step depth is less than 64 but we are intending to fix this to avoid a future footgun. Given the lack of immediate impact we would argue to classify as low/footgun.

nevillehuang commented 7 months ago

Agree with sponsors comment, as supported with additional details as below

The configured split depth on Sepolia is 30, and the max depth is 73. https://github.com/ethereum-optimism/optimism/blob/34ce96a09e88543c95c8dd6ec09d7e3531568b90/packages/contracts-bedrock/deploy-config/sepolia.json#L47-L51. It is possible for this to affect the dispute game with the current configuration theoretically, but the honest challenger would not travel that far right due to trace extension rules.

sherlock-admin4 commented 7 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/ethereum-optimism/optimism/pull/10150