movementlabsxyz / movement

The Movement Network is a Move-based L2 on Ethereum.
Apache License 2.0
78 stars 64 forks source link

"Cleaner" Movement token deployment #541

Open 0xPrimata opened 2 months ago

0xPrimata commented 2 months ago

Is your feature request related to a problem? Please describe. One requirement for the token is to have a cleaner initial role ownership. The suggestion is for a multisig to have full control over the token at its initial stage. This requires modifications to the contract.

Describe the solution you'd like

the mintableToken initializer should take two address parameters to grant initial minting and mint admin roles without requiring secondary transactions.

function __MintableToken_init(
        string memory name,
        string memory symbol,
        address minterAdmin,
        address minter
    ) internal onlyInitializing {
        __ERC20_init_unchained(name, symbol);
        __BaseToken_init_unchained();
        __MintableToken_init_unchained(minterAdmin, minter);
    }

    function __MintableToken_init_unchained(address minterAdmin, address minter) internal onlyInitializing {
        _grantRole(MINTER_ADMIN_ROLE, minterAdmin);
        _grantRole(MINTER_ROLE, minter);
    }

Describe alternatives you've considered Switch from MintableToken to ERC20Upgradeable inheritance. The new Contract would be:

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

import {ERC20Upgradeable} from "@openzeppelin/upgradeable-contracts/token/ERC20/ERC20Upgradeable.sol";
contract MOVEToken is is ERC20Upgradeable {
    /**
     * @dev Initialize the contract
     */
    function initialize(address multisig) public initializer {
        __ERC20_init("Movement", "MOVE");
        _mint(address(multisig), 10000000000 * 10 ** decimals());
    }

    function decimals() public pure override returns (uint8) {
        return 8;
    }
}

Additional context I do think that the second option is our best bet.

l-monninger commented 2 months ago

Why not just have the multisig address be the only minter as designated by the MINT_ROLE? The above mints a large amount of token for the multisig which can be immediately used, creating a high-incentive for an attack. Meanwhile, if we assume a timelock between the multisig and mint operations, such an attack is much less attractive.

0xPrimata commented 2 months ago

That token doesn't have any market value, if anything we remint the contract. It's also a requirement that the entire supply is minted at the time of the token deployment and no minter role is available without an upgrade.

l-monninger commented 2 months ago

It will have value.

Where is this requirement coming from and why? This might be internal, so Slack me.