Issues initializing EIP-1167 minimal proxies deployed with factory contract

Support
1

Hey everyone! So I’m trying to create a Forwarder contract that converts all received ETH/tokens to Chai and sends the Chai to a user’s address. The address that Chai gets forwarded to is the owner of the Forwarder contract, so each user should have their own Forwarder contract. As a result, I figured I’d deploy each contract as a minimal proxy to reduce gas costs. I’d also like to be able to upgrade the underlying logic of the Forwarder contract, and, since the proxy contracts delegate calls to this contract, my understanding is all proxy contracts would consequently be updated by simply updating the logic contract.

I also want to keep track of all deployed contracts on chain, so am using a factory contract to do this. The problem I’m having is that when the factory deploys the proxy in the createForwarder() function, I can’t seem to call the initialize() function of the Forwarder proxy contract afterwards in the same transaction. (It works as a separate transaction sent from JS, but this opens up the possibility of front-running).

There’s two approaches I’ve tried, and they both fail differently. The ForwarderFactory code below has comments explaining exactly how each approach fails. I’m running the tests with Truffle (v5.1.3) and ganache-cli (ganache-cli v6.6.0, ganache-core v2.7.0) on a forked mainnet.

Any help is appreciated!

The truffle test is as follows:

const Factory = artifacts.require('ForwarderFactory');
const Forwarder = artifacts.require('Forwarder');

beforeEach(async () => {
  // Deploy Forwarder logic template
  // Here, admin refers to our server
  ForwarderInstance = await Forwarder.new({ from: admin });
  forwarder = ForwarderInstance.address;
  // ***QUESTION***: should the proxy's logic template be initialized as done here?
  // Whether initialized or not, I have the same problem
  await ForwarderInstance.methods['initialize(address,address)'](admin, admin, { from: admin });

  // Deploy factory
  FactoryInstance = await Factory.new({ from: admin });
  factory = FactoryInstance.address;
  await FactoryInstance.methods['initialize()']({ from: admin });
});

it('deploys a clone and updates the state', async () => {
  // Create forwarder (alice is a user)
  await FactoryInstance.createForwarder(forwarder, alice, { from: admin, gas: '6000000' });

  // Confirm state was updated and that factory is a clone
  const forwarders = await FactoryInstance.getForwarders(); // get array of all forwarders
  const users = await FactoryInstance.getUsers(); // get array of all users
  expect(forwarders).to.be.an('array').with.lengthOf(1); // passes
  expect(users).to.be.an('array').with.lengthOf(1); // passes
  expect(users[0]).to.equal(alice); // passes
  const isClone = await FactoryInstance.isClone(forwarder, forwarders[0], { from: admin });
  expect(isClone).to.be.true; // passes

  // Create instance of deployed factory proxy and check the owner and version
  const ForwarderProxy = await new web3.eth.Contract(forwarderAbi, forwarders[0]);
  const owner = await ForwarderProxy.methods.owner().call();
  const version = await ForwarderProxy.methods.version().call();
  expect(owner).to.equal(users[0]); // fails, owner is still the zero address
  expect(version).to.equal('1'); // fails, version is still zero

  // The below commented out line does successfully initialize the contract,
  // but opens us up to front-running since it's a separate transaction
  // await ForwarderProxy.methods.initialize(alice, admin).send({ from: admin });
});

Here are the relevant parts of the Forwarder contract:

import "@openzeppelin/contracts-ethereum-package/contracts/ownership/Ownable.sol";
import "@openzeppelin/upgrades/contracts/Initializable.sol";

contract Forwarder is Initializable, Ownable {

  address public admin;
  uint256 public version;

  event Initialized(address indexed thisAddress);

  function initialize(address _recipient, address _admin) public initializer {
    emit Initialized(address(this));
    Ownable.initialize(_recipient);
    admin = _admin;
    version = 1;

  // other functions omitted for brevity...
  // there is a fallback function in case that helps
}

And here are the relevant parts of the Factory contract:

import "@openzeppelin/contracts-ethereum-package/contracts/ownership/Ownable.sol";
import "@openzeppelin/upgrades/contracts/Initializable.sol";
import "./Forwarder.sol";

contract ForwarderFactory is Initializable, Ownable {

  address[] public forwarders;
  address[] public users;
  mapping (address => address) public getForwarder; // maps user => forwarder

  event ForwarderCreated(address indexed _address);

  function initialize() public initializer {
    Ownable.initialize(msg.sender);
  }

  function createForwarder(address _target, address _user) external {
    // Deploy proxy
    address _forwarder = createClone(_target);
    emit ForwarderCreated(_forwarder);

    // Update state
    forwarders.push(_forwarder);
    users.push(_user);
    getForwarder[_user] = _forwarder;

    // Initialize the new contract
    // There are two approaches shown below which both do not work. Here
    // they are both not commented out so it's easier to read, but of course
    // when testing only one was used at a time
    address _admin = owner();

    // APPROACH 1
    // - The transaction completes, but tests show the new contract wasn't 
    //   actually initialized, as the owner is the zero address and the version 
    //   is still set to the default of zero. 
    // - The factory's `ForwarderCreated` event is emitted, but the forwarder's
    //   `Initialized` event is not emitted, which makes me think I'm calling the 
    //   initialization function wrong
    bytes memory _payload = abi.encodeWithSignature("initialize(address,address)", _user, _admin);
    (bool success,) = _forwarder.call(_payload);
    require(success, "forwarderFactory/forwarder-proxy-initialization-failed");

    // APPROACH 2
    // - This fails with "VM Exception while processing transaction: revert"
    // - If using `_forwarder` instead of `_forwarderPayable` to call the initialize
    //   function, compilation fails with "TypeError: Explicit type conversion not
    //   allowed from non-payable "address" to "contract Forwarder", which has
    //   a payable fallback function."
    address payable _forwarderPayable = address(bytes20(_forwarder));
    Forwarder(_forwarderPayable).initialize(_user, _admin);
  }

  function createClone(address target) internal returns (address result) {
    // source: https://github.com/optionality/clone-factory/blob/master/contracts/CloneFactory.sol
    bytes20 targetBytes = bytes20(target);
    assembly {
      let clone := mload(0x40)
      mstore(clone, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000)
      mstore(add(clone, 0x14), targetBytes)
      mstore(add(clone, 0x28), 0x5af43d82803e903d91602b57fd5bf30000000000000000000000000000000000)
      result := create(0, clone, 0x37)
    }
  }

  // other functions omitted for brevity...
  // no fallback function here
2 Likes
2

Hi @msolomon4,

My understanding is that minimal proxies are not upgradeable. We can change the logic contract that future minimal proxies point to, but can't change existing minimal proxies.

I suggest reading (if you haven't already): Deep dive into the Minimal Proxy contract

You may want to consider using OpenZeppelin's ProxyFactory to deploy minimal proxies:

What I am not sure of is how to encode the initialization data in Solidity, hopefully someone in the community can provide this answer.

3

Hey @abcoathup, thanks for the response!

That is my understanding as well—that the proxy itself cannot be upgraded, but the logic contract can.

Yes, that article was very helpful!

The createForwarder() function I'm using was based off of the deployMinimal() function in ProxyFactory (and that function seems to have been based on CloneFactory). The only difference with mine is that I've effectively hardcoded the parameters to the .call() method, whereas the ProxyFactory function takes them as an input.

It's possible I'm passing the inputs to .call() in wrong, but if so then it's odd the function fails silently in that case.

For what it's worth, I also just tried deploying a full contract from the factory instead of a minimal proxy, and get the same failures as described above. So it seems the problem isn't related to the fact that the contract is a proxy contract.

1 Like
4

Hi @msolomon4,

Using OpenZeppelin ProxyFactory I can create a Forwarder from the ForwarderFactory.
Have a look at the contracts and test below.

Forwarder.sol

pragma solidity ^0.5.0;

import "@openzeppelin/contracts-ethereum-package/contracts/ownership/Ownable.sol";
import "@openzeppelin/upgrades/contracts/Initializable.sol";

contract Forwarder is Initializable, Ownable {

  address public admin;
  uint256 public version;

  event Initialized(address indexed thisAddress);

  function initialize(address _recipient, address _admin) public initializer {
    emit Initialized(address(this));
    Ownable.initialize(_recipient);
    admin = _admin;
    version = 1;
  }
}

ForwarderFactory.sol

pragma solidity ^0.5.0;

import "@openzeppelin/upgrades/contracts/upgradeability/ProxyFactory.sol";

import "./Forwarder.sol";

contract ForwarderFactory is ProxyFactory {

    address[] public forwarders;
    address[] public users;
    mapping (address => address) public getForwarder; // maps user => forwarder

    event ForwarderCreated(address indexed _address);

    function createForwarder(address _target, address _user) external {

        address _admin = msg.sender;
        bytes memory _payload = abi.encodeWithSignature("initialize(address,address)", _user, _admin);

        // Deploy proxy
        address _forwarder = deployMinimal(_target, _payload);
        emit ForwarderCreated(_forwarder);

        // Update state
        forwarders.push(_forwarder);
        users.push(_user);
        getForwarder[_user] = _forwarder;
    }
}

ForwarderFactory.test.js

const { accounts, contract } = require('@openzeppelin/test-environment');

const {
    BN,           // Big Number support
    constants,    // Common constants, like the zero address and largest integers
    expectEvent,  // Assertions for emitted events
    expectRevert, // Assertions for transactions that should fail
  } = require('@openzeppelin/test-helpers');

const [ creator, user ] = accounts;

const { expect } = require('chai');

const Forwarder = contract.fromArtifact('Forwarder');
const ForwarderFactory = contract.fromArtifact('ForwarderFactory'); 

describe('ForwarderFactory', function () {
  it('creates proxy', async function () {
    const factory = await ForwarderFactory.new();
    const logic = await Forwarder.new();

    await factory.createForwarder(logic.address, user);

    forwarderAddress = await factory.forwarders(0);
    forwarder = await Forwarder.at(forwarderAddress);
    expect(await forwarder.version()).to.be.bignumber.equal("1");
  });
});
5

Hmm, and that test passed for you @abcoathup? I’ve tried replicating your setup above but it hasn’t changed anything. The only difference I can think of is that I’m using Truffle instead of the @openzeppelin/test-enviornment to leverage additional ganache-cli functionality. As a result my test looks like this:

contract('Factory', async (accounts) => {
  const alice = accounts[0]; // alice is the user
  const admin = process.env.ADMIN_ADDRESS; // server address with ETH
  let forwarder; // address of alice's forwarder contract
  let factory; // address of fowarder factory contract

  it('creates and initializes proxy', async () => {
    FactoryInstance = await Factory.new({ from: admin });
    ForwarderInstance = await Forwarder.new({ from: admin });
    factory = FactoryInstance.address;
    forwarder = ForwarderInstance.address;

    const { logs } = await FactoryInstance.createForwarder(forwarder, alice, { from: admin, gas: '6000000' });
    
    const forwarderAddresses = await FactoryInstance.getForwarders();
    const forwarderAddress = forwarderAddresses[0];

    await expectEvent.inLogs(logs, 'ProxyCreated', { proxy: forwarderAddress });
    await expectEvent.inLogs(logs, 'ForwarderCreated', { _address: forwarderAddress });

    const forwarderAbi = require('../build/contracts/Forwarder.json').abi; // from `npx oz compile`
    const ForwarderProxy = await new web3.eth.Contract(forwarderAbi, forwarderAddress);
    const version = await ForwarderProxy.methods.version().call();

    // Below test fails, as version is still zero.
    // Initialized event is also never emitted (checked manually for now using Truffle's ---show-events flag)
    expect(version).to.equal('1'); 
  });
});

I’ve also tried renaming and simplifying the initialize function as well to see if something odd was going on with the initializer modifier, etc. So I tested it out by updating the factory to call this new function

function testFunction() public {
  version = 1;
  emit Initialized(address(this));
 }

using bytes memory _payload = abi.encodeWithSignature("tomato()"); as the input for deployMinimal().

But the version was still not updated and the event not emitted.

I then threw some extra events into my local version of the deployMinimal() function of ProxyFactory to make sure the .call() was working properly. Turns out it is, as the value returned by (bool success,) = proxy.call(_data); is always true.

But we know it’s not calling the function, since the Initialized event is not emitted. So my thinking was that maybe the call data was not properly formatted. But…

For the next debugging test, I changed Forwarder.sol to look like this:

pragma solidity ^0.5.0;

import "@openzeppelin/contracts-ethereum-package/contracts/ownership/Ownable.sol";
import "@openzeppelin/upgrades/contracts/Initializable.sol";

contract Forwarder is Initializable, Ownable {

  address public admin;
  uint256 public version = 1; // CHANGE: originally was `uint256 public version;`

  event Initialized(address indexed thisAddress);

  function initialize(address _recipient, address _admin) public initializer {
    emit Initialized(address(this));
    Ownable.initialize(_recipient);
    admin = _admin;
    version = 2; // CHANGE: originally was version = 1
  }
}

Before tests would fail with expected '0' to equal '1', so here they should fail with expected '1' to equal '2'.

Instead, they fail with expected '0' to equal '2'…

Does this suggest there are two problems? The first being that the call data is not properly formatted, and the second being that somehow I’m calling the wrong contract, even though the expectEvent tests above pass?

1 Like
6

Hi @msolomon4,

The test passes. I converted to a Truffle Test (though I am a convert of OpenZeppelin Test Environment as it is fast) and it still passes.

You may want to setup a new project with the cut down versions of the contracts I provided above to get it working.

//const { accounts, contract } = require('@openzeppelin/test-environment');

const {
    BN,           // Big Number support
    constants,    // Common constants, like the zero address and largest integers
    expectEvent,  // Assertions for emitted events
    expectRevert, // Assertions for transactions that should fail
  } = require('@openzeppelin/test-helpers');

//const [ creator, user ] = accounts;

const { expect } = require('chai');

//const Forwarder = contract.fromArtifact('Forwarder');
//const ForwarderFactory = contract.fromArtifact('ForwarderFactory'); 
const Forwarder = artifacts.require('Forwarder');
const ForwarderFactory = artifacts.require('ForwarderFactory'); 

//describe('ForwarderFactory', function () {
contract('ForwarderFactory', function ([creator, user]) {
  it('creates proxy', async function () {
    const factory = await ForwarderFactory.new();
    const logic = await Forwarder.new();

    await factory.createForwarder(logic.address, user);

    forwarderAddress = await factory.forwarders(0);
    forwarder = await Forwarder.at(forwarderAddress);
    expect(await forwarder.version()).to.be.bignumber.equal("1");
  });
});
7

It works! Thanks a bunch @abcoathup :grinning:

Seems the culprit was using a forked mainnet to run the tests. When using an ordinary ganache instance, the test passes for both (1) a new project using your code and (2) the original project. With a forked mainnet, both fail.

I sure do love the fork feature, but it can be a bit buggy sometimes. I’ll go ahead and file an issue with the ganache team.

Thanks again for the help!

2 Likes
8

Hi @msolomon4,

Lesson learnt for me too, I didn’t run your code as is.

Though I would encourage using OpenZeppelin’s ProxyFactory if appropriate. Same too for OpenZeppelin Test Environment.

9

Yep, I’ll definitely be sticking with ProxyFactory since it’s working well. And surely a bit safer than manually pulling out the one function I need :slight_smile:

I do want to use OpenZeppelin’s Test Environment, though am just waiting for support of a few of the ganache-cli parameters listed in this issue.

1 Like
10

Just created the issue here, in case anyone was interested in following it.

1 Like
11

A post was split to a new topic: How does the proxy created by ProxyFactory work?