Dubious typecast in the following functions getTimeBounds & setValidKeyset & _setBufferConfig & submiteBatchSpendingReport & _setMaxTimeVariation & formCallDataHash & packHeader functions

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/bridge/SequencerInbox.sol#L216-L233 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/bridge/SequencerInbox.sol#L903-L919 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/bridge/SequencerInbox.sol#L847-L865 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/bridge/SequencerInbox.sol#L755-L789 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/bridge/SequencerInbox.sol#L867-L882 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/bridge/SequencerInbox.sol#L688-L718 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/bridge/SequencerInbox.sol#L637-L653

Vulnerability details

Impact

Detailed description of the impact of this finding.

Vulnerability: Dubious Typecasting in Various Functions

Description:

The vulnerability arises from improper typecasting in several functions, leading to potential data loss or unexpected behavior. Specifically, the conversion from larger data types to smaller ones occurs without proper validation.

Affected Functions and Locations:

1. getTimeBounds (SequencerInbox.sol lines 216-233):

   • Issue: Typecasting block.timestamp and block.number to uint64.
   • Impact: Potential overflow resulting in incorrect time bounds.
   • Recommendation: Use clear constants and validate values before typecasting.

2. setValidKeyset (SequencerInbox.sol lines 903-919):

   • Issue: Typecasting creationBlock to uint64.
   • Impact: Potential overflow leading to incorrect keyset information.
   • Recommendation: Use clear constants and validate values before typecasting.

3. _setBufferConfig (SequencerInbox.sol lines 847-865):

   • Issue: Typecasting block.number to uint64.
   • Impact: Potential overflow resulting in incorrect buffer configuration.
   • Recommendation: Use clear constants and validate values before typecasting.

4. submitBatchSpendingReport (SequencerInbox.sol lines 755-789):

   • Issue: Typecasting extraGas to uint64.
   • Impact: Potential overflow leading to incorrect gas values.
   • Recommendation: Use clear constants and validate values before typecasting.

5. _setMaxTimeVariation (SequencerInbox.sol lines 867-882):

   • Issue: Typecasting maxTimeVariation_ values to uint64.
   • Impact: Potential overflow resulting in incorrect time variation settings.
   • Recommendation: Use clear constants and validate values before typecasting.

6. formCallDataHash (SequencerInbox.sol lines 688-718):

   • Issue: Typecasting data to bytes32.
   • Impact: Potential data loss or unexpected behavior.
   • Recommendation: Use clear constants and validate values before typecasting.

7. packHeader (SequencerInbox.sol lines 637-653):

   • Issue: Typecasting afterDelayedMessagesRead to uint64.
   • Impact: Potential overflow resulting in incorrect header formation.
   • Recommendation: Use clear constants and validate values before typecasting.

General Recommendation:

To mitigate these vulnerabilities, implement validation checks before performing typecasting operations. Ensure that the values being cast do not exceed the maximum limits of the target data types. For example, verify that block.timestamp and block.number are within the allowable range before casting them to uint64. Similarly, validate the lengths of arrays and other data structures prior to typecasting.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "../src/SequencerInbox.sol";

contract SequencerInboxTest is Test {
    SequencerInbox sequencerInbox;
    IBridge bridge;
    IReader4844 reader4844;

    function setUp() public {
        bridge = IBridge(address(0x1234)); // Mock address
        reader4844 = IReader4844(address(0x5678)); // Mock address
        sequencerInbox = new SequencerInbox(128 * 1024, reader4844, false, false);
        sequencerInbox.initialize(bridge, ISequencerInbox.MaxTimeVariation(1, 1, 1, 1), BufferConfig(1, 1, 1));
    }

    function testGetTimeBoundsOverflow() public {
        // Set block.timestamp and block.number to values that will cause overflow when cast to uint64
        vm.warp(2**64); // Set block.timestamp to 2^64
        vm.roll(2**64); // Set block.number to 2^64

        // Call getTimeBounds and check for overflow
        IBridge.TimeBounds memory bounds = sequencerInbox.getTimeBounds();
        assertEq(bounds.minTimestamp, 0, "minTimestamp should overflow to 0");
        assertEq(bounds.minBlockNumber, 0, "minBlockNumber should overflow to 0");
    }

    function testSetValidKeysetOverflow() public {
        // Set block.number to a value that will cause overflow when cast to uint64
        vm.roll(2**64); // Set block.number to 2^64

        // Prepare a dummy keyset
        bytes memory keysetBytes = hex"1234";

        // Call setValidKeyset and check for overflow
        sequencerInbox.setValidKeyset(keysetBytes);
        bytes32 ksHash = keccak256(bytes.concat(hex"fe", keccak256(keysetBytes)));
        (bool isValid, uint64 creationBlock) = sequencerInbox.dasKeySetInfo(ksHash);
        assertEq(creationBlock, 0, "creationBlock should overflow to 0");
    }

    function testSetBufferConfigOverflow() public {
        // Set block.number to a value that will cause overflow when cast to uint64
        vm.roll(2**64); // Set block.number to 2^64

        // Prepare a dummy buffer config
        BufferConfig memory bufferConfig = BufferConfig(1, 1, 1);

        // Call setBufferConfig and check for overflow
        sequencerInbox.setBufferConfig(bufferConfig);
        (uint64 bufferBlocks, , , ) = sequencerInbox.buffer();
        assertEq(bufferBlocks, 0, "bufferBlocks should overflow to 0");
    }

    function testSubmitBatchSpendingReportOverflow() public {
        // Prepare dummy data
        bytes32 dataHash = keccak256("dummy data");
        uint256 seqMessageIndex = 1;
        uint256 gasPrice = 1;
        uint256 extraGas = 2**64; // Set extraGas to a value that will cause overflow when cast to uint64

        // Call submitBatchSpendingReport and check for overflow
        sequencerInbox.submitBatchSpendingReport(dataHash, seqMessageIndex, gasPrice, extraGas);
        // No direct way to check the overflow, but we can ensure it doesn't revert
    }

    function testSetMaxTimeVariationOverflow() public {
        // Prepare a dummy max time variation with values that will cause overflow when cast to uint64
        ISequencerInbox.MaxTimeVariation memory maxTimeVariation = ISequencerInbox.MaxTimeVariation(
            2**64, 2**64, 2**64, 2**64
        );

        // Call setMaxTimeVariation and check for overflow
        sequencerInbox.setMaxTimeVariation(maxTimeVariation);
        (uint64 delayBlocks, uint64 futureBlocks, uint64 delaySeconds, uint64 futureSeconds) = sequencerInbox.maxTimeVariationInternal();
        assertEq(delayBlocks, 0, "delayBlocks should overflow to 0");
        assertEq(futureBlocks, 0, "futureBlocks should overflow to 0");
        assertEq(delaySeconds, 0, "delaySeconds should overflow to 0");
        assertEq(futureSeconds, 0, "futureSeconds should overflow to 0");
    }

    function testFormCallDataHashOverflow() public {
        // Prepare dummy data
        bytes memory data = hex"1234";
        uint256 afterDelayedMessagesRead = 2**64; // Set afterDelayedMessagesRead to a value that will cause overflow when cast to uint64

        // Call formCallDataHash and check for overflow
        (bytes32 dataHash, ) = sequencerInbox.formCallDataHash(data, afterDelayedMessagesRead);
        // No direct way to check the overflow, but we can ensure it doesn't revert
    }

    function testPackHeaderOverflow() public {
        // Set afterDelayedMessagesRead to a value that will cause overflow when cast to uint64
        uint256 afterDelayedMessagesRead = 2**64;

        // Call packHeader and check for overflow
        (bytes memory header, ) = sequencerInbox.packHeader(afterDelayedMessagesRead);
        // No direct way to check the overflow, but we can ensure it doesn't revert
    }
}

Tools Used

Slither.

Recommended Mitigation Steps

Recommendation: Resolving Typecasting Issues

General Approach:

To mitigate vulnerabilities from improper typecasting, implement validation checks before typecasting operations. This ensures values do not exceed the maximum limits of the target data types. Below are specific recommendations for each affected function:

1. getTimeBounds:
   • Issue: Typecasting block.timestamp and block.number to uint64.
   • Resolution: Validate block.timestamp and block.number before casting.

function getTimeBounds() internal view virtual returns (IBridge.TimeBounds memory) {
    IBridge.TimeBounds memory bounds;
    uint256 currentTimestamp = block.timestamp;
    uint256 currentBlockNumber = block.number;

    require(currentTimestamp <= type(uint64).max, "Timestamp exceeds uint64 limit");
    require(currentBlockNumber <= type(uint64).max, "Block number exceeds uint64 limit");

    if (currentTimestamp > delaySeconds_) {
        bounds.minTimestamp = uint64(currentTimestamp - delaySeconds_);
    }
    bounds.maxTimestamp = uint64(currentTimestamp + futureSeconds_);
    if (currentBlockNumber > delayBlocks_) {
        bounds.minBlockNumber = uint64(currentBlockNumber - delayBlocks_);
    }
    bounds.maxBlockNumber = uint64(currentBlockNumber + futureBlocks_);
    return bounds;
}

2. setValidKeyset:
   • Issue: Typecasting creationBlock to uint64.
   • Resolution: Validate creationBlock before casting.

function setValidKeyset(bytes calldata keysetBytes) external onlyRollupOwner {
    uint256 ksWord = uint256(keccak256(bytes.concat(hex"fe", keccak256(keysetBytes))));
    bytes32 ksHash = bytes32(ksWord ^ (1 << 255));
    if (keysetBytes.length >= 64 * 1024) revert KeysetTooLarge();

    if (dasKeySetInfo[ksHash].isValidKeyset) revert AlreadyValidDASKeyset(ksHash);
    uint256 creationBlock = block.number;
    if (hostChainIsArbitrum) {
        creationBlock = ArbSys(address(100)).arbBlockNumber();
    }

    require(creationBlock <= type(uint64).max, "Creation block exceeds uint64 limit");

    dasKeySetInfo[ksHash] = DasKeySetInfo({
        isValidKeyset: true,
        creationBlock: uint64(creationBlock)
    });
    emit SetValidKeyset(ksHash, keysetBytes);
    emit OwnerFunctionCalled(2);
}

3. _setBufferConfig:
   • Issue: Typecasting block.number to uint64.
   • Resolution: Validate block.number before casting.

function _setBufferConfig(BufferConfig memory bufferConfig_) internal {
    if (!isDelayBufferable) revert NotDelayBufferable();
    if (!DelayBuffer.isValidBufferConfig(bufferConfig_)) revert BadBufferConfig();

    if (buffer.bufferBlocks == 0 || buffer.bufferBlocks > bufferConfig_.max) {
        buffer.bufferBlocks = bufferConfig_.max;
    }
    if (buffer.bufferBlocks < bufferConfig_.threshold) {
        buffer.bufferBlocks = bufferConfig_.threshold;
    }
    buffer.max = bufferConfig_.max;
    buffer.threshold = bufferConfig_.threshold;
    buffer.replenishRateInBasis = bufferConfig_.replenishRateInBasis;

    uint256 currentBlockNumber = block.number;
    require(currentBlockNumber <= type(uint64).max, "Block number exceeds uint64 limit");

    if (bridge.delayedMessageCount() == totalDelayedMessagesRead) {
        buffer.update(uint64(currentBlockNumber));
    }
}

4. submitBatchSpendingReport:
   • Issue: Typecasting extraGas to uint64.
   • Resolution: Validate extraGas before casting.

function submitBatchSpendingReport(
    bytes32 dataHash,
    uint256 seqMessageIndex,
    uint256 gasPrice,
    uint256 extraGas
) internal {
    address batchPoster = tx.origin;

    if (hostChainIsArbitrum) {
        uint256 l1Fees = ArbGasInfo(address(0x6c)).getCurrentTxL1GasFees();
        extraGas += l1Fees / block.basefee;
    }

    require(extraGas <= type(uint64).max, "Extra gas exceeds uint64 limit");

    bytes memory spendingReportMsg = abi.encodePacked(
        block.timestamp,
        batchPoster,
        dataHash,
        seqMessageIndex,
        gasPrice,
        uint64(extraGas)
    );

    uint256 msgNum = bridge.submitBatchSpendingReport(
        batchPoster,
        keccak256(spendingReportMsg)
    );

    emit InboxMessageDelivered(msgNum, spendingReportMsg);
}

5. _setMaxTimeVariation:
   • Issue: Typecasting maxTimeVariation_ values to uint64.
   • Resolution: Validate maxTimeVariation_ values before casting.

function _setMaxTimeVariation(ISequencerInbox.MaxTimeVariation memory maxTimeVariation_) internal {
    require(maxTimeVariation_.delayBlocks <= type(uint64).max, "delayBlocks exceeds uint64 limit");
    require(maxTimeVariation_.futureBlocks <= type(uint64).max, "futureBlocks exceeds uint64 limit");
    require(maxTimeVariation_.delaySeconds <= type(uint64).max, "delaySeconds exceeds uint64 limit");
    require(maxTimeVariation_.futureSeconds <= type(uint64).max, "futureSeconds exceeds uint64 limit");

    delayBlocks = uint64(maxTimeVariation_.delayBlocks);
    futureBlocks = uint64(maxTimeVariation_.futureBlocks);
    delaySeconds = uint64(maxTimeVariation_.delaySeconds);
    futureSeconds = uint64(maxTimeVariation_.futureSeconds);
}

6. formCallDataHash:
   • Issue: Typecasting data to bytes32.
   • Resolution: Validate data length before casting.

function formCallDataHash(bytes calldata data, uint256 afterDelayedMessagesRead)
    internal
    view
    returns (bytes32, IBridge.TimeBounds memory)
{
    uint256 fullDataLen = HEADER_LENGTH + data.length;
    if (fullDataLen > maxDataSize) revert DataTooLarge(fullDataLen, maxDataSize);

    (bytes memory header, IBridge.TimeBounds memory timeBounds) = packHeader(
        afterDelayedMessagesRead
    );

    if (data.length > 0) {
        if (!isValidCallDataFlag(data[0])) revert InvalidHeaderFlag(data[0]);

        if (data[0] & DAS_MESSAGE_HEADER_FLAG != 0 && data.length >= 33) {
            bytes32 dasKeysetHash = bytes32(data[1:33]);
            if (!dasKeySetInfo[dasKeysetHash].isValidKeyset) revert NoSuchKeyset(dasKeysetHash);
        }
    }

    return (keccak256(bytes.concat(header, data)), timeBounds);
}

7. packHeader:
   • Issue: Typecasting afterDelayedMessagesRead to uint64.
   • Resolution: Validate afterDelayedMessagesRead before casting.

function packHeader(uint256 afterDelayedMessagesRead)
    internal
    view
    returns (bytes memory, IBridge.TimeBounds memory)
{
    IBridge.TimeBounds memory timeBounds = getTimeBounds();

    require(afterDelayedMessagesRead <= type(uint64).max, "afterDelayedMessagesRead exceeds uint64 limit");

    bytes memory header = abi.encodePacked(
        timeBounds.minTimestamp,
        timeBounds.maxTimestamp,
        timeBounds.minBlockNumber,
        timeBounds.maxBlockNumber,
        uint64(afterDelayedMessagesRead)
    );

    assert(header.length == HEADER_LENGTH);

    return (header, timeBounds);
}

General Recommendation:

Implement validation checks before performing typecasting operations to ensure values do not exceed the maximum limits of the target data types. This will prevent potential data loss or unexpected behavior due to truncation or data loss.

Assessed type

Context