ak1 - Unsafe block variables handling.

[github-actions[bot]]

ak1

high

Unsafe block variables handling.

Summary

Through-out the contracts the block variables are treated as given below.

BlockNumber - uint256, uint128 and uint64

timestamp - uint256, uint128 and uint64

when we see the output root proposal, the block varibales are treated as uint128.

 * @custom:field outputRoot    Hash of the L2 output.
 * @custom:field timestamp     Timestamp of the L1 block that the output root was submitted in.
 * @custom:field l2BlockNumber L2 block number that the output corresponds to.
 */
struct OutputProposal {
    bytes32 outputRoot;
    uint128 timestamp;
    uint128 l2BlockNumber;
}

whereas while feeding the L1 datas, the block varibles are treated as uint64.

function setL1BlockValues(
    uint64 _number,
    uint64 _timestamp,
    uint256 _basefee,
    bytes32 _hash,
    uint64 _sequenceNumber,
    bytes32 _batcherHash,
    uint256 _l1FeeOverhead,
    uint256 _l1FeeScalar
) external {

In some places the variables are compared with different range value. one example is from OptimismPortal.sol

inside the function proveWithdrawalTransaction,

    require(
        provenWithdrawal.timestamp == 0 ||
            (_l2OutputIndex == provenWithdrawal.l2OutputIndex &&
                outputRoot != provenWithdrawal.outputRoot),
        "OptimismPortal: withdrawal hash has already been proven"
    );

_l2OutputIndex is uint256 but provenWithdrawal.l2OutputIndex is uint128

Theoretically speaking, the possibility of the reaching the value to uint128 or uint64 is infeasible.

but technically it is possible.

The optimism is re-organize itself when the Ethereum reorganized.

but what if Ethereum foundation decides to fork the chain which could start the block number from uint64 or uint128 value.

In this case, optimism can not cope itself due to these kind of data handling.

Vulnerability Detail

Refer the summary section.

Impact

The impact could be,

1. Optimism can not handle the reorganization when Ethereum does which start block number value starts from uint64 or uint128 max value.
2. When Ethereum's block number reaches more max value of uint128 or uint64.

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/L2OutputOracle.sol#L60-L65

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L2/L1Block.sol#L79-L88

Tool used

Manual Review

Recommendation

we suggest to use the uint256 type of data for all the block varibles.

[rcstanciu]

Comment from Optimism

---

Description: Unsafe block variables handling.

Reason: This is a non-issue. 64 bits (minimum) for the L1/L2 timestamp and l2BlockNumber is sufficient for longer than our galaxy's sun will be alive. In addition, because it is very unfeasible that these values will ever surpass 64 bits in the lifespan of the Ethereum network, type casting timestamp and block number values between uint64, uint128, and uint256 is always considered safe. As for the comment about the community deciding to increase the number to a value that surpasses 64 bits in a future hardfork, it is safe to assume that we will know far in advance and will have the opportunity to upgrade our contracts before the hardfork goes live.

Action: Although this is a non-issue, for consistency in our codebase, we may want to consider moving towards a unified custom type for Timestamp and BlockNumber variables (as well as other dedicated types for like variables.)

[aktech297]

Escalate for 50 USDC.

I gently like to put couple of points on why I though it could be valid one.

First of all , the finding tries to explain what would happen with current code base when Ethereum is forked which starts with block variables values that starts from the range that is used by the optimism contract.

Regarding, it is safe to assume that we will know far in advance and will have the opportunity to upgrade our contracts before the hardfork goes live - Team assumes that it would know everything in advance. But this is not always the case when it comes for security. Anything can happen in short span of time. Disaster can happen in short span of time. As far as I understand that that hack will not come with invitation. Again there are a lot to discuss about it. When the team anticipate that one of the block reorganization will not give any sort of work towards the contract to the team. But the issue that I mentioned will give more amount of work and one more round of audit process. This is again the cost consuming process to the team.

The comment says Although this is a non-issue, for consistency in our codebase, we may want to consider moving towards a unified custom type for Timestamp and BlockNumber variables (as well as other dedicated types for like variables.) Team says that i would want to change the contract by looking at the consistency. As a developer how I would see the code consistency is to avoid any issue that may occur due to error in development or missing some piece of codes in few places which would cause issue that can be found later in time.

Also, it is not sure whether the team will aware of this if the report is not there. Also, block variables are core part of the contracts. any mistake that happens can cause huge impact towards the protocol. By considering all above points, it is fair to treat this as valid one.

[sherlock-admin]

Escalate for 50 USDC.

I gently like to put couple of points on why I though it could be valid one.

First of all , the finding tries to explain what would happen with current code base when Ethereum is forked which starts with block variables values that starts from the range that is used by the optimism contract.

Regarding, it is safe to assume that we will know far in advance and will have the opportunity to upgrade our contracts before the hardfork goes live - Team assumes that it would know everything in advance. But this is not always the case when it comes for security. Anything can happen in short span of time. Disaster can happen in short span of time. As far as I understand that that hack will not come with invitation. Again there are a lot to discuss about it. When the team anticipate that one of the block reorganization will not give any sort of work towards the contract to the team. But the issue that I mentioned will give more amount of work and one more round of audit process. This is again the cost consuming process to the team.

The comment says Although this is a non-issue, for consistency in our codebase, we may want to consider moving towards a unified custom type for Timestamp and BlockNumber variables (as well as other dedicated types for like variables.) Team says that i would want to change the contract by looking at the consistency. As a developer how I would see the code consistency is to avoid any issue that may occur due to error in development or missing some piece of codes in few places which would cause issue that can be found later in time.

Also, it is not sure whether the team will aware of this if the report is not there. Also, block variables are core part of the contracts. any mistake that happens can cause huge impact towards the protocol. By considering all above points, it is fair to treat this as valid one.

You've created a valid escalation for 50 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

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

[Evert0x]

Escalation rejected, comment provides insufficient arguments to label as medium or high.

[sherlock-admin]

Escalation rejected, comment provides insufficient arguments to label as medium or high.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.