code-423n4 / 2024-05-arbitrum-foundation-findings

1 stars 2 forks source link

`sequencerBatchAcc` has an incorrect value in `RollupCore` #30

Closed howlbot-integration[bot] closed 1 month ago

howlbot-integration[bot] commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/RollupCore.sol#L471

Vulnerability details

Impact

In the current implementation of the RollupCore contract, createNewAssertion() function is used to create new assertions. The new assertion should have several parameters including the hash of the previous assertion, the state after, and the sequencer inbox accumulator value. The problem is that currently when determining sequencerInboxAcc value, the function uses afterInboxPosition that shows how much messages should the next assertion process instead of currentInboxPosition that shows how much messages this assertion has process (what the value of sequencer accumulator should represent).

Proof of Concept

Let's say there are 200 messages that were added to the inbox so that currentInboxPosition is equal to 200 (at the time assertion was created):

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/RollupCore.sol#L436

  uint256 currentInboxPosition = bridge.sequencerMessageCount();

Then the function fetches the new messages that may have added to the inbox since the last assertion:

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/RollupCore.sol#L450

uint256 afterInboxPosition = afterGS.getInboxPosition();

The problem is that the sequencer should represent how many messages this assertion has processed and not the value that the next assertion should represent:

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/RollupCore.sol#L471

sequencerBatchAcc = bridge.sequencerInboxAccs(afterInboxPosition - 1);

Tools Used

Manual review.

Recommended Mitigation Steps

Change afterInboxPosition on currentInboxPosition when assigning the value of sequence accumulator.

Assessed type

Other

gzeoneth commented 1 month ago

Invalid, CURRENT assertion consume up to afterInboxPosition, NEXT assertion consume up to currentInboxPosition.

c4-judge commented 1 month ago

Picodes marked the issue as unsatisfactory: Invalid