Lines of code

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&gt;

...

// 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&gt;

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  

---

The text was updated successfully, but these errors were encountered:

All reactions