feat: apply audit results about token minting and distribution

op-e2e/e2eutils/setup.go

@@ -263,44 +263,58 @@ func SetUpGovernanceTokenOnL1(t require.TestingT, ctx context.Context, l1Client
 	require.NoError(t, err)
 	deployConfig.GovernanceTokenAddress = govTokenAddr
 
+	// Deploy GovernanceToken on L1
 	l1GovTokenAddr, tx, _, err := bindings.DeployGovernanceToken(l1Opts, l1Client, l1Deployments.L1StandardBridgeProxy, govTokenAddr)
 	require.NoError(t, err)
 	_, err = wait.ForReceiptOK(ctx, l1Client, tx.Hash())
 	require.NoError(t, err)
 
+	// Deploy L1MintManager
 	l1MintManagerShares := make([]*big.Int, len(deployConfig.L1MintManagerShares))
 	for i, v := range deployConfig.L1MintManagerShares {
 		l1MintManagerShares[i] = new(big.Int).SetUint64(v)
 	}
 
-	// Deploy L1MintManager
 	l1MintManagerAddr, tx, l1MintManager, err := bindings.DeployMintManager(l1Opts, l1Client, l1GovTokenProxyAddr,
 		deployConfig.MintManagerOwner, deployConfig.L1MintManagerRecipients, l1MintManagerShares)
 	require.NoError(t, err)
 	_, err = wait.ForReceiptOK(ctx, l1Client, tx.Hash())
 	require.NoError(t, err)
 
+	// Accept ownership of L1MintManager (sysCfgOwner as owner)
+	tx, err = l1MintManager.AcceptOwnership(l1Opts)
+	require.NoError(t, err)
+	_, err = wait.ForReceiptOK(ctx, l1Client, tx.Hash())
+	require.NoError(t, err)
+
+	// Upgrade proxy and initialize L1GovernanceToken
 	govTokenABI, err := bindings.GovernanceTokenMetaData.GetAbi()
 	require.NoError(t, err)
 	data, err := govTokenABI.Pack("initialize", l1MintManagerAddr)
 	require.NoError(t, err)
 
-	// Upgrade proxy and initialize L1GovernanceToken
 	tx, err = l1GovTokenProxy.UpgradeToAndCall(l1Opts, l1GovTokenAddr, data)
 	require.NoError(t, err)
 	_, err = wait.ForReceiptOK(ctx, l1Client, tx.Hash())
 	require.NoError(t, err)
 
-	l1GovToken, err := bindings.NewGovernanceTokenCaller(l1GovTokenProxyAddr, l1Client)
+	// Accept ownership of L1GovernanceToken (L1MintManager as owner)
+	tx, err = l1MintManager.AcceptOwnershipOfToken(l1Opts)
+	require.NoError(t, err)
+	_, err = wait.ForReceiptOK(ctx, l1Client, tx.Hash())
 	require.NoError(t, err)
 
 	// Ensure that the GovernanceToken is not distributed yet
+	l1GovToken, err := bindings.NewGovernanceTokenCaller(l1GovTokenProxyAddr, l1Client)
+	require.NoError(t, err)
+
 	for _, recipient := range deployConfig.L1MintManagerRecipients {
 		balance, err := l1GovToken.BalanceOf(nil, recipient)
 		require.NoError(t, err)
 		require.Equal(t, "0", balance.String())
 	}
 
+	// Mint and distribute GovernanceToken
 	tx, err = l1MintManager.Mint(l1Opts)
 	require.NoError(t, err)
 	_, err = wait.ForReceiptOK(ctx, l1Client, tx.Hash())

packages/contracts/.gas-snapshot

@@ -90,19 +90,17 @@ GasPriceOracleEcotone_Test:test_l1BaseFee_succeeds() (gas: 10745)
 GasPriceOracleEcotone_Test:test_overhead_legacyFunction_reverts() (gas: 10507)
 GasPriceOracleEcotone_Test:test_scalar_legacyFunction_reverts() (gas: 10517)
 GasPriceOracleEcotone_Test:test_setEcotone_wrongCaller_reverts() (gas: 11616)
-GovernanceToken_Test:test_approve_succeeds() (gas: 146270)
-GovernanceToken_Test:test_burnFrom_succeeds() (gas: 136197)
-GovernanceToken_Test:test_burn_fromBridge_succeeds() (gas: 97657)
-GovernanceToken_Test:test_burn_succeeds() (gas: 125058)
-GovernanceToken_Test:test_constructor_succeeds() (gas: 26552)
-GovernanceToken_Test:test_decreaseAllowance_succeeds() (gas: 150838)
-GovernanceToken_Test:test_increaseAllowance_succeeds() (gas: 150886)
-GovernanceToken_Test:test_initialize_succeeds() (gas: 14841)
-GovernanceToken_Test:test_mint_fromBridge_succeeds() (gas: 116934)
-GovernanceToken_Test:test_mint_fromNotMinter_reverts() (gas: 29934)
-GovernanceToken_Test:test_mint_fromOwner_succeeds() (gas: 119073)
-GovernanceToken_Test:test_transferFrom_succeeds() (gas: 161152)
-GovernanceToken_Test:test_transfer_succeeds() (gas: 151943)
+GovernanceToken_Test:test_approve_succeeds() (gas: 147735)
+GovernanceToken_Test:test_burn_fromBridge_succeeds() (gas: 101673)
+GovernanceToken_Test:test_burn_fromNotBridge_reverts() (gas: 119917)
+GovernanceToken_Test:test_constructor_succeeds() (gas: 26460)
+GovernanceToken_Test:test_decreaseAllowance_succeeds() (gas: 152214)
+GovernanceToken_Test:test_increaseAllowance_succeeds() (gas: 152329)
+GovernanceToken_Test:test_mint_fromBridge_succeeds() (gas: 120468)
+GovernanceToken_Test:test_mint_fromNotMinter_reverts() (gas: 29912)
+GovernanceToken_Test:test_mint_fromOwner_succeeds() (gas: 122562)
+GovernanceToken_Test:test_transferFrom_succeeds() (gas: 162551)
+GovernanceToken_Test:test_transfer_succeeds() (gas: 153430)
 Hashing_hashDepositSource_Test:test_hashDepositSource_succeeds() (gas: 639)
 KromaMintableERC20_Test:test_bridge_succeeds() (gas: 7707)
 KromaMintableERC20_Test:test_burn_notBridge_reverts() (gas: 11172)
@@ -172,8 +170,8 @@ KromaVestingWalletTest:test_initialize_succeeds() (gas: 21631)
 KromaVestingWalletTest:test_release_afterFullyVested_succeeds() (gas: 79730)
 KromaVestingWalletTest:test_release_notBeneficiary_reverts() (gas: 28574)
 KromaVestingWalletTest:test_release_succeeds() (gas: 116326)
-KromaVestingWalletTest:test_release_tokenAfterFullyVested_succeeds() (gas: 95092)
-KromaVestingWalletTest:test_release_token_succeeds() (gas: 154585)
+KromaVestingWalletTest:test_release_tokenAfterFullyVested_succeeds() (gas: 95049)
+KromaVestingWalletTest:test_release_token_succeeds() (gas: 154458)
 L1BlockBedrock_Test:test_updateValues_succeeds() (gas: 65631)
 L1BlockEcotone_Test:test_setL1BlockValuesEcotone_isDepositor_succeeds() (gas: 80519)
 L1BlockEcotone_Test:test_setL1BlockValuesEcotone_notDepositor_fails() (gas: 7621)
@@ -285,20 +283,23 @@ L2StandardBridge_Test:test_receive_succeeds() (gas: 128689)
 L2ToL1MessagePasserTest:test_burn_succeeds() (gas: 109877)
 L2ToL1MessagePasserTest:test_initiateWithdrawal_fromContract_succeeds() (gas: 70364)
 L2ToL1MessagePasserTest:test_initiateWithdrawal_fromEOA_succeeds() (gas: 75917)
-MintManagerTest:test_constructor_invalidLengthArray_reverts() (gas: 144816)
-MintManagerTest:test_constructor_succeeds() (gas: 133099)
-MintManagerTest:test_constructor_tooManyShares_reverts() (gas: 663158)
-MintManagerTest:test_constructor_zeroRecipient_reverts() (gas: 146906)
-MintManagerTest:test_constructor_zeroShares_reverts() (gas: 146553)
-MintManagerTest:test_distribute_fromNotOwner_reverts() (gas: 13005)
-MintManagerTest:test_distribute_succeeds() (gas: 539675)
-MintManagerTest:test_mint_alreadyMinted_reverts() (gas: 228314)
-MintManagerTest:test_mint_fromNotOwner_reverts() (gas: 12975)
-MintManagerTest:test_mint_succeeds() (gas: 226333)
-MintManagerTest:test_renounceOwnershipOfToken_fromNotOwner_reverts() (gas: 12940)
-MintManagerTest:test_renounceOwnershipOfToken_succeeds() (gas: 27632)
-MintManagerTest:test_transferOwnershipOfToken_fromNotOwner_reverts() (gas: 13125)
-MintManagerTest:test_transferOwnershipOfToken_succeeds() (gas: 35019)
+MintManagerTest:test_acceptOwnershipOfToken_fromNotOwner_reverts() (gas: 13015)
+MintManagerTest:test_constructor_invalidLengthArray_reverts() (gas: 162680)
+MintManagerTest:test_constructor_sameRecipient_succeeds() (gas: 1059218)
+MintManagerTest:test_constructor_succeeds() (gas: 132879)
+MintManagerTest:test_constructor_tooManyShares_reverts() (gas: 688804)
+MintManagerTest:test_constructor_zeroRecipient_reverts() (gas: 164769)
+MintManagerTest:test_constructor_zeroShares_reverts() (gas: 164399)
+MintManagerTest:test_distribute_fromNotOwner_reverts() (gas: 13055)
+MintManagerTest:test_distribute_succeeds() (gas: 558730)
+MintManagerTest:test_mint_alreadyMinted_reverts() (gas: 248847)
+MintManagerTest:test_mint_fromNotOwner_reverts() (gas: 12959)
+MintManagerTest:test_mint_succeeds() (gas: 246927)
+MintManagerTest:test_renounceOwnershipOfToken_beforeMinted_reverts() (gas: 15197)
+MintManagerTest:test_renounceOwnershipOfToken_fromNotOwner_reverts() (gas: 13014)
+MintManagerTest:test_renounceOwnershipOfToken_succeeds() (gas: 254991)
+MintManagerTest:test_transferAndAcceptOwnershipOfToken_succeeds() (gas: 1430287)
+MintManagerTest:test_transferOwnershipOfToken_fromNotOwner_reverts() (gas: 13174)
 NodeReader_Test:test_readBytes32_32bytesting() (gas: 637)
 NodeReader_Test:test_readBytes32_too_short_byte() (gas: 3543)
 NodeReader_Test:test_readBytesN_4bytesting() (gas: 900)

packages/contracts/.storage-layout

@@ -379,14 +379,17 @@
 | __gap                            | uint256[50]                                        | 13   | 0      | 1600  | contracts/governance/GovernanceToken.sol:GovernanceToken |
 | _owner                           | address                                            | 63   | 0      | 20    | contracts/governance/GovernanceToken.sol:GovernanceToken |
 | __gap                            | uint256[49]                                        | 64   | 0      | 1568  | contracts/governance/GovernanceToken.sol:GovernanceToken |
+| _pendingOwner                    | address                                            | 113  | 0      | 20    | contracts/governance/GovernanceToken.sol:GovernanceToken |
+| __gap                            | uint256[49]                                        | 114  | 0      | 1568  | contracts/governance/GovernanceToken.sol:GovernanceToken |
 
 =======================
 ➡ contracts/governance/MintManager.sol:MintManager
 =======================
 
-| Name       | Type                        | Slot | Offset | Bytes | Contract                                         |
-|------------|-----------------------------|------|--------|-------|--------------------------------------------------|
-| _owner     | address                     | 0    | 0      | 20    | contracts/governance/MintManager.sol:MintManager |
-| minted     | bool                        | 0    | 20     | 1     | contracts/governance/MintManager.sol:MintManager |
-| recipients | address[]                   | 1    | 0      | 32    | contracts/governance/MintManager.sol:MintManager |
-| shareOf    | mapping(address => uint256) | 2    | 0      | 32    | contracts/governance/MintManager.sol:MintManager |
+| Name          | Type                        | Slot | Offset | Bytes | Contract                                         |
+|---------------|-----------------------------|------|--------|-------|--------------------------------------------------|
+| _owner        | address                     | 0    | 0      | 20    | contracts/governance/MintManager.sol:MintManager |
+| _pendingOwner | address                     | 1    | 0      | 20    | contracts/governance/MintManager.sol:MintManager |
+| minted        | bool                        | 1    | 20     | 1     | contracts/governance/MintManager.sol:MintManager |
+| recipients    | address[]                   | 2    | 0      | 32    | contracts/governance/MintManager.sol:MintManager |
+| shareOf       | mapping(address => uint256) | 3    | 0      | 32    | contracts/governance/MintManager.sol:MintManager |

packages/contracts/contracts/governance/GovernanceToken.sol

@@ -1,9 +1,8 @@
 // SPDX-License-Identifier: MIT
 pragma solidity 0.8.15;
 
-import { ERC20Burnable } from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol";
 import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol";
-import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
+import "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol";
 
 import { KromaMintableERC20 } from "../universal/KromaMintableERC20.sol";
 
@@ -15,7 +14,7 @@ import { KromaMintableERC20 } from "../universal/KromaMintableERC20.sol";
  *         `Bridge`, and the total supply amount is minted at once (TGE). `Bridge` has the
  *         permission to `mint` and `burn`, for the purpose of bridging KRO to the remote chain.
  */
-contract GovernanceToken is KromaMintableERC20, ERC20Burnable, ERC20Votes, OwnableUpgradeable {
+contract GovernanceToken is KromaMintableERC20, ERC20Votes, Ownable2StepUpgradeable {
     /**
      * @notice Constructs the GovernanceToken contract.
      *
@@ -35,7 +34,7 @@ contract GovernanceToken is KromaMintableERC20, ERC20Burnable, ERC20Votes, Ownab
      * @param _owner The owner of this contract.
      */
     function initialize(address _owner) public initializer {
-        __Ownable_init();
+        __Ownable2Step_init();
         transferOwnership(_owner);
     }
 
@@ -98,6 +97,7 @@ contract GovernanceToken is KromaMintableERC20, ERC20Burnable, ERC20Votes, Ownab
      */
     function _mint(address account, uint256 amount) internal override(ERC20, ERC20Votes) {
         super._mint(account, amount);
+        emit Mint(account, amount);
     }
 
     /**
@@ -108,6 +108,7 @@ contract GovernanceToken is KromaMintableERC20, ERC20Burnable, ERC20Votes, Ownab
      */
     function _burn(address account, uint256 amount) internal override(ERC20, ERC20Votes) {
         super._burn(account, amount);
+        emit Burn(account, amount);
     }
 
     /**

packages/contracts/contracts/governance/MintManager.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: MIT
 pragma solidity 0.8.15;
 
-import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
+import { Ownable2Step } from "@openzeppelin/contracts/access/Ownable2Step.sol";
 
 import { GovernanceToken } from "./GovernanceToken.sol";
 
@@ -10,7 +10,7 @@ import { GovernanceToken } from "./GovernanceToken.sol";
  * @notice MintManager issues mint cap amount of GovernanceToken at once (TGE) and distributes the
  *         tokens to specified recipients.
  */
-contract MintManager is Ownable {
+contract MintManager is Ownable2Step {
     /**
      * @notice The amount of tokens that can be minted.
      */
@@ -69,9 +69,11 @@ contract MintManager is Ownable {
             uint256 share = _shares[i];
             require(share != 0, "MintManager: share cannot be 0");
 
+            if (shareOf[recipient] == 0) {
+                recipients.push(recipient);
+            }
+            shareOf[recipient] += share;
             totalShares += share;
-            recipients.push(recipient);
-            shareOf[recipient] = share;
         }
 
         require(
@@ -113,15 +115,14 @@ contract MintManager is Ownable {
             uint256 amount = (mintCap * share) / SHARE_DENOMINATOR;
             GOVERNANCE_TOKEN.transfer(recipient, amount);
         }
-
-        uint256 balance = GOVERNANCE_TOKEN.balanceOf(address(this));
-        require(balance == 0, "MintManager: tokens remain after distribution");
     }
 
     /**
      * @notice Only the owner is allowed to renounce the ownership of the GovernanceToken.
      */
     function renounceOwnershipOfToken() external onlyOwner {
+        require(minted, "MintManager: not minted before renounce ownership");
+
         GOVERNANCE_TOKEN.renounceOwnership();
     }
 
@@ -133,4 +134,11 @@ contract MintManager is Ownable {
     function transferOwnershipOfToken(address newMintManager) external onlyOwner {
         GOVERNANCE_TOKEN.transferOwnership(newMintManager);
     }
+
+    /**
+     * @notice Only the owner is allowed to accept the ownership of the GovernanceToken.
+     */
+    function acceptOwnershipOfToken() external onlyOwner {
+        GOVERNANCE_TOKEN.acceptOwnership();
+    }
 }

packages/contracts/contracts/test/GovernanceToken.t.sol

@@ -17,6 +17,10 @@ contract GovernanceToken_Test is CommonTest {
     address remoteToken;
     address mintManager;
 
+    event Mint(address indexed account, uint256 amount);
+
+    event Burn(address indexed account, uint256 amount);
+
     /// @dev Sets up the test suite.
     function setUp() public virtual override {
         super.setUp();
@@ -33,6 +37,11 @@ contract GovernanceToken_Test is CommonTest {
             address(govTokenImpl),
             abi.encodeCall(governanceToken.initialize, mintManager)
         );
+        assertEq(governanceToken.pendingOwner(), mintManager);
+
+        vm.prank(mintManager);
+        governanceToken.acceptOwnership();
+        assertEq(governanceToken.owner(), mintManager);
     }
 
     /// @dev Tests that the constructor sets the correct initial state.
@@ -45,13 +54,11 @@ contract GovernanceToken_Test is CommonTest {
         assertEq(governanceToken.totalSupply(), 0);
     }
 
-    function test_initialize_succeeds() external {
-        assertEq(governanceToken.owner(), mintManager);
-    }
-
     /// @dev Tests that the owner can successfully call `mint`.
     function test_mint_fromOwner_succeeds() external {
         // Mint 100 tokens.
+        vm.expectEmit(true, false, false, true, address(governanceToken));
+        emit Mint(rando, 100);
         vm.prank(mintManager);
         governanceToken.mint(rando, 100);
 
@@ -63,6 +70,8 @@ contract GovernanceToken_Test is CommonTest {
     /// @dev Tests the bridge contract can successfully call `mint`.
     function test_mint_fromBridge_succeeds() external {
         // Mint 100 tokens.
+        vm.expectEmit(true, false, false, true, address(governanceToken));
+        emit Mint(rando, 100);
         vm.prank(bridge);
         governanceToken.mint(rando, 100);
 
@@ -83,28 +92,15 @@ contract GovernanceToken_Test is CommonTest {
         assertEq(governanceToken.totalSupply(), 0);
     }
 
-    /// @dev Tests that the token owner can successfully call `burn`.
-    function test_burn_succeeds() external {
-        // Mint 100 tokens to rando.
-        vm.prank(mintManager);
-        governanceToken.mint(rando, 100);
-
-        // Rando burns their tokens.
-        vm.prank(rando);
-        governanceToken.burn(50);
-
-        // Balances have updated correctly.
-        assertEq(governanceToken.balanceOf(rando), 50);
-        assertEq(governanceToken.totalSupply(), 50);
-    }
-
     /// @dev Tests that the bridge contract can successfully call `burn`.
     function test_burn_fromBridge_succeeds() external {
         // Mint 100 tokens to rando.
         vm.prank(mintManager);
         governanceToken.mint(rando, 100);
 
         // Bridge burns rando's tokens.
+        vm.expectEmit(true, false, false, true, address(governanceToken));
+        emit Burn(rando, 100);
         vm.prank(bridge);
         governanceToken.burn(rando, 100);
 
@@ -113,23 +109,15 @@ contract GovernanceToken_Test is CommonTest {
         assertEq(governanceToken.totalSupply(), 0);
     }
 
-    /// @dev Tests that non-owner can successfully call `burnFrom`.
-    function test_burnFrom_succeeds() external {
+    /// @dev Tests that other than bridge contract cannot call `burn`.
+    function test_burn_fromNotBridge_reverts() external {
         // Mint 100 tokens to rando.
         vm.prank(mintManager);
         governanceToken.mint(rando, 100);
 
-        // Rando approves alice to burn 50 tokens.
+        vm.expectRevert("KromaMintableERC20: only bridge can mint and burn");
         vm.prank(rando);
-        governanceToken.approve(alice, 50);
-
-        // Alice burns 50 tokens from rando.
-        vm.prank(alice);
-        governanceToken.burnFrom(rando, 50);
-
-        // Balances have updated correctly.
-        assertEq(governanceToken.balanceOf(rando), 50);
-        assertEq(governanceToken.totalSupply(), 50);
+        governanceToken.burn(rando, 100);
     }
 
     /// @dev Tests that `transfer` correctly transfers tokens.

packages/contracts/contracts/test/KromaVestingWallet.t.sol

@@ -49,6 +49,8 @@ contract KromaVestingWalletTest is CommonTest {
             address(tokenImpl),
             abi.encodeCall(token.initialize, tokenOwner)
         );
+        vm.prank(tokenOwner);
+        token.acceptOwnership();
 
         vm.prank(tokenOwner);
         token.mint(address(vestingWallet), totalAllocation);