Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/assertionStakingPool/AssertionStakingPool.sol#L40

Vulnerability details

Impact

An attacker can create invalid assertion without risking his own funds, instead locking the honest parties' staked funds.

Proof of Concept

Attack explanation in general

A valid assertion is already confirmed, called A. So we have the following assertion tree:

A ---------------------> 
valid                                 
confirmed                           
assertion

An AssertionStakingPool contract is already created with immutable assertionHash equal to zero. When the staked amount in this contract reaches to the requiredStake amount, it creates a valid assertion. I am assuming all the participants in this staking pool are honest. So we have:
```
A ---------------------> B ----------------->
valid                  valid               
confirmed              pending             
assertion              assertion
                    staker:
                    AssertionStakingPool
```

After some time (note that the assertion B is not confirmed yet), the attacker gets a flashloan and creates an invalid assertion, called C as a child of assertion B. So, we will have the following assertion tree.

A ---------------------> B -------------------------> C ------------------------->
valid                  valid                       invalid                     
confirmed              pending                     pending                     
assertion              assertion                   assertion                   
                    staker:                     staker:                     
                    AssertionStakingPool        Attacker

Since the assertion B now has a child, its staker (which is the staking pool) is considered as an inactive staker. So, the attacker, in the same transaction, calls the function makeStakeWithdrawableAndWithdrawBackIntoPool in AssertionStakingPool contract. By doing so all the staked amount will return to the pool again.

In the same transaction, the attacker, by using the available amount in AssertionStakingPool contract, creates another invalid assertion, called D, as a child of assertion C. In other words, the attacker calls createAssertion in the AssertionStakingPool contract to create a new invalid assertion. So we will have:

A ---------------------> B -------------------------> C -------------------------> D ----------------->
valid                  valid                       invalid                     invalid
confirmed              pending                     pending                     pending
assertion              assertion                   assertion                   assertion
                    staker:                     staker:                     staker:
                    AssertionStakingPool        Attacker                    AssertionStakingPool

Since assertion C has a child now, the attacker (who is the staker of assertion C) can withdraw his deposited amount, and repays the flashloan.

Notes:
- The attacker could create two invalid assertions, without risking his own funds.
- The attacker wasted the fund staked in the contract AssertionStakingPool, because the assertions C and D will be challenged by honest parties and will be rejected in the end. Therefore, the staked amount related to assertion D will be locked. This locked staked amount was owned by honest stakers in the AssertionStakingPool. In other words, honest parties who staked to create the valid assertion B are now fined because of creating invalid assertion D.
Attack explanation in details
- In step 2, it is assumed that the deployed AssertionStakingPool contract has zero immutable assertionHash.
```
bytes32 public immutable assertionHash;
constructor(
address _rollup,
bytes32 _assertionHash
) AbsBoldStakingPool(IRollupCore(_rollup).stakeToken()) {
rollup = _rollup;
assertionHash = _assertionHash;
}
```
  https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/assertionStakingPool/AssertionStakingPool.sol#L36
Having this immutable equal to zero allows any user to call createAssertion, when the total staked amount reaches to threshold requiredStake, with arbitrary data assertionInput as the input parameter. In this scenario, I was assuming the honest parties provided valid input to create the valid assertion B. But, later, the attacker misuses this opportunity and provides malicious input to create invalid assertion D.
```
function createAssertion(AssertionInputs calldata assertionInputs) external {
    uint256 requiredStake = assertionInputs.beforeStateData.configData.requiredStake;
    // approve spending from rollup for newStakeOnNewAssertion call
    IERC20(stakeToken).safeIncreaseAllowance(rollup, requiredStake);
    // reverts if pool doesn't have enough stake and if assertion has already been asserted
    IRollupUser(rollup).newStakeOnNewAssertion(requiredStake, assertionInputs, assertionHash, address(this));
}
```
https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/assertionStakingPool/AssertionStakingPool.sol#L40

Because, when immutable assertionHash is zero, the parameter expectedAssertionHash in the subsequent functions will be zero. So, it will bypass the check between the expected assertion hash and the newly created assertion hash.
```
function stakeOnNewAssertion(AssertionInputs calldata assertion, bytes32 expectedAssertionHash)
    public
    onlyValidator
    whenNotPaused
{
    //.....
    require(
        expectedAssertionHash == bytes32(0)
            || getAssertionStorage(expectedAssertionHash).status == AssertionStatus.NoAssertion,
        "EXPECTED_ASSERTION_SEEN"
    );
    //.....
    (bytes32 newAssertionHash, bool overflowAssertion) =
        createNewAssertion(assertion, prevAssertion, expectedAssertionHash);
    //.....
}
```
https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/RollupUserLogic.sol#L163
```
function createNewAssertion(
    AssertionInputs calldata assertion,
    bytes32 prevAssertionHash,
    bytes32 expectedAssertionHash
) internal returns (bytes32 newAssertionHash, bool overflowAssertion) {
    //.....
    require(
        newAssertionHash == expectedAssertionHash || expectedAssertionHash == bytes32(0),
        "UNEXPECTED_ASSERTION_HASH"
    );
    //.....
}
```
https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/RollupCore.sol#L478

In step 4, the attacker calls the function makeStakeWithdrawableAndWithdrawBackIntoPool to return the staked amount to the AssertionStakingPool contract.
```
function makeStakeWithdrawableAndWithdrawBackIntoPool() external {
    makeStakeWithdrawable();
    withdrawStakeBackIntoPool();
}
```
https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/assertionStakingPool/AssertionStakingPool.sol#L60

This is possible, because the assertion B now has a child, so the withdrawal is possible as AssertionStakingPool contract address is considered as an inactive staker.

    function returnOldDeposit() external override onlyValidator whenNotPaused {
        requireInactiveStaker(msg.sender);
        withdrawStaker(msg.sender);
    }

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

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

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

In step 5, the attacker, in the same transaction, calls the function createAssertion in the AssertionStakingPool contract to create the invalid assertion D on behalf of the AssertionStakingPool contract. Note that, the required stake is available in this contract, because in step 4, the staked amount related to the assertion B are returned.
A question may arise that there should be minimumAssertionPeriod delay between assertions creation. So, the attacker can not create the assertion D immediately after the assertion C. But this is not the correct statement. Because the attacker creates assertion D, so that the overflowAssertion is true.
```
    if (!overflowAssertion) {
        uint256 timeSincePrev = block.number - getAssertionStorage(prevAssertion).createdAtBlock;
        // Verify that assertion meets the minimum Delta time requirement
        require(timeSincePrev >= minimumAssertionPeriod, "TIME_DELTA");
    }
```
https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/RollupUserLogic.sol#L204-L208

In other words, suppose when the assertion B is created, nextInboxPosition is set to 100. When the invalid assertion C is going to be created, for example, 10 more messages are added to the inbox. So, assertion C is created with after state inboxPosition = 100 and nextInboxPosition = 110. In the same transaction, assertion D is going to be created. This time, the attacker sets after state inboxPosition to 105 (which is smaller than the target nextInboxPisition = 110). By doing so, the overflowAssertion will become true, as a result, it allows to create assertions without any delay.
```
        if (assertion.afterState.machineStatus != MachineStatus.ERRORED && afterStateCmpMaxInbox < 0) {
            // If we didn't reach the target next inbox position, this is an overflow assertion.
            overflowAssertion = true;
            // This shouldn't be necessary, but might as well constrain the assertion to be non-empty
            require(afterGS.comparePositions(beforeGS) > 0, "OVERFLOW_STANDSTILL");
        }
```
https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/RollupCore.sol#L429-L434

Tools Used

Recommended Mitigation Steps

Preventing from creating another assertion in AssertionStakingPool contract.

function createAssertion(AssertionInputs calldata assertionInputs) external {
    require(!alreadyCreated, "An assertion has already been created.");
    //......
    alreadyCreated = true;
}

Putting an access control on the function createAssertion in the contract AssertionStakingPool, when the immutable assertionHash is zero. This might lead to centralization.

Assessed type

Context

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

Creating invalid assertion using honest parties' staked funds #25