howlbot-integration[bot] commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/rollup/RollupCore.sol#L565-L574

Vulnerability details

Impact

Extending the delay of withdrawals

Proof of Concept

The maximum delay for withdrawals from L2 to L1 can be up to approximately 1 week (specifically 6.4 days), primarily due to the challenge period inherent in the optimistic rollup design used by Arbitrum. This challenge period is a security mechanism that allows validators to dispute state assertions before they are finalized on the L1 Ethereum blockchain.

The reason for this delay is rooted in the dispute resolution process. When a state assertion is made on the L2 chain, there is a fixed period during which it can be challenged. This is to ensure that only valid state transitions are confirmed on the L1 chain. If a dispute arises, it must be resolved within this period, and only after the period expires without disputes, or after a dispute is resolved in favor of the assertion, can the withdrawal be finalized on L1.

But this could be somehow bricked with an aditional period of time for specific conditions.

Validators can call newStakeOnNewAssertion and their funds will be taken on stake while the new assertion created in RollUpCore;

The function is as below:

Contract: RollupUserLogic.sol

363:     function newStakeOnNewAssertion( 
364:         uint256 tokenAmount,
365:         AssertionInputs calldata assertion,
366:         bytes32 expectedAssertionHash,
367:         address withdrawalAddress
368:     ) public {
369:         require(withdrawalAddress != address(0), "EMPTY_WITHDRAWAL_ADDRESS");
370:         _newStake(tokenAmount, withdrawalAddress);
371:         stakeOnNewAssertion(assertion, expectedAssertionHash);
372:         /// @dev This is an external call, safe because it's at the end of the function
373:         receiveTokens(tokenAmount); 
374:     }

L:371 calls stakeOnNewAssertion in the same contract.

Contract: RollupUserLogic.sol

173:     function stakeOnNewAssertion(AssertionInputs calldata assertion, bytes32 expectedAssertionHash)

And when the stakers want to withdraw their stakes, the first function to call is as follows:

Contract: RollupUserLogic.sol

240:     /**
241:      * @notice Refund a staker that is currently staked on an assertion that either has a chlid assertion or is the latest confirmed assertion.
242:      */
243:     function returnOldDeposit() external override onlyValidator whenNotPaused {
244:         requireInactiveStaker(msg.sender);
245:         withdrawStaker(msg.sender);
246:     }

So the staker should be inactive - validated by requireInactiveStaker And the funds should be made withdrawable by withdrawStaker

The logic of the requireInactiveStaker is as below;

Contract: RollupCore.sol

614:     function requireInactiveStaker(address stakerAddress) internal view { 
615:         require(isStaked(stakerAddress), "NOT_STAKED");
616:         // A staker is inactive if
617:         // a) their last staked assertion is the latest confirmed assertion
618:         // b) their last staked assertion have a child
619:         bytes32 lastestAssertion = latestStakedAssertion(stakerAddress);
620:         bool isLatestConfirmed = lastestAssertion == latestConfirmed();
621:         bool haveChild = getAssertionStorage(lastestAssertion).firstChildBlock > 0; 
622:         require(isLatestConfirmed || haveChild, "STAKE_ACTIVE");
623:     }

As can be seen that, the staker´s latest assertion should be latest confirmed one - Honest Stake OR There should be a child block of the new assertion. If any of these suffice, the staker can initiate to withdraw their stakes.

This opens a possibility for the attacker to delay the withdrawals.

When an dishonest actor stakes on a new assertion by newStakeOnNNewAssertion for a prevAssertion, they should be rivaled by edges.

If the attacker creates a new stake for an assertion (prevAssertion) which was challenged and has childs, they would be eligible to withdraw their funds as per requireInactiveStakerfunction, L:621

Since the attacker´s latestStakedAssertion has already a child - and registered as _latestConfirmed in RollupCore.createNewStake as below, they can just stake and create arbitrary expectedAssertionHash.

Contract: RollupCore.sol

288:     function createNewStake(
289:         address stakerAddress,
290:         uint256 depositAmount,
291:         address withdrawalAddress
292:     ) internal {
293:         uint64 stakerIndex = uint64(_stakerList.length);
294:         _stakerList.push(stakerAddress);
295:         _stakerMap[stakerAddress] = Staker(
296:             depositAmount,
297:  >>         _latestConfirmed, //@audit latestStakedAssertion == _latestConfirmed
298:             stakerIndex,
299:             true,
300:             withdrawalAddress 
302:         emit UserStakeUpdated(stakerAddress, withdrawalAddress, 0, depositAmount);
303:     }

So once the expectedAssertionHash is challenged - which they can do with their other account as well, the attacker only need to wait in order no to waste small stakes in bisections and challenges. Accordingly the attacker can frontrun confirmEdgeByTime by calling returnOldDeposit. This will make the attacker funds withdrawable and the attacker will succeed to make the withdrawals wait for a period of max challengePeriod + first rivaling time.

Tools Used

Manual Review

Recommended Mitigation Steps

Let the withdrawal mechanism have a short timelock

Assessed type

Other

gzeoneth commented 5 months ago

Unclear POC. Assertion can only be confirmed from lastConfirmed, attacker’s child assertion will never be confirmed and hence the stake would not be withdrawable. Unclear what “delay” mean in the topic which seems to be unrelated to the content.

c4-judge commented 5 months ago

Picodes marked the issue as unsatisfactory: Insufficient proof

0xSorryNotSorry commented 5 months ago

Dear @Picodes ,

I apologize for being unclear due to time constraints and wasting the Sponsor´s and your time.

To rephrase all the above:

The attacker creates a new stake (C) challenging the latest confirmed A

A (latest confirmed) - C (Invalid proposed)
\
\
B(invalid child  -  challenged before)

A has already won a challenge and has the child B (invalid)
The stake for C receives a rival since A is valid
The attacker doesn´t bisect it and waits for the max challengePeriod to pass.
The challenge can be confirmed by the time to defeat the attacker by calling confirmEdgeByTime. But the attacker frontruns this call by calling returnOldDeposit due to requireInactiveStaker logic;
```
Contract: RollupCore.sol
```

614: function requireInactiveStaker(address stakerAddress) internal view { 615: require(isStaked(stakerAddress), "NOT_STAKED"); 616: // A staker is inactive if 617: // a) their last staked assertion is the latest confirmed assertion 618: // b) their last staked assertion have a child 619: bytes32 lastestAssertion = latestStakedAssertion(stakerAddress); 620: bool isLatestConfirmed = lastestAssertion == latestConfirmed(); 621: > bool haveChild = getAssertionStorage(lastestAssertion).firstChildBlock > 0; 622: require(isLatestConfirmed || haveChild, "STAKE_ACTIVE"); 623: }


Because `lastestAssertion` (`A`) has a child and this allows the attacker to make their funds withdrawable.
This creates a delay for L2 -> L1 withdrawals as per the challenge time.