c4-bot-7 commented 2 months ago

Lines of code

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

Vulnerability details

Impact

RollupUserLogic.sol holds a lot of important functions which are protected by the whenNotPaused modifier, hence ensuring the functions cannot only be called when the contract is not paused. This indicates that for some reason or the other or in case of emergencies, these functions should be inaccessible until the contract is unpaused. The issue, however, is that the contract doesn't expose internal pause and unpause function, causing that in dire cases, the contract cannot be paused (or unpaused). This means regardless of the situation, the functions protected by the whenNotPaused modifier will still be open to users to access.

Proof of Concept

RollupUserLogic.sol inherits abstract contract RollupCore.sol, which inherits PausableUpgradeable.sol;

contract RollupUserLogic is RollupCore, UUPSNotUpgradeable, IRollupUser {

abstract contract RollupCore is IRollupCore, PausableUpgradeable {

And upon its deployment and core intialization, the __Pausable_init(); function is called, which automatically sets the contract's state to unpaused.

    function initializeCore(AssertionNode memory initialAssertion, bytes32 assertionHash) internal {
        __Pausable_init();
        initialAssertion.status = AssertionStatus.Confirmed;
        _assertions[assertionHash] = initialAssertion;
        _latestConfirmed = assertionHash;
    }

Now, RollupUserLogic.sol contains two sets of functions, ones with the whenNotPaused modifier and ones without it, indicating that some functions are intended to only work when the contract is not paused, while some are meant to work regardless of the contract's paused status. As an example, two sets of functions are compared below.


    function removeWhitelistAfterValidatorAfk() external {
    ...
    }

    function confirmAssertion(
        bytes32 assertionHash,
        bytes32 prevAssertionHash,
        AssertionState calldata confirmState,
        bytes32 winningEdgeId,
        ConfigData calldata prevConfig,
        bytes32 inboxAcc
    ) external onlyValidator whenNotPaused {
    ...
    }

Going through both RollupUserLogic.sol and RollupCore.sol, no public or external pause/unpause functions could be found, causing that in cases where the contract needs to be paused, there's no way to do that. The _pause/_unpause functions in the inherited PausableUpgradeable.sol are both internal, hence functions protected by the whenNotPaused are affected and can't be paused.

    function _pause() internal virtual whenNotPaused {
        _paused = true;
        emit Paused(_msgSender());
    }

    /**
     * @dev Returns to normal state.
     *
     * Requirements:
     *
     * - The contract must be paused.
     */
    function _unpause() internal virtual whenPaused {
        _paused = false;
        emit Unpaused(_msgSender());
    }

The functions are:

Tools Used

Manual code review

Recommended Mitigation Steps

Expose the pause and unpause function.

Assessed type

Context

carlitox477 commented 1 month ago

I think you didn't get the idea of the proxy implementation, function pause and unpause are managed by Admin logic.

This proxy is like a 2 faucets diamond proxy, the storage is definied in RollupCore and must be inherit by every faucet to access to the storage, in addition no other faucet must create a storage variable. The two faucets are Admin logic and user logic, and its validation is done by function _fallback which will check who is calling the function, if it is admin address, then it will only access to admin faucet, being able to call pause and resume and altering the storage acted by the two faucets.

Here how OZ library handle the call of proxy calls. Hoping to have been clear with this explanation.

godzillaba commented 1 month ago

hi @ZanyBonzy , @carlitox477 is correct here, the pause and unpause functions exist in RollupAdminLogic

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

RollupUserLogic.sol in intended to be pausable but cannot be paused/unpaused. #217