c4-bot-3 commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/cannon/PreimageOracle.sol#L601-L605 https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/cannon/PreimageOracle.sol#L633-L636

Vulnerability details

Impact

In PreimageOracle, successful challengers of LPP might lose bond rewards due to missing checks.

Proof of Concept

A large preimage proposal can be challenged through challengeLPP() and challengeFirstLPP().

The issue is challengeLPP() andchallengeFirstLPP()` don't have checks on the proposal counter status, allowing an LPP to be countered multiple times.

For example, in challengeLPP(), when the challenger's proof passes validation, the proposalMetadata[_claimant][_uuid] will be set as countered. However, challengeLPP() and challengeFirstLPP() don't check whether the proposal has already been countered at the time of the tx settlement.

//packages/contracts-bedrock/src/cannon/PreimageOracle.sol
    function challengeLPP(
        address _claimant,
        uint256 _uuid,
        LibKeccak.StateMatrix memory _stateMatrix,
        Leaf calldata _preState,
        bytes32[] calldata _preStateProof,
        Leaf calldata _postState,
        bytes32[] calldata _postStateProof
    )
        external
    {
        // Verify that both leaves are present in the merkle tree.
        bytes32 root = getTreeRootLPP(_claimant, _uuid);
        if (
            !(
                _verify(_preStateProof, root, _preState.index, _hashLeaf(_preState))
                    && _verify(_postStateProof, root, _postState.index, _hashLeaf(_postState))
            )
        ) revert InvalidProof();

        // Verify that the prestate passed matches the intermediate state claimed in the leaf.
        if (keccak256(abi.encode(_stateMatrix)) != _preState.stateCommitment) revert InvalidPreimage();

        // Verify that the pre/post state are contiguous.
        if (_preState.index + 1 != _postState.index) revert StatesNotContiguous();

        // Absorb and permute the input bytes.
        LibKeccak.absorb(_stateMatrix, _postState.input);
        LibKeccak.permutation(_stateMatrix);

        // Verify that the post state hash doesn't match the expected hash.
        if (keccak256(abi.encode(_stateMatrix)) == _postState.stateCommitment) revert PostStateMatches();

        // Mark the keccak claim as countered.
|>      proposalMetadata[_claimant][_uuid] = proposalMetadata[_claimant][_uuid].setCountered(true);

        // Pay out the bond to the challenger.
|>      _payoutBond(_claimant, _uuid, msg.sender);
    }

  function _payoutBond(address _claimant, uint256 _uuid, address _to) internal {
    // Pay out the bond to the claimant.
    uint256 bond = proposalBonds[_claimant][_uuid];
    proposalBonds[_claimant][_uuid] = 0;
    (bool success, ) = _to.call{value: bond}('');
    if (!success) revert BondTransferFailed();
  }

(https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/cannon/PreimageOracle.sol#L601-L605)

Suppose two challengers A and B both send correct challengeLPP() tx.

Challenger A's tx settles before B. A get the bonds.
Challenger B's tx settles after A. B's tx will go through. _payoutBond() will send 0 value to B.

B successfully counters LPP and sets counter states but receives zero bonds.

Tools Used

Manual

Recommended Mitigation Steps

In challengeLPP() and challengeFirstLPP(), add check to ensure the LPP' hasn't been countered before performing validation logic.

Assessed type

Other

c4-judge commented 1 month ago

zobront changed the severity to QA (Quality Assurance)