MerkleMinter created through TokenFactory cannot be upgraded

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/TokenFactory.sol#L119-L125

Vulnerability details

Impact

During the token creation process in the TokenFactory contract, the function creates a MerkleMinter contract to setup and handle token initial token distribution.

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/framework/utils/TokenFactory.sol#L119-L125

...

// Clone and initialize a `MerkleMinter`
address merkleMinter = merkleMinterBase.clone();
MerkleMinter(merkleMinter).initialize(
    _managingDao,
    IERC20MintableUpgradeable(token),
    distributorBase
);

...

The MerkleMinter contract is an upgradeable contract, as it inherits from PluginUUPSUpgradeable:

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/plugins/token/MerkleMinter.sol#L20

contract MerkleMinter is IMerkleMinter, PluginUUPSUpgradeable {

However, as we can see in the first code snippet, the MerkleMinter instance created in createToken is a cloned instance (using OpenZeppelin Clones library). This is incompatible with upgradeable contracts, which require the use of a proxy.

This issue will cause the MerkleMinter instance created through TokenFactory to fail to be upgraded. The MerkleMinter contract will contain all the required logic to be upgraded, but the action will fail as there is no proxy to change to a new potential implementation.

Proof of Concept

The following test illustrates the issue. We call createToken to get an instance of MerkleMinter. We then simulate a new version of the contract to upgrade to (merkleMinterV2Impl) and try to upgrade the MerkleMinter instance to this new implementation. The call fails with a "Function must be called through active proxy" error (error is defined in OpenZeppelin base UUPSUpgradeable contract).

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_TokenFactory_createToken_MerkleMinterNotUpgradeable() public {
    DAO dao = createDao();
    TokenFactory tokenFactory = new TokenFactory();
    grantRootPermission(dao, address(tokenFactory));

    TokenFactory.TokenConfig memory tokenConfig = TokenFactory.TokenConfig({
        addr: address(0),
        name: "DAO Token",
        symbol: "DAOT"
    });

    address[] memory receivers = new address[](0);
    uint256[] memory amounts = new uint256[](0);
    GovernanceERC20.MintSettings memory mintSettings = GovernanceERC20.MintSettings({
        receivers: receivers,
        amounts: amounts
    });

    (, MerkleMinter merkleMinter) = tokenFactory.createToken(dao, tokenConfig, mintSettings);

    // Assume we have a new V2 implementation...
    MerkleMinter merkleMinterV2Impl = new MerkleMinter();

    // The following will fail when the UUPS checks if the upgrade came from the proxy (since there's no proxy)
    vm.expectRevert("Function must be called through active proxy");
    PluginUUPSUpgradeable(merkleMinter).upgradeTo(address(merkleMinterV2Impl));
}

Recommendation

The MerkleMinter instance should be created using a proxy over the base implementation (createERC1967Proxy) instead of cloning the implementation:

diff --git a/src/framework/utils/TokenFactory.sol b/src/framework/utils/TokenFactory.sol
index 381e745..91441e5 100644
--- a/src/framework/utils/TokenFactory.sol
+++ b/src/framework/utils/TokenFactory.sol
@@ -15,6 +15,7 @@ import {GovernanceWrappedERC20} from "../../token/ERC20/governance/GovernanceWra
 import {IERC20MintableUpgradeable} from "../../token/ERC20/IERC20MintableUpgradeable.sol";
 import {DAO} from "../../core/dao/DAO.sol";
 import {IDAO} from "../../core/dao/IDAO.sol";
+import {createERC1967Proxy} from "../../utils/Proxy.sol";

 /// @title TokenFactory
 /// @author Aragon Association - 2022-2023
@@ -116,12 +117,15 @@ contract TokenFactory {
             _mintSettings
         );

-        // Clone and initialize a `MerkleMinter`
-        address merkleMinter = merkleMinterBase.clone();
-        MerkleMinter(merkleMinter).initialize(
-            _managingDao,
-            IERC20MintableUpgradeable(token),
-            distributorBase
+        // Create proxy and initialize a `MerkleMinter`
+        address merkleMinter = createERC1967Proxy(
+            merkleMinterBase,
+            abi.encodeWithSelector(
+                MerkleMinter.initialize.selector,
+                _managingDao,
+                token,
+                distributorBase
+            )
         );

         // Emit the event

[0xean]

I don't believe a MerkleMinter instance is intended to be upgradeable.

The Clones library provides a way to deploy minimal non-upgradeable proxies for cheap. This can be useful for applications that require deploying many instances of the same contract (for example one per user, or one per task). These instances are designed to be both cheap to deploy, and cheap to call. The drawback being that they are not upgradeable.

Will leave open for sponsor confirmation. But most likely this is invalid.

[c4-sponsor]

novaknole20 marked the issue as sponsor confirmed

[0xean]

It appears the sponsor does intend for this to be upgradeable since they confirmed the issue. Awarding as M

[c4-judge]

0xean marked the issue as satisfactory

[novaknole20]

Sorry we don't know why this got confirmed from our side.... As seen in the code the MerkleMinter is only used by the TokenFactory and there we use clones (minimal non-upgradable proxies). Therefore we need to extend from PluginUUPSUpgradeable to disable the initializer in the base during construction. We are aware that we cannot take back the bounty paid to the warden but we would like to have it removed from the report or a comment mentioning our mistake in accepting it and that we never attend to use it in an upgradeable pattern.

[novaknole20]

In Discord we discussed adding additional wording/context to the finding M-02:

MerkleMinter is UUPSUpgradeable and the upgradeability is part of its feature set. However, in the context of the TokenFactory we opted to deploy it via the minimal proxy pattern to save gas. This is because its upgradeability feature is not needed here and the cloning can be done as well. We are fully aware that the MerkleMinter proxies created through the TokenFactory are not upgradeable. Although we accidentally accepted this medium risk finding initially, we don't think that intentional absent of upgradeability qualifies as medium risk/finding. Accordingly, we would suggest to not rate this as a medium risk finding in the final audit report.

To make our intentions more clear, we made the following change to the documentation: https://github.com/aragon/osx/pull/362/files

It is also worth noting that we are currently using neither TokenFactory nor MerkleMinter/MerkleDistributor in our framework and that we will move the code out of the aragon/osx repository in the future.

[zarkk01]

warden got that $17k tho?