howlbot-integration[bot] commented 1 month ago

Lines of code

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

Vulnerability details

Impact

anyTrustFastConfirmer is supposed to be set only on an AnyTrust chain, it can force confirm any pending assertion, this is a feature which allows a committee members (multi-sig) to confirm assertions quicker.

For this reason, deadline, prev, and challenge validations are skipped, unlike other kind of validations (e.g. parentAssertionHash, confirmState, assertionHash is currently pending) are still applied. Those are done in confirmAssertionInternal.

However, it doesn't check the status of its sibling (rival assertion) which allows confirming the intended assertion even if it has a confirmed sibling. This should never happen as it creates two correct assertion chains. In other words, we would get two confirmed assertions which have the same parent. This breaks a core invariant in BoLD that is, at any point of time, only one assertion chain can be correct.

Proof of Concept

Assume we have the following assertion chain

A -- B -- C

C is confirmed.

anyTrustFastConfirmer want to confirm a new assertion under B, let's call it D

So we would have

 A -- B -- C
       \-- D

anyTrustFastConfirmer will call fastConfirmNewAssertion which does the following:

It requires expectedAssertionHash to be supplied.


    // Must supply expectedAssertionHash to fastConfirmNewAssertion
    require(expectedAssertionHash != bytes32(0), "EXPECTED_ASSERTION_HASH");

[RollupUserLogic.sol:L278](https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/rollup/RollupUserLogic.sol#L278)

- It checks for `prevAssertion` existence 
```solidity
getAssertionStorage(prevAssertion).requireExists();

RollupUserLogic.sol:L286

If the assertion to be created does not exist, it create a new one, otherwise, it skips this part.

    if (status == AssertionStatus.NoAssertion) {
        // If not exists, we create the new assertion
        (bytes32 newAssertionHash,) = createNewAssertion(assertion, prevAssertion, expectedAssertionHash);

RollupUserLogic.sol:L288

Last step, call fastConfirmAssertion which checks if the caller is the anyTrustFastConfirmer, then calls confirmAssertionInternal skipping the validations that are normally done.
If we examine confirmAssertionInternal, we find that it has two checks only;
- it checks if the assertion exists and is pending.
```
    // Check that assertion is pending, this also checks that assertion exists
    require(assertion.status == AssertionStatus.Pending, "NOT_PENDING");
```
  RollupCore.sol:L243-L245
- validate data against assertionHash pre-image

As noticed, there is no check if the sibling is already confirmed. Thus, we would have two siblings confirmed which breaks a core invariant in BoLD.

Tools Used

Manual analysis

Recommended Mitigation Steps

Checks if there is a already another child confirmed, revert. This is recommended to prevent breaking the invariant. However, if you still want to confirm the assertion anyway, remove the confirmation of the other confirmed child.

Assessed type

Other

gzeoneth commented 1 month ago

Out-of-scope. anyTrustFastConfirmer is a highly privileged role that is allowed to confirm arbitrary assertions. That invariant does not apply for privileged actions.

Also, it is a design choice to not requiring anyTrustFastConfirmer to strictly confirm from latest confirmed as it allow skipping through assertions. For example, assertions can be made continuously (without full participation of DAC); but fast confirmation (require full participation of DAC) can happen only once every day.

c4-judge commented 1 month ago

Picodes marked the issue as unsatisfactory: Out of scope

koolexcrypto commented 1 month ago

Hi @Picodes

Not sure how this is out of scope. I understand that it is a highly privileged role, However, it is not mentioned as a trusted role in All trusted roles in the protocol in README, nor as a known issue. This makes sense since the main goal of BoLD is to enable permissionless validation as it is emphasised everywhere. Furthermore, the action seems to be irreversible (that's the reason why I reported it tbh).

Also, since this action doesn't involve upgrading contracts (changing code), I believe an invariant should always hold regardless of any action by any role. If there is a role that can break an invariant, this makes it not an invariant.

Thank you!

Picodes commented 1 month ago

@koolexcrypto it is clearly mentioned in the readme that Centralization risk is out-of-scope, and unless proven otherwise what's described here falls within the power of this role

koolexcrypto commented 1 month ago

Thank you for your time.

I do agree. I may have misunderstood README.

I'm not sure at this point, would that qualify as QA?