A malicious user can misconfigure the buffer.

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/bridge/SequencerInbox.sol#L310

Vulnerability details

The buffer it is a way of limiting repeated sequencer censorship of delayed inbox messages. (took it directly by @godzillaba from discord chat arbitrum code4arena)

This buffer is been update in forceInclusion function (see the arrow below):

 function forceInclusion(
        uint256 _totalDelayedMessagesRead,
        uint8 kind,
        uint64[2] calldata l1BlockAndTime,
        uint256 baseFeeL1,
        address sender,
        bytes32 messageDataHash
    ) external {
        if (_totalDelayedMessagesRead <= totalDelayedMessagesRead) revert DelayedBackwards();
        bytes32 messageHash = Messages.messageHash(
            kind,
            sender,
            l1BlockAndTime[0],
            l1BlockAndTime[1],
            _totalDelayedMessagesRead - 1,
            baseFeeL1,
            messageDataHash
        );

        uint256 delayBlocks_ = delayBlocks;

        if (isDelayBufferable) {
            // proactively apply any pending delay buffer updates before the force included message l1BlockAndTime
            buffer.update(l1BlockAndTime[0]);  <-------
            delayBlocks_ = delayBufferableBlocks(buffer.bufferBlocks);
        }
...
}

[Link]

function update(BufferData storage self, uint64 blockNumber) internal {
        self.bufferBlocks = calcPendingBuffer(self, blockNumber);

        // store a new starting reference point
        // any buffer updates will be applied retroactively in the next batch post
        self.prevBlockNumber = blockNumber;  <------
        self.prevSequencedBlockNumber = uint64(block.number);
    }

[Link]

The problem is that the l1BlockAndTime can be passed with a l1BlockAndTime[0] arbitrary value so low and the checks are passing. this successfully set the self.prevBlockNumber see the arrow above in the update function to a incorrect value.

Impact

A malicious user can misconfigure the buffer leading to error in calcPendingBuffer and also the isSynced function return value can be manipulated to make the contract believe that isSynced but it not.

Proof of Concept

We can see that forceInclusion functions is calling update in the delay buffer library which is setting self.prevBlockNumber = blockNumber.

 function forceInclusion(
        uint256 _totalDelayedMessagesRead,
        uint8 kind,
        uint64[2] calldata l1BlockAndTime,
        uint256 baseFeeL1,
        address sender,
        bytes32 messageDataHash
    ) external {
        if (_totalDelayedMessagesRead <= totalDelayedMessagesRead) revert DelayedBackwards();
        bytes32 messageHash = Messages.messageHash(
            kind,
            sender,
            l1BlockAndTime[0],
            l1BlockAndTime[1],
            _totalDelayedMessagesRead - 1,
            baseFeeL1,
            messageDataHash
        );

        uint256 delayBlocks_ = delayBlocks;

        if (isDelayBufferable) {
            // proactively apply any pending delay buffer updates before the force included message l1BlockAndTime
            buffer.update(l1BlockAndTime[0]);  <-------
            delayBlocks_ = delayBufferableBlocks(buffer.bufferBlocks);
        }
...
}

[Link]

function update(BufferData storage self, uint64 blockNumber) internal {
        self.bufferBlocks = calcPendingBuffer(self, blockNumber);

        // store a new starting reference point
        // any buffer updates will be applied retroactively in the next batch post
        self.prevBlockNumber = blockNumber;  <------
        self.prevSequencedBlockNumber = uint64(block.number);
    }

[Link]

The calcPendingBuffer functions is calling

function calcPendingBuffer(BufferData storage self, uint64 blockNumber)
        internal
        view
        returns (uint64)
    {
        // bufferUpdate will not overflow since inputs are uint64
        return
            uint64(
                calcBuffer({
                    start: self.prevBlockNumber,
                    end: blockNumber,
                    buffer: self.bufferBlocks,
                    threshold: self.threshold,
                    sequenced: self.prevSequencedBlockNumber,
                    max: self.max,
                    replenishRateInBasis: self.replenishRateInBasis
                })

[Link]

see the calcBuffer function:

 function calcBuffer(
        uint256 start,
        uint256 end,
        uint256 buffer,
        uint256 sequenced,
        uint256 threshold,
        uint256 max,
        uint256 replenishRateInBasis
    ) internal pure returns (uint256) {
        uint256 elapsed = end > start ? end - start : 0;
        uint256 delay = sequenced > start ? sequenced - start : 0;
        // replenishment rounds down and will not overflow since all inputs including
        // replenishRateInBasis are cast from uint64 in calcPendingBuffer
        buffer += (elapsed * replenishRateInBasis) / BASIS;

        uint256 unexpectedDelay = delay > threshold ? delay - threshold : 0;
        if (unexpectedDelay > elapsed) {
            unexpectedDelay = elapsed;
        }

        // decrease the buffer
        if (buffer > unexpectedDelay) {
            buffer -= unexpectedDelay;
            if (buffer > threshold) {
                // saturating above at the max
                return buffer > max ? max : buffer;
            }
        }
        // saturating below at the threshold
        return threshold;
    }

[Link]

As you can see this function is not reverting is the l1BlockAndTime[0] which is the end input is not less than the start instead the function is just setting the elapse to zero the problem is that this l1BlockAndTime[0] was set to the prevBlockNumber in the update function:

function update(BufferData storage self, uint64 blockNumber) internal {
        self.bufferBlocks = calcPendingBuffer(self, blockNumber);

        // store a new starting reference point
        // any buffer updates will be applied retroactively in the next batch post
        self.prevBlockNumber = blockNumber;  <------
        self.prevSequencedBlockNumber = uint64(block.number);
    }

[Link]

Tools Used

Manual

Recommended Mitigation Steps

Since the end can not be before to the start a revert can be add it in calcBuffer see the arrow above

 function calcBuffer(
        uint256 start,
        uint256 end,
        uint256 buffer,
        uint256 sequenced,
        uint256 threshold,
        uint256 max,
        uint256 replenishRateInBasis
    ) internal pure returns (uint256) {
if (end < start) {revert customError():} <-----
        uint256 elapsed = end > start ? end - start : 0; //@audit (medium 2) see what happend when this end is less than the start apartently the function does not revert
        uint256 delay = sequenced > start ? sequenced - start : 0;
        // replenishment rounds down and will not overflow since all inputs including
        // replenishRateInBasis are cast from uint64 in calcPendingBuffer
        buffer += (elapsed * replenishRateInBasis) / BASIS;//@audit (medium 2) can this round to zero?

        uint256 unexpectedDelay = delay > threshold ? delay - threshold : 0;
        if (unexpectedDelay > elapsed) {
            unexpectedDelay = elapsed;
        }

        // decrease the buffer
        if (buffer > unexpectedDelay) {
            buffer -= unexpectedDelay;
            if (buffer > threshold) {
                // saturating above at the max
                return buffer > max ? max : buffer;
            }
        }
        // saturating below at the threshold
        return threshold; 
    }

Assessed type

Other

[jorgect207]

hey @Picodes thanks so much for judge this, this is not been invalidated in the validation repo but its not in the findings repo, thank for take a look of this issue. the problem here is that there is not validation for l1BlockAndTime[0] allowing a malicious attacker misconfig the buffer.

[gzeoneth]

It need to match messageHash and block/timestamp in the delayed inbox is strictly increasing.

[Picodes]

@jorgect207 the validation is done with messageHash