Security advisory: Initialize UUPS implementation contracts

General Announcements
8

Depends. If your initializer assigned roles to msg.sender, then the role was assigned to the batch contract, which we selfdestructed after the operation. If it accepted the address of each role as a parameter, then it was assigned to a dummy contract with the following code, required to pass the initializer checks we found on the multiple contracts.

contract Fake is IERC165 {
    function name() public pure returns (string memory) { return "Nil"; }
    function symbol() public pure returns (string memory) { return "NIL"; }
    function decimals() public pure returns (uint) { return 18; }

    function lp() public view returns (address) { return address(this); }
    function token0() public view returns (address) { return address(this); }
    function token1() public view returns (address) { return address(this); }
    function rewardsToken() public view returns (address) { return address(this); }
    function WETH() public view returns (address) { return address(this); }
    function factory() public view returns (address) { return address(this); }

    function supportsInterface(bytes4 interfaceId) public pure override returns (bool) {
        return interfaceId != 0xffffffff;
    }
}
9

Great! Thanks for the quick reply!

1 Like
10

Hey guys, thank you for this information!, If we were to stay at 4.3.1, would be ensuring the initialization of the Implementation contracts be enough?, The increased bytecode size of patch 4.3.2 is troublesome on many of my contracts! :dizzy_face:.

11

As long as you ensure that no one can call the upgradeTo or upgradeToAndCall methods directly on your implementation contracts, you're good!

1 Like
12

You made it crystal clear to me, thanks a lot Santiago!

13

We've just released a post-mortem with more details about this vulnerability here: UUPSUpgradeable Vulnerability Post-mortem.

14

Am I correct that if you're still in development, there is this easy fix to add this to your implementation?

    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() initializer {} // <== ADDING THIS

    function initialize(
        // ... arguments
    ) public initializer {
        __ERC20_init(_name, _symbol);
        __Ownable_init();

        // ... your initializer logic
    }

Basically, this added constructor ensures that on implementation contract, the real initializer cannot be called. Thus, as the owner of the contract is non-initialized, no one can do anything meaningful with the contract

On the other hand, it's impossible to call constructor in proxied versions, so the real initializer is the only initializer that can be used

1 Like
15

Hey @caffeinum! That's correct, we have updated our documentation to reflect that:

Initializing the Implementation Contract

Do not leave an implementation contract uninitialized. An uninitialized implementation contract can be taken over by an attacker, which may impact the proxy. You can either invoke the initializer manually, or you can include a constructor to automatically mark it as initialized when it is deployed:

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() initializer {}

Just make sure that flagging the implementation as initialized is enough for preventing any calls to upgradeTo on the implementation contract. Or make sure you're using the latest version of OpenZeppelin Contracts which includes the fix to prevent direct upgradeTo calls to the implementation.

1 Like
16

Hi, so just to be clear. If using the latest version of OpenZeppelin do we still need to add this fix?

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() initializer {}

I see it's mentioned in the documentation and is still being generated by the Wizard.

17

Hey! While the latest version should protect against this vulnerability, we still recommend that you initialize implementation contracts for an extra layer of defense.

3 Likes
18

Thank you for the update. I just got a clue from this announcement.

19

They do not have to be values that match the proxies. In fact, it is better if they are gibberish values, so that the contract is left in an unusable state.

I am needing some fake wallet and contract addresses for this purpose. How to get one that isn't a security risk?

20

0x00..00 or 0x00..dead could be used.

21

Thank you @spalladino I understand that an implementation contract should be initialised. Meanwhile, does this specific line of code, constructor() initializer {}, need to be included in the Smart Contract even if the Contract is being initialised? I am not sure I fully understand the use of that specific line of code. Is calling the function function initialize() initializer public {...} sufficient? Finally, does version 0.8.0 protect against the vulnerability in question? Thank you again.

22

constructor() initializer {} is generally enough to fully initialize the contract. If you want to initialize it manually separately you will need to omit that line.

The latest versions of OpenZeppelin Contracts are not vulnerable to this vulnerability.

23

Thank you @frangio I have upgraded the OpenZeppelin Upgradeable Contracts to version 4.3.2. I am using the following in my Smart Contract, and do not have any Constructor in my Contract itself:

contract MyContract is Initializable, ERC721Upgradeable, OwnableUpgradeable, PausableUpgradeable, ReentrancyGuardUpgradeable, 
ERC721EnumerableUpgradeable, ERC721URIStorageUpgradeable {

    function initialize() initializer public {
        __ERC721_init("MyToken", "MYT");
        __ERC721Enumerable_init();
        __ERC721URIStorage_init();
        __Ownable_init();
        __Pausable_init();
        __ReentrancyGuard_init();
    }

...
}

However, when I include the line constructor() initializer {} I receive the following error, which appears to point to OpenZeppelin and Truffle modules:

../project:/contracts/MyContract.sol:15: Contract `MyContract` has a constructor
    Define an initializer instead
    https://zpl.in/upgrades/error-001
    at Object.assertUpgradeSafe (.../node_modules/@openzeppelin/upgrades-core/src/validate/query.ts:17:11)
    at Object.deployImpl (.../node_modules/@openzeppelin/truffle-upgrades/src/utils/deploy-impl.ts:33:3)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at deployProxy (.../node_modules/@openzeppelin/truffle-upgrades/src/deploy-proxy.ts:46:16)
    at module.exports (.../migrations/2_deploy_contract.js:9:3)
    at Migration._deploy (/usr/lib/node_modules/truffle/build/webpack:/packages/migrate/Migration.js:75:1)
    at Migration._load (/usr/lib/node_modules/truffle/build/webpack:/packages/migrate/Migration.js:56:1)
    at Migration.run (/usr/lib/node_modules/truffle/build/webpack:/packages/migrate/Migration.js:217:1)
    at Object.runMigrations (/usr/lib/node_modules/truffle/build/webpack:/packages/migrate/index.js:150:1)
    at Object.runFrom (/usr/lib/node_modules/truffle/build/webpack:/packages/migrate/index.js:110:1)
    at Object.runAll (/usr/lib/node_modules/truffle/build/webpack:/packages/migrate/index.js:114:1)
    at Object.run (/usr/lib/node_modules/truffle/build/webpack:/packages/migrate/index.js:79:1)
    at runMigrations (/usr/lib/node_modules/truffle/build/webpack:/packages/core/lib/commands/migrate.js:258:1)
    at Object.run (/usr/lib/node_modules/truffle/build/webpack:/packages/core/lib/commands/migrate.js:223:1)
    at Command.run (/usr/lib/node_modules/truffle/build/webpack:/packages/core/lib/command.js:183:1)
Truffle v5.4.13 (core: 5.4.13)
Node v14.18.1

If I do not include that line, the Contract deploys properly.

Thank you again. J

24

Hey @JF0001, can you try adding the following line immediately above the constructor?

/// @custom:oz-upgrades-unsafe-allow constructor

This will tell the upgrade plugins that you know what you're doing, and let you keep the constructor. The latest version of the wizard generates upgradeable contracts using this pattern.

2 Likes
25

Thank you @spalladino That works!

1 Like
26

Hello @spalladino, i have a question about add this. I had deployed a contract before see this post last week. My contract code didn't include this constructor() initializer method. I only had this

   function initialize() public initializer {
        __ERC20_init("SIMPLE", "SIMPLE");
        __ERC20Burnable_init();
        __Pausable_init();
        __AccessControl_init();
        __Ownable_init();
        __UUPSUpgradeable_init();

        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _grantRole(PAUSER_ROLE, msg.sender);
        _grantRole(MINTER_ROLE, msg.sender);
        _grantRole(UPGRADER_ROLE, msg.sender);
    }

I am current using this version openzeppelin/contracts-upgradeable": "^4.4.1
If I would need to add this constructor, it is possible that I add it on upgrade? Does it work if adding this by upgrade? Or I need to deploy a new contract instead?

27

@xfss_foo You can send a transaction to your implementation contract to invoke the initialize() function to make sure no one else can do it.

1 Like