Simulating a DELEGATECALL with Gnosis in Hardhat Mainnet Forking fails

Support
1

Hi, has anyone successfully simulated a DELEGATE CALL operation with Hardhat Mainnet forking in tests? I'm doing this so reduce the total number of transactions I need to do from the governance contract (basically like a multisend). I'm failing and I don't really understand why. I'm mostly following along with docs for Gnosis SDK from here. My code looks like this...

const tx = {
  to: migrator.address,
  value: "0",
  data: ethersMigrator.interface.encodeFunctionData("upgradeImplementations", [config, newImplementations]),
}

await delegateSafeTransaction(tx)

async function delegateSafeTransaction(tx) {
  const safeAddress = MAINNET_MULTISIG
  const blakesGovAddress = "0xf13eFa505444D09E176d83A4dfd50d10E399cFd5"
  const mikesGovAddress = "0x5E7b1B5d5B03558BA57410e5dc4DbFCA71C92B84"
  await impersonateAccount(hre, blakesGovAddress)
  await impersonateAccount(hre, mikesGovAddress)
  let ownerSigner = await ethers.getSigner(blakesGovAddress)
  let ownerSigner2 = await ethers.getSigner(mikesGovAddress)
  const ethAdapterOwner1 = new EthersAdapter({ethers, signer: ownerSigner})
  const ethAdapterOwner2 = new EthersAdapter({ethers, signer: ownerSigner2})
  const safeSdk = await Safe.create({ethAdapter: ethAdapterOwner1, safeAddress})
  const owner2 = await safeSdk.connect({ethAdapter: ethAdapterOwner2, safeAddress})
  tx.operation = 1
  const transactions = [tx]

  const safeTx = await safeSdk.createTransaction(...transactions)
  console.log("safe tx is:", safeTx)
  const txHash = await owner2.getTransactionHash(safeTx)
  const approveTx = await owner2.approveTransactionHash(txHash)
  await approveTx.transactionResponse.wait()
  const executeTxResponse = await safeSdk.executeTransaction(safeTx)
  return await executeTxResponse.transactionResponse.wait()
}

The relevant code from the migrator contract...

// SPDX-License-Identifier: MIT

pragma solidity 0.6.12;
pragma experimental ABIEncoderV2;

import "../core/BaseUpgradeablePausable.sol";
import "../core/ConfigHelper.sol";
import "../core/CreditLine.sol";
import "../core/GoldfinchConfig.sol";
import "../../interfaces/IBase.sol";
import "hardhat/console.sol";

contract V2Migrator is BaseUpgradeablePausable {
  bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
  bytes32 public constant GO_LISTER_ROLE = keccak256("GO_LISTER_ROLE");
  using SafeMath for uint256;

  GoldfinchConfig public config;
  using ConfigHelper for GoldfinchConfig;

  mapping(address => address) public borrowerContracts;

  function initialize(address owner, address _config) external initializer {
    require(owner != address(0) && _config != address(0), "Owner and config addresses cannot be empty");
    __BaseUpgradeablePausable__init(owner);
    config = GoldfinchConfig(_config);
  }

  function upgradeImplementations(GoldfinchConfig _config, address[] calldata newDeployments) public onlyAdmin {
    console.log("msg sender", msg.sender);
    console.log("address this", address(this));
    address poolAddress = newDeployments[0];
    address creditDeskAddress = newDeployments[1];
    address fiduAddress = newDeployments[2];
    address goldfinchFactoryAddress = newDeployments[3];

    bytes calldata data;
    console.log("proxy address is:", _config.poolAddress());
    console.log("pool implementation address is", poolAddress);
    IBase(_config.poolAddress()).changeImplementation(poolAddress, data);
    console.log("It appears to have worked", poolAddress);

    IBase(_config.creditDeskAddress()).changeImplementation(creditDeskAddress, data);
    IBase(_config.fiduAddress()).changeImplementation(fiduAddress, data);
    IBase(_config.goldfinchFactoryAddress()).changeImplementation(goldfinchFactoryAddress, data);
  }

}

And here are some useful logs...

governance contract is: 0xBEb28978B2c755155f20fd3d09Cb37e300A6981f
new implementation is: 0x0f5D1ef48f12b6f691401bfe88c2037c690a6afe
msg sender 0xbeb28978b2c755155f20fd3d09cb37e300a6981f
address this 0x59f2f1fcfe2474fd5f0b9ba1e73ca90b143eb8d0
proxy address is: 0xb01b315e32d1d9b5ce93e296d483e1f0aad39e75
pool implementation address is 0x0f5d1ef48f12b6f691401bfe88c2037c690a6afe

safe tx is: EthSafeTransaction {
  signatures: Map {},
  data: {
    to: '0x59F2f1fCfE2474fD5F0b9BA1E73ca90b143Eb8d0',
    value: '0',
    data: '0x1b7e0e8a0000000000000000000000007daa6477194b784d384e79333230daec3b32a65e000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000040000000000000000000000000f5d1ef48f12b6f691401bfe88c2037c690a6afe00000000000000000000000090118d110b07abb82ba8980d1c5cc96eea810d2c000000000000000000000000ca03dc4665a8c3603cb4fd5ce71af9649dc00d440000000000000000000000002de080e97b0cae9825375d31f5d0ed5751fdf16d',
    operation: 1,
    baseGas: 0,
    gasPrice: 0,
    gasToken: '0x0000000000000000000000000000000000000000',
    refundReceiver: '0x0000000000000000000000000000000000000000',
    nonce: 40,
    safeTxGas: 0
  }
}

execution result is: {
  to: '0xBEb28978B2c755155f20fd3d09Cb37e300A6981f',
  from: '0xf13eFa505444D09E176d83A4dfd50d10E399cFd5',
  contractAddress: null,
  transactionIndex: 0,
  gasUsed: BigNumber { _hex: '0xbfb1', _isBigNumber: true },
  logsBloom: '0x00000000400000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000800000000000000200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000',
  blockHash: '0x374e8c16d52567f2d93b729d6c72a10f3f9a92f96ea53d461bcc794f86205459',
  transactionHash: '0xac122c6077df323a0769a669ea6333c7e2c0ed67b00621bc44d83028d6aa93f9',
  logs: [
    {
      transactionIndex: 0,
      blockNumber: 12896337,
      transactionHash: '0xac122c6077df323a0769a669ea6333c7e2c0ed67b00621bc44d83028d6aa93f9',
      address: '0xBEb28978B2c755155f20fd3d09Cb37e300A6981f',
      topics: [Array],
      data: '0x62d25ac1a3d8db126aa75594cca3e3fa60929f815c302d48293853d767887d3d0000000000000000000000000000000000000000000000000000000000000000',
      logIndex: 0,
      blockHash: '0x374e8c16d52567f2d93b729d6c72a10f3f9a92f96ea53d461bcc794f86205459'
    }
  ],
  blockNumber: 12896337,
  confirmations: 1,
  cumulativeGasUsed: BigNumber { _hex: '0xbfb1', _isBigNumber: true },
  status: 1,
  byzantium: true,
  events: [
    {
      transactionIndex: 0,
      blockNumber: 12896337,
      transactionHash: '0xac122c6077df323a0769a669ea6333c7e2c0ed67b00621bc44d83028d6aa93f9',
      address: '0xBEb28978B2c755155f20fd3d09Cb37e300A6981f',
      topics: [Array],
      data: '0x62d25ac1a3d8db126aa75594cca3e3fa60929f815c302d48293853d767887d3d0000000000000000000000000000000000000000000000000000000000000000',
      logIndex: 0,
      blockHash: '0x374e8c16d52567f2d93b729d6c72a10f3f9a92f96ea53d461bcc794f86205459',
      args: [Array],
      decode: [Function],
      event: 'ExecutionSuccess',
      eventSignature: 'ExecutionSuccess(bytes32,uint256)',
      removeListener: [Function],
      getBlock: [Function],
      getTransaction: [Function],
      getTransactionReceipt: [Function]
    }
  ]
}

So you can see a few things here.

  1. The only event is "ExecutionSuccess" from the multisig
  2. The console logs show that we do not even make it past the actual attempt to change the implementation (the log for "it appears to have worked never shows up")
  3. The msg.sender while in the "upgradeImplementations" method is in fact the governance contract. But "address(this)" is still the original contract. If it was actually a delegate call, I would expect "address(this)" to be the governance contract.

Also worth noting, if I change the "operation" on the multisig TX to be 0, then the 'msg.sender' and 'address(this)' values remain identical, and I receive an expected error of "NOT_AUTHORIZED". So my best guess right now is that DELEGATE_CALL is not really working, but the multisig is somehow swallowing the error when I do it? Any ideas on this? Any help would be greatly appreciated! Thanks!

:computer: Environment

I'm using a mac with latest Hardhat and Ethers/Truffle.

2

Hey @blakewest, can you share the code of the ethersMigrator, as well as the events you're getting out of the executeTxResponse and the logs you get in the test run? Thanks!

3

@spalladino question updated with more info! Thanks so much for any help!

4

Note that if it were actually a delegatecall, then msg.sender should not be the governance contract, but whoever called the governance contract (in this case, 0xf13eFa505444D09E176d83A4dfd50d10E399cFd5). IMO this is a regular call, and not a delegatecall.

What's surprising to me is that you're seeing an ExecutionSuccess (and not ExecutionFailure) on the tx, but it is not logging the It appears to have worked event.

Out of curiosity: if you check the implementation addresses of your contracts after the tx is run, do they get updated or not?

Can you confirm this revert reason is only on your IBase contract?

5

@spalladino ...

Note that if it were actually a delegatecall, then msg.sender should not be the governance contract, but whoever called the governance contract

Yes, 100%. That's why I was saying "if it was actually a delegate call, I would expect address(this) to be the governance contract". Perhaps I worded my surprise confusingly. But I agree with your expectations.

Out of curiosity: if you check the implementation addresses of your contracts after the tx is run, do they get updated or not?

No they do not. I tried checking the implementation address (by pulling from the storage location on the proxy), and it did not update. Also, there is a later call in the migration steps that requires the update to take place, and that call fails with "unrecognized function selector". If I make the upgrades via a regular call, then the whole migration works just fine in my tests.

Can you confirm this revert reason is only on your IBase contract?

I can't truly confirm this, because I'm in mainnet forking mode, and thus can't drop a console log into the already deployed proxy contract. However, I know we used hardhat-deploy to deploy the original, and you can see in their proxy source code here that the onlyOwner modifier reverts with exactly the same "NOT_AUTHORIZED" error message, so I think it's a pretty good bet. (granted, the thing I linked to is actually a slightly newer version of the proxy, but I think the error message likely is the same as the one I deployed).

Without the ability to test locally, I can't really use this approach, which sucks. Because it otherwise seems awesome.

I wonder if this is a hardhat mainnet forking bug? Or any other ideas?

6

Can you upload this to a github repo I can clone and run locally?

7

I'll just give you access to our repo. It's easier that way. Will email you.

8

Hey @blake! After some debugging, I think the problem is related to the deployment of the V2Migrator contract, since you deploy it as a proxy. If you have the Safe (or any other contract) delegatecalling into a proxy, then they will execute the code of the proxy using their own state: this means that they will forward the call to whatever the Safe (not the proxy!) have stored in the IMPLEMENTATION_SLOT, which, in the case of the Safe, is zero.

Can you try deploying the migrator as a regular contract instead? Also, you may need to remove the onlyAdmin modifier from the public methods of the migrator, since the msg.sender is your EOA and not the Safe (if you run a delegatecall).

9

Hot damn @spalladino you are correct! That did it. Thanks so much. Amazing work!

10

@spalladino Just wanted to add a post-script here. After some playing around, I ran into more bugs, and for time reasons, I have to give up on a lot of what I wanted to do here. I was able to successfully upgrade our implementations using this approach. However, I also wanted to grant some roles to the migrator contracts, and this brought in all kinds of weird errors. After adding some console logs, I noticed that the console log in my delegated function call ran 3 times... but it would still succeed and my tests passed. If I added other state changing functions though, then the execution of the delegate call would fail. And the console logs got weirder. Here was my function (notice I add a call to deposit at the end to test what happens. Without the deposit call, this function works perfectly)...

  function upgradeImplementations(GoldfinchConfig _config, address[] calldata newDeployments) public {
    console.log("Upgrading implementations in the contract...");
    console.log("Address this:", address(this));
    address newPoolAddress = newDeployments[0];
    address newCreditDeskAddress = newDeployments[1];
    address newFiduAddress = newDeployments[2];
    address newGoldfinchFactoryAddress = newDeployments[3];

    bytes memory data;
    IBase pool = IBase(_config.poolAddress());
    IBase creditDesk = IBase(_config.creditDeskAddress());
    IBase fidu = IBase(_config.fiduAddress());
    IBase goldfinchFactory = IBase(_config.goldfinchFactoryAddress());

    // Upgrade implementations
    pool.changeImplementation(newPoolAddress, data);
    creditDesk.changeImplementation(newCreditDeskAddress, data);
    fidu.changeImplementation(newFiduAddress, data);
    goldfinchFactory.changeImplementation(newGoldfinchFactoryAddress, data);

    console.log("Depositing...");
    pool.deposit(1);
    console.log("Deposited...");
  }

And then here were the console logs...

Upgrading implementations in the contract...
Address this: 0xbeb28978b2c755155f20fd3d09cb37e300a6981f
Depositing...
Upgrading implementations in the contract...
Address this: 0xbcf26943c0197d2ee0e5d05c716be60cc2761508
Upgrading implementations in the contract...
Address this: 0xbeb28978b2c755155f20fd3d09cb37e300a6981f
Depositing...

Check out what's going on there. For one, it appears to be running 3 times, which is incredibly strange (could be a hardhat or testing framework issue though, so I'm mostly ignoring this). I'm certain it's not my code. Secondly, none of the deposits appear to work (as we never see the "deposited" console log). Third... the "address(this)" actually changes on the second run!

This was especially confusing since I'm successfully taking action on the very same contracts just a few lines above.

Eventually, it stood out to me that all the contracts for which I want to do things are deployed as proxies. And the "changeImplementation" functions (which are successful) are on the proxy itself. But the other functions I tested with (deposit and grantRole) are on the implementations of those proxies. Which is very suspicious, considering the previous bug where delegate calling into a proxy caused the issue.

I'm wondering.. how far does DELEGATECALL go? Is it possible that when calling deposit or grantRole, that I'm actually still using the storage of the Safe? Or do you have any ideas here?

Thanks! - Blake

11

Hey @blakewest! There are a few things to unpack here. First of all: the Safe SDK, in order to properly estimate the gas for your call, will run a few eth_call and eth_estimateGas actions which may trigger hardhat's console.logs several times. Also, the different contexts in which is called could amount for the differences in the "Address this" logs. For debugging, I'd suggest emitting a custom event, and inspecting the logs by logging the tx receipt in js.

As for the deposit call failing, this could be caused by one of the following: the newPoolAddress corresponds to an implementation that does not properly implement deposit, or the Safe does not have 1 token to pay for the deposit, or the Safe hadn't previously approved the token to the pool so it could be transferFrom'd.

Delegate call can be difficult to wrap your head around, but think of it this way: a delegate call means "load the code from this address and run it". This means that, when you create the Safe tx with operation=delegatecall, you are telling the Safe to get the code from your migrator contract and execute it. This is why it was failing when you were pointing the Safe to a proxy to the migrator: because the code being loaded was that of a proxy, not of the migrator implementation.

Now, when you're calling (as in a regular call) from the migrator (well, actually would be from the Safe using the code of the migrator, because you delegatecall into it!) to another contract in your system to execute a changeImplementation, a deposit, or a grantRole you should always interact with the proxy, because it is the proxy who holds the state of your contract. As a rule of thumb: you delegatecall into implementation contracts, but call into proxies.

Just to make sure we're on the same page: when you run pool.deposit(1) from your migrator (again, it's actually from the Safe but using the code of the migrator), you're interacting with the pool proxy contract as usual. The proxy then loads the code from the new implementation contract, but that should be transparent to you, since you're issuing a regular call.

Hope this helps!

12

@spalladino ahh, great call on the eth_call and eth_estimateGas actions. That is a pretty reasonable explanation for the multiple console.log and different address(this) observations.

Also, yeah those are fair guesses on the deposit action failing. May be true, I'd have to investigate. But really though, it's the grantRole that is still a mystery. The way you explained delegatecall is how I thought it worked, which is why I'm stumped that it fails. In my code, I can do pool.grantRole(OWNER_ROLE, migrator.address, {from: MAINNET_MULTISIG}), and it works just fine. But when I do delegateFromMultisig(grantPermsTx), it fails.

I thought maybe it was running out of gas, so I tried creating a separate function that only did the one grantRole call, but that also failed. It's so frustrating that you can't get a real error message here. I'm working with Tenderly to see about debugging local transactions. Maybe that will shed some light on it. If it does, I'll report back. For now, I'm just gonna "do it the dumb way", and I'm printing out the args I need and manually inputting a multi-call using the Gnosis Safe UI. Oh well.

We'll figure it out for the next upgrade. Thank you SOOOO much for all your help here. It's been very useful, and I've already learned a lot!

1 Like