Unsafe casting of L2 Output Index in proveWithdrawalTransaction function OptimismPortal.sol

[avishkarabhishek786]

Describe the bug The OptimismPortal.proveWithdrawalTransaction() function has a parameter _l2OutputIndex which is downcasted from uint256 to uint128. This may result in recording invalid _l2OutputIndex if original _l2OutputIndex is out of range of uint128.

provenWithdrawals[withdrawalHash] = ProvenWithdrawal({
            outputRoot: outputRoot,
            timestamp: uint128(block.timestamp),
            l2OutputIndex: uint128(_l2OutputIndex)
  });

proveWithdrawalTransaction

Configuration

• Severity: Medium

Impact Downcasting from uint256 to uint128 may result in loss of significant digits

Recommendation In the parameter we can start with _l2OutputIndex as uint128

function proveWithdrawalTransaction(
        Types.WithdrawalTransaction memory _tx,
        uint128 _l2OutputIndex,
        Types.OutputRootProof calldata _outputRootProof,
        bytes[] calldata _withdrawalProof
    )

Then for l2Oracle.getL2Output() function we could typecast it to unint256 which would not result in error.

bytes32 outputRoot = l2Oracle.getL2Output(uint256(_l2OutputIndex)).outputRoot;

And record the l2OutputIndex as uint128

provenWithdrawals[withdrawalHash] = ProvenWithdrawal({
            outputRoot: outputRoot,
            timestamp: uint128(block.timestamp),
            l2OutputIndex: _l2OutputIndex
 });

This is assuming it is essential to record l2OutputIndex in ProvenWithdrawal as uint128.

[nguyenzung]

Thank you @avishkarabhishek786

Thanos's proveWithdrawalTransaction is the same Optimism's proveWithdrawalTransaction. In theory, it is possible for lossing of significant digits, however in actual, it is impossible because uint128 is still a very big number

[theo-learner]

@avishkarabhishek786 Could you check and closing this issue?

[avishkarabhishek786]

Sure @boohyung