Unable to run actions other than upgrade from a Gnosis Safe multisig

Support Defender
1

:computer: Environment

Using Defender on Chrome / Rinkeby / Metamask.

:memo:Details

I'm currently trying to setup a development flow that is easy to integrate with Defender. Using hardhat, hardhat-deploy and hardhat-upgradeable, I'm able to deploy upgradeable contracts, upgrade them, even prepare them. During the deployment process, I automatically change the Proxy ownership to a multisig that I'm using inside Defender.

Afterwards, it works for upgrades, but I'm unable to run Admin Actions or Pause/Unpause actions from the multisig. There's an rpc error I can see in the logs (with no details).

:1234: Code to reproduce

For the contracts

Versioned.sol

// SPDX-License-Identifier: MIT
pragma solidity 0.8.6;

import '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol';

contract Versioned {
    uint256 version;

    mapping(uint256 => bool) private _initialized;

    mapping(uint256 => bool) private _initializing;

    function getVersion() public view returns (uint256) {
        return version;
    }

    modifier initializer(uint256 _version) {
        require(
            _initializing[_version] || !_initialized[_version],
            'Initializable: contract version is already initialized'
        );

        bool isTopLevelCall = !_initializing[_version];
        if (isTopLevelCall) {
            _initializing[_version] = true;
            _initialized[_version] = true;
            version = _version;
        }

        _;

        if (isTopLevelCall) {
            _initializing[_version] = false;
        }
    }
}

Pausable.sol

// SPDX-License-Identifier: MIT
pragma solidity 0.8.6;

import '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol';

contract Pausable {
    bool public paused;
    address public pauser;

    modifier isSystemPaused() {
        require(paused == true, 'P1');
        _;
    }

    modifier isSystemNotPaused() {
        require(paused == false, 'P1');
        _;
    }

    function pause() external {
        require(msg.sender == pauser, 'P2');
        paused = true;
    }

    function unpause() external {
        require(msg.sender == pauser, 'P2');
        paused = false;
    }

    function setPauser(address _pauser) internal {
        pauser = _pauser;
    }
}

GatewayV0.sol

// SPDX-License-Identifier: MIT
pragma solidity 0.8.6;

import '../Versioned.sol';
import '../Pausable.sol';
import '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol';

contract GatewayV0 is Versioned, Pausable {
    uint256 public test;

    function __GatewayV0__constructor(address _pauser) public initializer(0) {
        Pausable.setPauser(_pauser);
    }

    function inc() public {
        test += 1;
    }
}

and the deployment script

import { HardhatRuntimeEnvironment } from 'hardhat/types';
import { DeployFunction } from 'hardhat-deploy/types';
import { storeProxy } from '../../deploy_utils/storeProxy';
import { getProxy } from '../../deploy_utils/getProxy';
import { isLive } from '../../deploy_utils/isLive';

const deploy: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {

  const { deployments, getNamedAccounts, upgrades, ethers } = hre;
  const { deploy } = deployments;
  const { deployer } = await getNamedAccounts();

  const proxyAddress = getProxy('Gateway', hre);

  if (!proxyAddress) {
    const GatewayV0 = await ethers.getContractFactory('GatewayV0');

    const GatewayV0Proxy = await upgrades.deployProxy(GatewayV0, [process.env.OWNER || deployer], {initializer: '__GatewayV0__constructor'})

    console.log('deployed GatewayV0', GatewayV0Proxy.address);

    if (isLive(hre) && process.env.OWNER) {
      console.log('changing owner');
      const Admin = await upgrades.admin.getInstance();
      await Admin.changeProxyAdmin(GatewayV0Proxy.address, process.env.OWNER, {gasLimit: 1000000});
      console.log('changed owner');
    }

    storeProxy('Gateway', GatewayV0Proxy.address, hre);
  } else {
    console.log('skipped GatewayV0');
  }

};

module.exports = deploy;
module.exports.tags = ['Gateway']

Once added inside Defender, whenever I try to pause I get

Error: Transaction validation failed. This usually means that this proposal would fail to execute given its current parameters.
Make sure all involved accounts have the necessary permissions for the transaction to succeed.
Check pause's code to identify possible revert conditions.
You are proposing to execute function pause through address 0x809c4d1Eb772E72765A9440fbF5A0564E33D1bb9. Make sure that address has enough permissions to execute the function.

And I'm sure the Multisig is the pauser (TBD is the name of the multisig)
Screenshot 2021-07-27 at 15.18.07

Any insights on what might be happening inside Defender ? It's crucial for me to be able to run admin actions (upgradeToAndCall is not available otherwise) and pausing from the multisig.

2

Hi @mortimr, thanks for the detailed reproduction details. I'm at EOD here, but will go into them tomorrow and get back to you with any insights.

In the meantime, in case you want to do some troubleshooting before I get to this, you can try pausing the contract directly from the Gnosis Safe UI. If that succeeds we'll know for sure the issue is in Defender, otherwise we'll know for sure that it lies in contract state + code :).

Probably unrelated to your issue, but are you aware of https://docs.openzeppelin.com/contracts/4.x/api/security#Pausable? You might want to check it out instead of rolling out your own pause primitives. One of the advantages is tighter integration with Defender. For example, at its current version Defender won't be able to pick up whether your contract is paused or not because it doesn't know that the corresponding query function is called isSystemPaused.

Will get back to you with more details tomorrow.

3

Ok so I found out where the issue was coming from. The TransparentUpgradeProxy prevents the admin from running implementation calls

    function _beforeFallback() internal virtual override {
        require(msg.sender != _getAdmin(), "TransparentUpgradeableProxy: admin cannot fallback to proxy target");
        super._beforeFallback();
    }

This means that the Multisig that I set as an Admin inside the Proxy (changing the ADMIN_SLOT) will not be able to pause the system as the pausing logic is in the Implementation and he's not allowed to call methods there.

To solve this issue I had to do 2 things:

  • Deploy my own TransparentUpgradeableProxy that is also in charge of pausing the system
  • Add the ability to read the paused storage slot in the implementation
  • Write to the proper manifest the implementation and proxy details (allowing me to use prepareUpgrade or upgradeProxy afterwards and run the storage layout checks)
  1. Custom TransparentUpgradeable(Pausable)Proxy
// SPDX-License-Identifier: MIT
pragma solidity 0.8.6;

import '@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol';

contract PausableProxy is TransparentUpgradeableProxy {
    bytes32 internal constant _PAUSED_SLOT = 0x8dea8703c3cf94703383ce38a9c894669dccd4ca8e65ddb43267aa0248711450;

    constructor(
        address _logic,
        address _admin,
        bytes memory _data
    ) payable TransparentUpgradeableProxy(_logic, _admin, _data) {}

    function isPaused() external view returns (bool) {
      return StorageSlot.getBooleanSlot(_PAUSED_SLOT).value;
    }

    function pause() ifAdmin external {
        StorageSlot.getBooleanSlot(_PAUSED_SLOT).value = true;
    }

    function unpause() ifAdmin external {
        StorageSlot.getBooleanSlot(_PAUSED_SLOT).value = false;
    }

}
  1. Create the Pausable contract to read from storage in the Implementation
// SPDX-License-Identifier: MIT
pragma solidity 0.8.6;

import '@openzeppelin/contracts/utils/StorageSlot.sol';

contract Pausable {
    bytes32 internal constant _PAUSED_SLOT = 0x8dea8703c3cf94703383ce38a9c894669dccd4ca8e65ddb43267aa0248711450;

    modifier whenPaused() {
        require(StorageSlot.getBooleanSlot(_PAUSED_SLOT).value == true, 'P1');
        _;
    }

    modifier whenNotPaused() {
        require(StorageSlot.getBooleanSlot(_PAUSED_SLOT).value == false, 'P1');
        _;
    }
}
  1. Write to the manifests

To deploy the implementation and add it to the OZ manifest:

import { deployImpl } from "@openzeppelin/hardhat-upgrades/dist/utils";
import { withValidationDefaults } from "@openzeppelin/upgrades-core";

await deployImpl(hre, ContractFactory, withValidationDefaults({}))

Once the proxy is deployed and I can retrieve its address and txHash:

import { Manifest } from "@openzeppelin/upgrades-core";

const { provider } = hre.network;
const manifest = await Manifest.forNetwork(provider);
manifest.addProxy({
  kind: 'transparent',
  address: PausableProxyReceipt.address,
  txHash: PausableProxyReceipt.transactionHash
});

and it should do the trick !

A simple solution to avoid doing all this is to add the ability to use a custom proxy implementation. In my case, it would work seemlessly as I simply extend the proxy used by OZ without extra arguments.

1 Like
4

But, after all, it's not a problem. This is a regular feature of working with a proxy...

The administrator can manage, but cannot use.

The user can use, but cannot administer.

This should hardly continue to be considered a problem.

(Added a comment because it feels like this behavior of proxy contract code is higher than colleagues and other guys, including the starter topic, are considered a bug. Heh, and that's not a bug. It's a feature;)))

Но, ведь, это не является проблемой. Это штатная особенность работы с прокси…

Ru:

Администратор может управлять, но не может пользоваться.
Пользователь может пользоваться, но не может администрировать.

Это, вряд ли, стоит продолжать считать проблемой.

(Добавил комментарий, потому что складывается ощущения, что это поведение кода прокси-контрактов выше коллеги и прочие ребята, включая топик стартера считают багом. Хех, а это не баг. Это фича;)))