Skip to content

Latest commit

 

History

History
179 lines (134 loc) · 9.99 KB

M-03.md

File metadata and controls

179 lines (134 loc) · 9.99 KB

System contract upgrades will revert when provided value

Summary

System contract upgrades are proposed on L1 to be performed on L2.

Some upgrades need to be provided ETH value in order to perform deployment operations within their constructors.

ForceDeployUpgrader is the contract that calls force deployment during the L2 system contract upgrade.

Impact

Due to an error on this contract, it is not possible to execute an upgrade on L2 with value > 0.

System contract upgrades take time to be proposed + executed on L1 to send the upgrade transaction to L2.

If a system upgrade fails, it will take time and effort from the Governance owner and the security council to fix the issue and retry the execution.

This time window will also attract the attention of malicious actors, as system upgrades are rare events, and a failed transaction can expose an upgrade to a potential vulnerability.

Impact assessed as Medium according to "the function of the protocol or its availability could be impacted".

Proof of Concept

The expected upgrade flow on L2 is:

Note that ComplexUpgrader has a payable to receive ETH and ultimately send it to the ContractDeployer. ContractDeployer checks that the value it receives corresponds with the sum to be used by the upgraded contracts.

The problem is that the delegated ForceDeployUpgrader does not send that value to the ContractDeployer, making the transaction revert due to the inconsistency between the expected value and the one received (which is zero).

The missing code can be seen in the Recommendation section. The Coded Proof of Concept section shows how upgrade deployments that require ETH revert on the current codebase and proves that the recommended changes fixes this.

Coded Proof of Concept

Copy this file to code/system-contracts/contracts/ForceDeployUpgrader.sol. It's a copy of ForceDeployUpgrader.sol for the purpose of running an integration test with the system contracts on the same Hardhat environment and shared artifacts. (note that the corresponding interface and the constants were copied to the file but the contract remains the same).

// SPDX-License-Identifier: MIT OR Apache-2.0

pragma solidity ^0.8.0;

uint160 constant SYSTEM_CONTRACTS_OFFSET = 0x8000; // 2^15
address constant DEPLOYER_SYSTEM_CONTRACT = address(SYSTEM_CONTRACTS_OFFSET + 0x06);

interface IContractDeployer {
    struct ForceDeployment {
        bytes32 bytecodeHash;
        address newAddress;
        bool callConstructor;
        uint256 value;
        bytes input;
    }

    function forceDeployOnAddresses(ForceDeployment[] calldata _deployParams) external payable;
}

/// @custom:security-contact [email protected]
/// @notice The contract that calls force deployment during the L2 system contract upgrade.
/// @notice It is supposed to be used as an implementation of the ComplexUpgrader.
contract ForceDeployUpgrader {
    /// @notice A function that performs force deploy
    /// @param _forceDeployments The force deployments to perform.
    function forceDeploy(IContractDeployer.ForceDeployment[] calldata _forceDeployments) external {
        IContractDeployer(DEPLOYER_SYSTEM_CONTRACT).forceDeployOnAddresses(_forceDeployments);
    }
}

Copy this file to code/system-contracts/test/ComplexUpgraderPOC.spec.ts and run the test suite via the quick-setup.sh.

The expected behavior after the fix can also be checked by commenting and uncommenting some lines pointed out on the test.

import { expect } from 'chai';
import { ComplexUpgrader__factory, ContractDeployer__factory, ForceDeployUpgrader } from '../typechain-types';
import { DEPLOYER_SYSTEM_CONTRACT_ADDRESS, FORCE_DEPLOYER_ADDRESS } from './shared/constants';
import { Wallet, utils } from 'zksync-web3';
import { getWallets, deployContract, setCode } from './shared/utils';
import { network, ethers } from 'hardhat';
import { loadArtifact, publishBytecode } from '../test/shared/utils';
import { SYSTEM_CONTRACTS } from '../scripts/constants';

describe('ComplexUpgrader POC tests', function () {
    const RANDOM_SYSTEM_ADDRESS = ethers.utils.getAddress('0x0000000000000000000000000000000000000666');
    const COMPLEX_UPGRADER_CONTRACT_ADDRESS = SYSTEM_CONTRACTS.complexUpgrader.address;

    let wallet: Wallet;
    let forceDeployUpgrader: ForceDeployUpgrader;
    
    let deployableArtifact;

    before(async () => {
        wallet = getWallets()[0];
        forceDeployUpgrader = (await deployContract('ForceDeployUpgrader')) as ForceDeployUpgrader;

        const complexUpgraderArtifact = await loadArtifact('ComplexUpgrader');
        await setCode(COMPLEX_UPGRADER_CONTRACT_ADDRESS, complexUpgraderArtifact.bytecode);

        const contractDeployerArtifact = await loadArtifact('ContractDeployer');
        await setCode(DEPLOYER_SYSTEM_CONTRACT_ADDRESS, contractDeployerArtifact.bytecode);

        deployableArtifact = await loadArtifact('Deployable');
        await publishBytecode(deployableArtifact.bytecode);
    });

    it.only('reverts for ComplexUpgrader + ForceDeployUpgrader with value > 0', async () => {
        // Data of the contract to be deployed
        const deploymentData = [{
            bytecodeHash: utils.hashBytecode(deployableArtifact.bytecode),
            newAddress: RANDOM_SYSTEM_ADDRESS,
            callConstructor: true,
            value: 1,
            input: '0x'
        }];
        const encodedData = forceDeployUpgrader.interface.encodeFunctionData('forceDeploy', [deploymentData]);

        // Impersonate the force deployer and give them 1 wei for the deployment
        const force_deployer = await ethers.getSigner(FORCE_DEPLOYER_ADDRESS);
        await network.provider.send("hardhat_setBalance", [
            force_deployer.address,
            "0x01",
          ]);

        await network.provider.request({
            method: 'hardhat_impersonateAccount',
            params: [FORCE_DEPLOYER_ADDRESS]
        });

        // Attempt to perform the deployment providing `value`, but it doesn't work
        await expect(
            ComplexUpgrader__factory
                .connect(COMPLEX_UPGRADER_CONTRACT_ADDRESS, force_deployer)
                .upgrade(forceDeployUpgrader.address, encodedData, {value: 1})
        ).to.be.reverted;

        // You can prove the fix and expected behavior by:
        // - Comment the above lines and uncomment the ones below
        // - Add `payable` to `ForceDeployUpgrader::forceDeploy()`
        // - Add `{value: msg.value}` to `IContractDeployer(DEPLOYER_SYSTEM_CONTRACT).forceDeployOnAddresses(_forceDeployments);`

        // const contractDeployer = ContractDeployer__factory.connect(DEPLOYER_SYSTEM_CONTRACT_ADDRESS, wallet);
        // await expect(
        //     ComplexUpgrader__factory
        //         .connect(COMPLEX_UPGRADER_CONTRACT_ADDRESS, force_deployer)
        //         .upgrade(forceDeployUpgrader.address, encodedData, {value: 1})
        // ).to.emit(contractDeployer, 'ContractDeployed')
        //  .withArgs(COMPLEX_UPGRADER_CONTRACT_ADDRESS, utils.hashBytecode(deployableArtifact.bytecode), RANDOM_SYSTEM_ADDRESS);

        await network.provider.request({
            method: 'hardhat_stopImpersonatingAccount',
            params: [FORCE_DEPLOYER_ADDRESS]
        });
    });
});

Recommendation

In order to fix it, it is important to make both of these changes:

  • Add payable to forceDeploy()
  • Add {value: msg.value} to the forceDeployOnAddresses() call
  contract ForceDeployUpgrader {
      /// @notice A function that performs force deploy
      /// @param _forceDeployments The force deployments to perform.
-      function forceDeploy(IContractDeployer.ForceDeployment[] calldata _forceDeployments) external {
-          IContractDeployer(DEPLOYER_SYSTEM_CONTRACT).forceDeployOnAddresses(_forceDeployments);
+      function forceDeploy(IContractDeployer.ForceDeployment[] calldata _forceDeployments) external payable {
+          IContractDeployer(DEPLOYER_SYSTEM_CONTRACT).forceDeployOnAddresses{value: msg.value}(_forceDeployments);
      }
  }