howlbot-integration[bot] commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/rollup/RollupAdminLogic.sol#L143-L161 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/rollup/RollupUserLogic.sol#L62-L75

Vulnerability details

Description

If validator whitelist is not meant to be diasable, and rollup contract is pause for enough time, the whitelist can be removed by calling removeWhitelistAfterFork and removeWhitelistAfterValidatorAfk immediatelly after admin unpause rollup contract

Impact

If:

Rollup contract is paused
Validator whitelist mechanis is enable and is not meant to be disabled
There is no intention of Rollup admin to force the inclusion of a new assertion

Then, immediately after admin call RollupAdminLogic.unpause(), anyone can call removeWhitelistAfterFork and removeWhitelistAfterValidatorAfk, enforcing whitelist mechanism to disable and enabling anyone to submit new assertions.

Recommended mitgation

To solve this:

Add a check in removeWhitelistAfterFork and removeWhitelistAfterValidatorAfk to ensure that rollup contract is not paused
Add a grace period to check if rollup when removeWhitelistAfterFork or removeWhitelistAfterValidatorAfk are called to allow validators to confirm assertions.

In this way:

abstract contract RollupCore is IRollupCore, PausableUpgradeable {
    // After already decleared variable
    //...
    uint256 public unpauseTimestampGracePeriod
    //...
}

    contract RollupAdminLogic is RollupCore, IRollupAdmin, DoubleLogicUUPSUpgradeable {
    //...
-      function resume() external override {
+      function resume(uint256 _unpauseTimestampGracePeriod) external override {
+          require(_unpauseTimestampGracePeriod >= block.timestamp);
+          unpauseTimestampGracePeriod = _unpauseTimestampGracePeriod;
            _unpause();
            emit OwnerFunctionCalled(4);
        }
    }
    // ...
}

    contract RollupUserLogic is RollupCore, UUPSNotUpgradeable, IRollupUser {
    //...

+   function _requirePauseChecks(){
+       require(!paused(), "CONTRACT_PAUSED");
+       require(block.timestamp > unpauseTimestampGracePeriod, "UNPAUSE_GRACE_PERIOD");
+   }

    function removeWhitelistAfterFork() external {
+       // unpause checks
+       _requirePauseChecks();
        require(!validatorWhitelistDisabled, "WHITELIST_DISABLED");
        require(_chainIdChanged(), "CHAIN_ID_NOT_CHANGED");
        validatorWhitelistDisabled = true;
    }

    /**
     * @notice Remove the whitelist after the validator has been inactive for too long
     */
    function removeWhitelistAfterValidatorAfk() external {
+       // unpause checks
+       _requirePauseChecks();
        require(!validatorWhitelistDisabled, "WHITELIST_DISABLED");
        require(_validatorIsAfk(), "VALIDATOR_NOT_AFK");
        validatorWhitelistDisabled = true;
    }
    //...

Assessed type

Invalid Validation

c4-judge commented 6 months ago

Picodes marked the issue as satisfactory

c4-judge commented 5 months ago

Picodes changed the severity to QA (Quality Assurance)