howlbot-integration[bot] commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/rollup/RollupUserLogic.sol#L210-L216 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/rollup/RollupUserLogic.sol#L331-L342 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/rollup/RollupUserLogic.sol#L366-L368 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/assertionStakingPool/AssertionStakingPool.sol#L40-L46

Vulnerability details

Impact

Users will not be able to create assertions leading to a DOS on a core functionality of the protocol.

Proof of Concept

Note that the IRollupUser is not deployed with funds deposited in it.

User’s create an assertion using the AssertionStakingPool::createAssertion(...) function, at a high level the order of execution is as shown below

L0 -> AssertionStakingPool::createAssertion(...)
L1 --> IERC20(stakeToken).safeIncreaseAllowance(rollup, ...)
L2 --> IRollupUser(rollup)::newStakeOnNewAssertion(...)
L3 ---> RollupUserLogic::newStakeOnNewAssertion(...)
...
L4 ----> RollupUserLogic::stakeOnNewAssertion(...)
....
L5 -----> IERC20(stakeToken).safeTransfer(loserStakeEscrow, .requiredStake) 
L6 --> receiveTokens(tokenAmount);
L7 ---> IERC20(stakeToken).safeTransferFrom(msg.sender, address(this), tokenAmount)

I have marked the portions of interest (as L1, L2, L5 and L7) in this high level trace. Lets go over the important steps

L1 - the creator of the assertion gives the rollup contract allowance (1) to spend it’s stake in order to create the assertion
L2 the rollup contract starts the creation of the assertion
L5 the rollup contract transfers an equivalent of the users stake amount to the loserStakeEscrow contract from it’s own stakeToken balance (at this point its balance may be less than the requiredStake
L7 the rollup contract collects the users stake from the user

However, when there are no fund in the rollup contract, the AssertionStakingPool::createAssertion(...) call will revert at L5 because in the current implementation, the order of execution requires that the rollup contract transfer funds to the loserStakeEscrow before actually receiving the funds from the user who is creating the assertion.


File: AssertionStakingPool.sol
40:     function createAssertion(AssertionInputs calldata assertionInputs) external {
41:         uint256 requiredStake = assertionInputs.beforeStateData.configData.requiredStake;
42:         // approve spending from rollup for newStakeOnNewAssertion call
43:         IERC20(stakeToken).safeIncreaseAllowance(rollup, requiredStake);
44:         // reverts if pool doesn't have enough stake and if assertion has already been asserted
45:         IRollupUser(rollup).newStakeOnNewAssertion(requiredStake, assertionInputs, assertionHash, address(this));
46:     }

File: RollupUserLogic.sol
331:     function newStakeOnNewAssertion(
332:         uint256 tokenAmount,
333:         AssertionInputs calldata assertion,
334:         bytes32 expectedAssertionHash,
335:         address withdrawalAddress
336:     ) public {
337:         require(withdrawalAddress != address(0), "EMPTY_WITHDRAWAL_ADDRESS");
338:         _newStake(tokenAmount, withdrawalAddress);
339:         stakeOnNewAssertion(assertion, expectedAssertionHash);
340:         /// @dev This is an external call, safe because it's at the end of the function
341:         receiveTokens(tokenAmount);
342:     }
...

163:     function stakeOnNewAssertion(AssertionInputs calldata assertion, bytes32 expectedAssertionHash)
164:         public
165:         onlyValidator
166:         whenNotPaused
167:     {
...
210:         if (!getAssertionStorage(newAssertionHash).isFirstChild) {
216:             // We assume assertion.beforeStateData is valid here as it will be validated in createNewAssertion
217:             // only 1 of the children can be confirmed and get their stake refunded
218:             // so we send the other children's stake to the loserStakeEscrow
219:             // NOTE: if the losing staker have staked more than requiredStake, the excess stake will be stuck
220:  @>         IERC20(stakeToken).safeTransfer(loserStakeEscrow, assertion.beforeStateData.configData.requiredStake);
221:         }
...

366:     function receiveTokens(uint256 tokenAmount) private {
361:  @>     IERC20(stakeToken).safeTransferFrom(msg.sender, address(this), tokenAmount);
362:     }

Poc Summary

current stakeToken balance of the rollup contract is zero or less than requiredStake
Alice calls AssertionStakingPool::createAssertion(...) to create a new assertion
rollup tries to lock Alice requiredStake but the function reverts because the rollup does not have up to the current requiredStake amount of the stakeToken
Alice assertion could not be created because the call reverts.

Tools Used

Manual review

Recommended Mitigation Steps

Modify the RollupUserLogic::newStakeOnNewAssertion(...) function as shown below

File: RollupUserLogic.sol
331:     function newStakeOnNewAssertion(
332:         uint256 tokenAmount,
333:         AssertionInputs calldata assertion,
334:         bytes32 expectedAssertionHash,
335:         address withdrawalAddress
336:     ) public {
337:         require(withdrawalAddress != address(0), "EMPTY_WITHDRAWAL_ADDRESS");
338:         _newStake(tokenAmount, withdrawalAddress);

339: -       stakeOnNewAssertion(assertion, expectedAssertionHash);
340: -       /// @dev This is an external call, safe because it's at the end of the function
341: -       receiveTokens(tokenAmount);

339: +       receiveTokens(tokenAmount);
340: +       /// @dev This is an external call, safe because it's at the end of the function
341: +       stakeOnNewAssertion(assertion, expectedAssertionHash);

342:     }

The rollup should move the funds from the user’s account before it calls stakeOnNewAssertion(...).

Assessed type

DoS

gzeoneth commented 1 month ago

Invalid. The rollup would already have token to transfer to loser escrow since the first child would have sent token there. This is unless you try to create a second child on an assertion that is before latest confirmed, where the staker of latest confirmed had withdrawn his stake and thus the contract may have no fund. This edge case is out-of-scope tho because any assertion created on an assertion before latest confirmed should never be confirmable, and is ok to be reverted.

Picodes commented 1 month ago

Downgrading to QA as the edge case scenario is indeed outside of the normal behavior of the system

c4-judge commented 1 month ago

Picodes changed the severity to QA (Quality Assurance)