Risk of Storage Collision in OptimismMintableXERC20 Upgrades

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/xERC20/contracts/optimism/OptimismMintableXERC20.sol#L11

Vulnerability details

Note on Risk Classification:

While automated tools classify the absence of a storage gap in upgradeable contracts as a low-risk issue, I believe that the specific circumstances and potential consequences in our case warrant a higher risk classification. This reassessment is based on several factors:

1. Contract Value and Functionality: The contracts in question manage significant assets and facilitate critical functionalities within their respective ecosystems. Any disruption caused by storage collisions could therefore lead to disproportionately high impacts.

2. Complexity of Inheritance: Given that XERC20 serves as a base for other contracts such as OptimismMintableXERC20, the propagation effect of this vulnerability could affect multiple derived contracts, thus amplifying potential security threats.

3. Future Upgradeability: The expected frequency and complexity of future upgrades increase the likelihood that the absence of a storage gap could result in serious issues, making proactive mitigation crucial.

Impact

The lack of explicit storage gaps in XERC20, which is a base contract for OptimismMintableXERC20, may lead to storage collisions when new state variables are added to XERC20 in future upgrades. This issue can cause data corruption or incorrect data mapping, severely impacting the contract's integrity and functionality.

Proof of Concept

The XERC20 contract is inherited by OptimismMintableXERC20. If XERC20 has new state variables added, without reserved storage spaces (gaps), these new variables would displace the storage mapping of OptimismMintableXERC20.

// XERC20.sol
contract XERC20 is Initializable, ERC20Upgradeable, OwnableUpgradeable, IXERC20, ERC20PermitUpgradeable {
    // Storage variables
    address public FACTORY;
    address public lockbox;
    mapping(address => Bridge) public bridges;
    ...
}

// OptimismMintableXERC20.sol
contract OptimismMintableXERC20 is ERC165Upgradeable, XERC20, IOptimismMintableERC20 {
    address public l1Token;
    ...
}

Test Case (Foundry)

// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.4 <0.9.0;

import "forge-std/Test.sol";
import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import "contracts/Bridge/xERC20/contracts/XERC20.sol";
import "contracts/Bridge/xERC20/contracts/optimism/OptimismMintableXERC20.sol";

// Mock versions of the contracts to simulate an upgrade that adds new storage variables.
contract MockXERC20V2 is XERC20 {
    uint256[1] value;
}

// This contract simulates an upgraded version of OptimismMintableXERC20 including changes from MockXERC20V2.
contract MockOptimismMintableXERC20V2 is ERC165Upgradeable, MockXERC20V2, IOptimismMintableERC20 {
    /**
     * @notice The address of the l1 token (remoteToken)
     */
    address public l1Token;

    /**
     * @notice The address of the optimism canonical bridge
     */
    address public optimismBridge;

    /// @dev Prevents implementation contract from being initialized.
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    /**
     * @notice Constructs the initial config of the XERC20
     *
     * @param _name The name of the token
     * @param _symbol The symbol of the token
     * @param _factory The factory which deployed this contract
     */
    function initialize(
        string memory _name,
        string memory _symbol,
        address _factory,
        address _l1Token,
        address _optimismBridge
    ) public initializer {
        __ERC165_init();
        __XERC20_init(_name, _symbol, _factory);
        l1Token = _l1Token;
        optimismBridge = _optimismBridge;
    }

    function supportsInterface(
        bytes4 interfaceId
    ) public view override(ERC165Upgradeable) returns (bool) {
        return
            interfaceId == type(IOptimismMintableERC20).interfaceId ||
            super.supportsInterface(interfaceId);
    }

    function remoteToken() public view override returns (address) {
        return l1Token;
    }

    function bridge() public view override returns (address) {
        return optimismBridge;
    }

    function mint(address _to, uint256 _amount) public override(XERC20, IOptimismMintableERC20) {
        XERC20.mint(_to, _amount);
    }

    function burn(address _from, uint256 _amount) public override(XERC20, IOptimismMintableERC20) {
        XERC20.burn(_from, _amount);
    }
}

// Contract to run tests demonstrating the storage collision.
contract PoC is Test {
    OptimismMintableXERC20 oxERC20Impl;
    MockOptimismMintableXERC20V2 oxERC20V2Impl;

    TransparentUpgradeableProxy oxERC20;

    address proxyAdmin;
    address mockL1Token;
    address mockOptimismBridge;

    function setUp() public {
        // xERC20Impl = new XERC20();
        oxERC20Impl = new OptimismMintableXERC20();
        oxERC20V2Impl = new MockOptimismMintableXERC20V2();

        mockL1Token = makeAddr("l1Token");
        mockOptimismBridge = makeAddr("optimismBridge");
        proxyAdmin = makeAddr("proxyAdmin");

        // Deploying the proxy for OptimismMintableXERC20 using the initialize function with initial parameters.
        oxERC20 = new TransparentUpgradeableProxy(
            address(oxERC20Impl),
            proxyAdmin,
            abi.encodeCall(
                OptimismMintableXERC20.initialize,
                ("name", "symbol", address(this), mockL1Token, mockOptimismBridge)
            )
        );
    }

    function test_addingNewStorageVariableToXERC20CouldCorrupteStateVariablesOfOptimismMintableXERC20() public {
        // Verifying the state before upgrading to new version
        assertEq(getRemoteToken(address(oxERC20)), mockL1Token);

        // Upgrading the proxy to the new contract version that includes additional storage variables.
        vm.prank(proxyAdmin);
        (bool success, ) = address(oxERC20).call(
            abi.encodeWithSelector(
                ITransparentUpgradeableProxy.upgradeTo.selector,
                address(oxERC20V2Impl)
            )
        );
        require(success);

        // Checking the state corruption by verifying if the remoteToken address has changed unexpectedly.
        assertNotEq(getRemoteToken(address(oxERC20)), mockL1Token);
    }

    function getRemoteToken(address _oxERC20) private returns(address addr) {
        (, bytes memory result) = _oxERC20.call(abi.encodeWithSignature("remoteToken()"));
        return abi.decode(result, (address));
    }
}

Test output

2024-04-renzo main* 3s
❯ forge test -vvvv
[⠊] Compiling...
[⠘] Compiling 1 files with 0.8.19
[⠃] Solc 0.8.19 finished in 1.80s
Compiler run successful!

Ran 1 test for test/XERC20.t.sol:PoC
[PASS] test_addingNewStorageVariableToXERC20CouldCorrupteStateVariablesOfOptimismMintableXERC20() (gas: 37486)
Traces:
  [37486] PoC::test_addingNewStorageVariableToXERC20CouldCorrupteStateVariablesOfOptimismMintableXERC20()
    ├─ [9498] TransparentUpgradeableProxy::remoteToken()
    │   ├─ [2398] OptimismMintableXERC20::remoteToken() [delegatecall]
    │   │   └─ ← [Return] l1Token: [0x2d7dC9F6B280314624bf3d5c5adc5bF24753B5ad]
    │   └─ ← [Return] l1Token: [0x2d7dC9F6B280314624bf3d5c5adc5bF24753B5ad]
    ├─ [0] VM::assertEq(l1Token: [0x2d7dC9F6B280314624bf3d5c5adc5bF24753B5ad], l1Token: [0x2d7dC9F6B280314624bf3d5c5adc5bF24753B5ad]) [staticcall]
    │   └─ ← [Return]
    ├─ [0] VM::prank(proxyAdmin: [0x129d58674d5511922A08169F6965b6958A3D07b0])
    │   └─ ← [Return]
    ├─ [7700] TransparentUpgradeableProxy::upgradeTo(MockOptimismMintableXERC20V2: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
    │   ├─ emit Upgraded(implementation: MockOptimismMintableXERC20V2: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
    │   └─ ← [Return]
    ├─ [2998] TransparentUpgradeableProxy::remoteToken()
    │   ├─ [2398] MockOptimismMintableXERC20V2::remoteToken() [delegatecall]
    │   │   └─ ← [Return] optimismBridge: [0xBa2ee9789e3A6e7dE5027E409B7b393C350b8619]
    │   └─ ← [Return] optimismBridge: [0xBa2ee9789e3A6e7dE5027E409B7b393C350b8619]
    ├─ [0] VM::assertNotEq(optimismBridge: [0xBa2ee9789e3A6e7dE5027E409B7b393C350b8619], l1Token: [0x2d7dC9F6B280314624bf3d5c5adc5bF24753B5ad]) [staticcall]
    │   └─ ← [Return]
    └─ ← [Stop]

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.58ms (101.92µs CPU time)

Ran 1 test suite in 117.70ms (1.58ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Solidity Compiler
• Foundry for Testing
• OpenZeppelin Contracts (Upgradeable)

Recommended Mitigation Steps

Strategic Structural Changes

• Consider Independent Upgradeability: Evaluate the feasibility of designing OptimismMintableXERC20 as an independent upgradeable contract, separate from XERC20. This approach reduces dependency risks and isolates upgrades, significantly mitigating the potential for storage collisions across dependent contracts.

Technical Improvements

• Introduce Storage Gaps: Ensure future-proofing by modifying the XERC20 contract to include reserved storage slots that accommodate additional variables in future upgrades: uint256[50] private __gap; // Reserve 50 storage slots for future use

• Specify Gap Usage: Clearly define how these gaps should be utilized in future contract versions to prevent misuse or misalignment of the storage structure.

• Best Practices Manual: Create a comprehensive guide on best practices for upgrading contracts within your ecosystem, focusing on maintaining the integrity of the storage layout.

Assessed type

Upgradable

[alcueca]

I don't see that the upgradability feature is seriously damaged by the lack of a gap, given this is an ERC20. Potential issues like storage corruption should be caught during governance testing. QA is appropriate.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a

[c4-judge]

alcueca marked the issue as grade-b