Trying to build a Factory of Upgradeable smart contracts

Smart Contracts Review Wanted
,
#1

Hi, I'm trying to create an upgradeable factory contract that can create proxies of ERC20 tokens.

I have the Factory contract that is:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/CountersUpgradeable.sol";
import "./TokenProxyR.sol";
import "./TokenR.sol";

contract TokenFactoryR is Initializable, OwnableUpgradeable {
    using CountersUpgradeable for CountersUpgradeable.Counter;

    mapping(uint256 => address) private addressesProxies;
    CountersUpgradeable.Counter private counter;
    string private version;

    function initialize(string memory _version) public initializer {
        _transferOwnership(tx.origin);
        version = _version;
    }

    function versionContract() public view returns (string memory) {
        return version;
    }

    function createProxyContract(string memory _name, string memory _symbol) public onlyOwner {
        TokenR token = new TokenR();
        TokenProxyR newProxy = new TokenProxyR(address(token), abi.encodeWithSelector(TokenR(address(0)).initialize.selector, _name, _symbol));
        uint256 currentCounter = counter.current();
        addressesProxies[currentCounter] = address(newProxy);
        counter.increment();
    }

    function getProxyAddress(uint256 id) public view returns(address) {
        return addressesProxies[id];
    }

    function getCurrentCounter() public view returns(uint256) {
        return counter.current();
    }
}

This factory creates Proxies:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;
import "@openzeppelin/contracts/proxy/Proxy.sol";
import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

contract TokenProxyR is ERC1967Proxy {
    uint32 private version;
    
    constructor(address _logic, bytes memory _data) ERC1967Proxy(_logic, _data) {}

    function versionProxy() public view returns(uint32) {
        return version;
    }

    function upgradeTo(address implementation) public {
        super._upgradeTo(implementation);
    }
}

This proxy creates forwards the functions calls to this contract here:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;
import "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20BurnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import "@openzeppelin/contracts-upgradeable/utils/math/SafeMathUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/cryptography/ECDSAUpgradeable.sol";

contract TokenR is
    Initializable,
    ERC20Upgradeable,
    ERC20BurnableUpgradeable,
    PausableUpgradeable,
    OwnableUpgradeable
{
    //This function gets called by the TokenFactory contract
    function initialize(string memory _name, string memory _symbol)
        external
        initializer
    {
        __ERC20_init(_name, _symbol);
        __ERC20Burnable_init();
        __Pausable_init();
        __Ownable_init();
    }
}

Is this correct?
I have tested throughly and all the tests pass successfully.
But I was thinking: is this practice correct?
So can an upgradeable contract (Factory) create Proxies in this way (these contracts don't use the "initialize" function but a ERC1967 constructor)?
Thanks a bunch.

2 Likes
#2

It looks like the factory is generating proxies and implementation contracts with basic ERC1967Proxy pattern. Wondering why TransparentProxy + UUPS compliant implementation is not used? Would there be any difference?

#3

I was studying how to create a Factory that can create upgradeable contracts and that could be upgradeable itself. So To achieve this behaviour I’ve started with basic stuff because I needed to “get my feet wet” on this topic (proxies) and it worked like a charm!
So maybe, the next thing I’ll do is upgrade this architecture so that it uses UUPS

#4

This is the link of the project that works

#5

Something here does not look right at all: your proxy is upgradeable by anyone!

By making this a public function with no access restrictions, anyone can upgrade your proxies.

You should use the proxy contracts that we provide, ideally without modification. You can use TransparentUpgradeableProxy, and if you use ERC1967Proxy you want to use the UUPS pattern that requires you to inherit UUPSUpgradeable in TokenR. You may find this presentation useful to understand these two alternatives:

One more thing that isn't wrong but is extremely inefficient is that you're deploying a new TokenR for each proxy and you don't need to. You can deploy it once and reuse the same address from then on.

Lastly, I would recommend you check out abi.encodeCall instead of abi.encodeWithSelector, and that you consider using an array with the native .push operation instead of a mapping along with a counter.

1 Like
#6

Hi frangio! Thanks for your feedback, I really appreciate it. Yes I've noticed that there are security problems with my implementation. That's because I was only testing the logic of the process to see if it could work out or I was doing something impossible. That worked though so I'm really happy with the results. Thanks again