confirmAssertin() can be DOSed.

[howlbot-integration[bot]]

Lines of code

https://github.com/OffchainLabs/bold/blob/0420b9ddb88f71f5e86ca1b3bc256c09346b8315/contracts/src/rollup/RollupUserLogic.sol#L115

Vulnerability details

Impact

Confirmation of honest assertion can be DOSed after delay period when a malicious validator front-runs confirmAsseertion() to post a malicious assertion.

By design, a honest validator should be able confirm assertion and withdraw bonds if there is no rival sibling after delay(1 week) period.

However, the check for rival sibling is incomplete and allows posting rival after delay period has passed.

The implication of this is that honest validator will have to get funds to create a challenge edge(which can be confirmed with time).

Due to the large amount of bond required to create assertion, honest validator might have created an assertion pool to raise funds to create assertion, with this development, honest participant is required to get funds to create an edge so that assertion will be confirmed.

Proof of Concept

The check for rivalry sibling does not include time.

1. Alice creates a valid assertion
2. After delay period is passed, no counter assertion.
3. Alice tries to confirm assertion, but Bob frontons it with a rival assertion, Alice transaction revert, since no edge was provided.

   @> if (prevAssertion.secondChildBlock > 0 ) { //@audit-issue  
           // if the prev has more than 1 child, check if this assertion is the challenge winner
           ChallengeEdge memory winningEdge = IEdgeChallengeManager(prevConfig.challengeManager).getEdge(winningEdgeId);
           require(winningEdge.claimId == assertionHash, "NOT_WINNER");
           require(winningEdge.status == EdgeStatus.Confirmed, "EDGE_NOT_CONFIRMED");
           require(winningEdge.confirmedAtBlock != 0, "ZERO_CONFIRMED_AT_BLOCK");
           // an additional number of blocks is added to ensure that the result of the challenge is widely
           // observable before it causes an assertion to be confirmed. After a winning edge is found, it will
           // always be challengeGracePeriodBlocks before an assertion can be confirmed
           require(
               block.number >= winningEdge.confirmedAtBlock + challengeGracePeriodBlocks,
               "CHALLENGE_GRACE_PERIOD_NOT_PASSED"
           );
       }

Paste testSuccessConfirmUnchallengedAssertions() in Rollup.t.sol

    function testSuccessConfirmUnchallengedAssertions() public returns (bytes32, AssertionState memory, uint64) {
        (bytes32 assertionHash, AssertionState memory state, uint64 inboxcount) = testSuccessCreateAssertion();
        vm.roll(userRollup.getAssertion(genesisHash).firstChildBlock + CONFIRM_PERIOD_BLOCKS + 1);
        bytes32 inboxAccs = userRollup.bridge().sequencerInboxAccs(0);

        //Frontrun Confirmation with invalid assertion that has same parent with assertion to be confirmed
        //uint64 inboxcount = uint64(_createNewBatch());
        AssertionState memory beforeState;
        beforeState.machineStatus = MachineStatus.FINISHED;
        AssertionState memory afterState;
        afterState.machineStatus = MachineStatus.FINISHED;
        afterState.globalState.bytes32Vals[0] = keccak256("FIRST_ASSERTION_BLOCKHASH_A_WRONG_VALUE"); // blockhash
        afterState.globalState.bytes32Vals[1] = keccak256("FIRST_ASSERTION_SENDROOT_A_WRONG_VALUE"); // sendroot
        afterState.globalState.u64Vals[0] = 1; // inbox count
        afterState.globalState.u64Vals[1] = 0; // pos in msg

        bytes32 expectedAssertionHash = RollupLib.assertionHash({
            parentAssertionHash: genesisHash,
            afterState: afterState,
            inboxAcc: userRollup.bridge().sequencerInboxAccs(0)
        });

        vm.prank(validator2);
        userRollup.newStakeOnNewAssertion({
            tokenAmount: BASE_STAKE,
            assertion: AssertionInputs({
                beforeStateData: BeforeStateData({
                    sequencerBatchAcc: bytes32(0),
                    prevPrevAssertionHash: bytes32(0),
                    configData: ConfigData({
                        wasmModuleRoot: WASM_MODULE_ROOT,
                        requiredStake: BASE_STAKE,
                        challengeManager: address(challengeManager),
                        confirmPeriodBlocks: CONFIRM_PERIOD_BLOCKS,
                        nextInboxPosition: afterState.globalState.u64Vals[0]
                    })
                }),
                beforeState: beforeState,
                afterState: afterState
            }),
            expectedAssertionHash: expectedAssertionHash,
            withdrawalAddress: validator2Withdrawal
        });

        vm.prank(validator1);
        userRollup.confirmAssertion(
            assertionHash,
            genesisHash,
            firstState,
            bytes32(0),
            ConfigData({
                wasmModuleRoot: WASM_MODULE_ROOT,
                requiredStake: BASE_STAKE,
                challengeManager: address(challengeManager),
                confirmPeriodBlocks: CONFIRM_PERIOD_BLOCKS,
                nextInboxPosition: firstState.globalState.u64Vals[0]
            }),
            inboxAccs
        );
        return (assertionHash, state, inboxcount);
    }

Test revert with error

Failing tests:
Encountered 1 failing test in test/Rollup.t.sol:RollupTest
[FAIL. Reason: EdgeNotExists(0x0000000000000000000000000000000000000000000000000000000000000000)] testSuccessConfirmUnchallengedAssertions() (gas: 662856)

Tools Used

Manual review and foundry.

Recommended Mitigation Steps

---   if (prevAssertion.secondChildBlock > 0 ) { 
+++   if (prevAssertion.secondChildBlock > 0 && prevAssertion.firstChildBlock + prevConfig.confirmPeriodBlocks > prevAssertion.secondChildBlock) { 
            // if the prev has more than 1 child, check if this assertion is the challenge winner
            ChallengeEdge memory winningEdge = IEdgeChallengeManager(prevConfig.challengeManager).getEdge(winningEdgeId);
            require(winningEdge.claimId == assertionHash, "NOT_WINNER");
            require(winningEdge.status == EdgeStatus.Confirmed, "EDGE_NOT_CONFIRMED");
            require(winningEdge.confirmedAtBlock != 0, "ZERO_CONFIRMED_AT_BLOCK");
            // an additional number of blocks is added to ensure that the result of the challenge is widely
            // observable before it causes an assertion to be confirmed. After a winning edge is found, it will
            // always be challengeGracePeriodBlocks before an assertion can be confirmed
            require(
                block.number >= winningEdge.confirmedAtBlock + challengeGracePeriodBlocks,
                "CHALLENGE_GRACE_PERIOD_NOT_PASSED"
            );
        }

Assessed type

DoS

[gzeoneth]

Invalid, expected behavior. There are no advantage to do this as the honest validator can create a challenge and immediately win due to time already accumulated in the assertion.

[Picodes]

This would lead to a direct loss of funds for the attacker?

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof