`SequencerInbox::_setBufferConfig()` changes the parameters if not all delayed messages were read, leading to different past buffer delay

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Buffer will be modified with new parameters for periods where the old parameters were active, harming either the users (if it increases) or the sequencer (if it decreases and lowers it too much).

Proof of Concept

SequencerInbox::_setBufferConfig() modifies the parameters of the buffer, including buffer.bufferBlocks, buffer.max, buffer.threshold, buffer.replenishRateInBasis. If no delayed messages are present (the sequencer is synced), it updates the buffer up to block.number, but in this calculation uses the new parameters instead, leading to a different buffer. This alone should be performed before changing the parameters, as the previous buffer period had different parameters.

More impactul is when there are unread delayed messages. In this case, the parameters will be updated, and the past users that had already queued their messages will be subjected to sudden different buffer delays.

Add the following test as a POC to SequencerInbox.t.sol. Here a replenishRate change was tested, but it may also happen to a threshold or max change.

function test_POC_Buffer_modifies_delayed_messages() public {
    BufferConfig memory configBufferable = BufferConfig({
        threshold: 600, //60 * 60 * 2 / 12
        max: 14400, //24 * 60 * 60 / 12 * 2
        replenishRateInBasis: 714
    });

    (SequencerInbox seqInbox, Bridge bridge) = deployRollup(false, true, configBufferable);

    // Reduce delay blocks below the maximum to illustrate the issue
    vm.startPrank(rollupOwner);
    configBufferable.max = 7000;
    seqInbox.setBufferConfig(configBufferable);
    configBufferable.max = 14400;
    seqInbox.setBufferConfig(configBufferable);
    (uint64 bufferBlocks, ,,,,) = seqInbox.buffer();
    assertEq(bufferBlocks, 7000);
    vm.stopPrank();

    // Create a delayed message
    address delayedInboxSender = address(140);
    uint8 delayedInboxKind = 3;
    bytes32 messageDataHash = RAND.Bytes32();
    vm.prank(dummyInbox);
    bridge.enqueueDelayedMessage(delayedInboxKind, delayedInboxSender, messageDataHash);

    vm.roll(block.number + 500);

    // Include the first delayed message
    seqInbox.forceInclusion(
            bridge.delayedMessageCount(),
            delayedInboxKind,
            [uint64(block.number - 500), uint64(block.timestamp)],
            0,
            delayedInboxSender,
            messageDataHash
    );
    // Create another delayed message
    vm.prank(dummyInbox);
    bridge.enqueueDelayedMessage(delayedInboxKind, delayedInboxSender, messageDataHash);

    vm.roll(block.number + 100);

    // Change the replenish rate. This will apply retroactively,
    // reducing the buffer during the period in which replenishRateInBasis was 714
    vm.prank(rollupOwner);
    configBufferable.replenishRateInBasis = 300;
    seqInbox.setBufferConfig(configBufferable);

    // buffer does is not increased as much as expected
    seqInbox.forceInclusion(
            bridge.delayedMessageCount(),
            delayedInboxKind,
            [uint64(block.number - 100), uint64(block.timestamp)],
            0,
            delayedInboxSender,
            messageDataHash
    );
    (bufferBlocks, ,,,,) = seqInbox.buffer();
    assertEq(bufferBlocks, 7000 + 500*300/10_000); // replenish rate of 300, not 714
}

Tools Used

Vscode

Foundry

Recommended Mitigation Steps

Move the update logic to before the parameters are changed. Also, if the delayed messages are not synced, it should probably better to sync them first or similar.

Assessed type

Context

[c4-sponsor]

gzeoneth (sponsor) disputed

[gzeoneth]

Expected behavior, owner (or governance) should consider the risk when changing any parameters. Misconfigurations are out-of-scope.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid