Logical flaw in `_setBufferConfig` function that can lead to unexpected behavior and potentially incorrect state updates.

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

_setBufferConfig function src\bridge\SequencerInbox.sol:847-865

The following lines: src\bridge\SequencerInbox.sol:851-855

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

The problem is that these two conditions are not mutually exclusive, and the second condition can override the first condition's assignment.

For example, if buffer.bufferBlocks is initially 0, and bufferConfig_.max is greater than bufferConfig_.threshold, the first condition will set buffer.bufferBlocks to bufferConfig_.max. However, the second condition will then immediately set buffer.bufferBlocks to bufferConfig_.threshold, effectively overriding the previous assignment.

This behavior is likely unintended and could lead to unexpected results.

Proof of Concept

The purpose of the _setBufferConfig function is to update the buffer configuration based on the provided bufferConfig_ parameters. However, the two conditional statements that update the buffer.bufferBlocks value have overlapping conditions, which can result in unintended behavior.

Here's a breakdown of the issue:

The first condition buffer.bufferBlocks == 0 || buffer.bufferBlocks > bufferConfig_.max checks if the current buffer.bufferBlocks value is either zero or greater than the new bufferConfig_.max value. If this condition is true, it sets buffer.bufferBlocks to bufferConfig_.max.

The second condition buffer.bufferBlocks < bufferConfig_.threshold checks if the current buffer.bufferBlocks value is less than the new bufferConfig_.threshold value. If this condition is true, it sets buffer.bufferBlocks to bufferConfig_.threshold.

The problem arises when bufferConfig_.threshold is less than bufferConfig_.max. In this case, the second condition can override the assignment made by the first condition, leading to an incorrect state update.

For example, consider the following scenario:

Initial state: buffer.bufferBlocks = 0 New configuration: bufferConfig_.max = 100, bufferConfig_.threshold = 50 In this scenario, the first condition buffer.bufferBlocks == 0 || buffer.bufferBlocks > bufferConfig_.max will be true, and buffer.bufferBlocks will be set to bufferConfig_.max = 100.

However, the second condition buffer.bufferBlocks < bufferConfig_.threshold will also be true (since 100 < 50), and buffer.bufferBlocks will be overwritten with bufferConfig_.threshold = 50.

This behavior is likely unintended, as it violates the expected logic of setting buffer.bufferBlocks to the maximum value (bufferConfig_.max) if it is initially zero or greater than the maximum.

The impact of this issue can be significant, as it can lead to incorrect state updates and potentially cause the contract to behave unexpectedly in certain scenarios. This could have implications for the overall functionality and security of the contract, especially if the buffer.bufferBlocks value is used in other parts of the contract logic.

To illustrate the concept further, here's an example of how the code could be modified to address the issue:

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

By using an else if statement instead of separate if statements, the conditions are evaluated in the correct order, and the assignment of buffer.bufferBlocks will be consistent with the intended behavior.

Tools Used

Vs src\bridge\SequencerInbox.sol:847-865

Recommended Mitigation Steps

Combine the two conditions into a single if statement with appropriate logic:

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

This way, the conditions are evaluated in the correct order, and the assignment of buffer.bufferBlocks will be consistent with the intended behavior.

Assessed type

Error

[gzeoneth]

Invalid. buffer.bufferBlocks would be updated and the second condition will not be met (threshold ≤ max) We also check the config is valid - https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/bridge/DelayBuffer.sol#L120

[Picodes]

The report read the seconds condition incorrectly: the second condition buffer.bufferBlocks < bufferConfig_.threshold will also be true (since 100 < 50), whereas what the code do is correct: provided threshold < max which is enforced here you can't enter both conditions

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid