GoGoPool contest - chaduke's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. avaxAssignedHighWater is set wrongly in MinipoolManager.sol at L373, particularly when the minipool was created via recreateMinipool().

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. According to the documentation of avaxAssignedHighWater at L26 of Staking.sol， ”staker.item<index>.avaxAssignedHighWater = Highest amt of liquid staker funds assigned during a GGP rewards cycle“. However, avaxAssignedHighWater is set wrongly in MinipoolManager.sol at L373 below:

if (staking.getAVAXAssignedHighWater(owner) < staking.getAVAXAssigned(owner)) {
			staking.increaseAVAXAssignedHighWater(owner, avaxLiquidStakerAmt);
}

It is wrong because instead of replacing the avaxAssignedHighWater with a larger value of avaxAssigned, it adds avaxLiquidStakerAmt on top of the existing avaxAssignedHighWater , which is inconsistent with the definition. In particular, when this minipool was created via recreateMinipool(), the new avaxLiquidStakerAmt is a compound value which contains the original avaxLiquidStakerAmt ALREADY. The original avaxLiquidStakerAmt will be added AGAIN in this case, which is wrong. Therefore, the safest way is to assign the larger avaxAssigned, based on the definition to be 100% correct.

Tools Used

Remix

Recommended Mitigation Steps

We can fix it with the following code, which will replace the existing avaxAssignedHighWater with the current larger value of avaxAssigned using function resetAVAXAssignedHighWater.

if (staking.getAVAXAssignedHighWater(owner) < staking.getAVAXAssigned(owner)) {
			staking.resetAVAXAssignedHighWater(owner);
}

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. The main problem with the implementation of requireValidStateTransition()is the mix of a normal state with the error state. As a result, some undesirable consequence might occur. For example, when the current state is MinipoolStatus.Withdrawable, it is possible to have the next state as MinipoolStatus.prelaunch. As a result, a minipool might be get into prelaunch state before the rewards and the orginal AVAX of amount avaxNodeOpAmt have been claimed.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. Consider the scenario that a minipool is currently in the state of MinipoolStatus.Withdrawable`, then the owner or another person can run the createMinipool()with the samenodeId(either on purpose or by accident), which will eliminate all the existing data of the minipool before the rewards and the originalavaxNodeOpRewardAmt`` have been claimed. As result, the owner lost all the staking AVAX and validation rewards.

if (minipoolIndex != -1) {
			requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
			resetMinipoolData(minipoolIndex);
			// Also reset initialStartTime as we are starting a whole new validation
			setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), 0);

Tools Used

Remix

Recommended Mitigation Steps

Fix the logic of requireValidStateTransition() to separate normal states from error states so that transition such as MinipoolStatus.Withdrawable -> MinipoolStatus.Prelaunch is impossible. Note in our design, the owner will call withdrawMinipoolFunds() to claim all funds and transit the status of the minipool to MinipoolStatus.Finished before he/she relaunch the minipool, so this design is much safer:

function requireValidStateTransition(int256 minipoolIndex, MinipoolStatus to) private view {
		bytes32 statusKey = keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status"));
		MinipoolStatus currentStatus = MinipoolStatus(getUint(statusKey));
		bool isValid;

		if (currentStatus == MinipoolStatus.Prelaunch) {
			isValid = (to == MinipoolStatus.Launched || to == MinipoolStatus.Canceled);
		} else if (currentStatus == MinipoolStatus.Launched) {
			isValid = (to == MinipoolStatus.Staking || to == MinipoolStatus.Error);
		} else if (currentStatus == MinipoolStatus.Staking) {
			isValid = (to == MinipoolStatus.Withdrawable || to == MinipoolStatus.Error);
		} else if (currentStatus == MinipoolStatus.Withdrawable) {
			isValid = (to == MinipoolStatus.Finished);
		}else if (currentStatus == MinipoolStatus.Error) {
			isValid = (to == MinipoolStatus.Finished);
		else if (currentStatus == MinipoolStatus.Finished) {
			isValid = (to == MinipoolStatus.Prelaunch);
                else if (currentStatus == MinipoolStatus.Cancelled) {
			isValid = (to == MinipoolStatus.Prelaunch);		
		} else {
			isValid = false;
		}

		if (!isValid) {
			revert InvalidStateTransition();
		}
	}

As a result, only MinipoolStatus.Finished->MinipoolStatus.Prelaunch and MinipoolStatus.Cancelled->MinipoolStatus.Prelaunch are possible. In both cases, the owner has claimed AVAX of amount avaxNodeOpAmt and reward amount avaxNodeOpRewardAmt if any, so it is safe to relaunch the minipool.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. A malicous user might steal the ownership from a minipool owner. As a result, the original owner will lose all the staked AVAX and AVAX rewards.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. Consider the following scenario:

1. Suppose a minipool with nodedID = 007 has been created by and currently owned by Alice;
2. Suppose the status of the minipool is MinipoolStatus.Withdrawable, it is possible to steal the owner in other status too, but in this status, the original owner will lose the most.
3. Now Bob calls createMinipool(007, ...), which will succeed since 1) MinipoolStatus.Withdrawable-> MinipoolStatus.Prelaunch is a legal transition; and 2) there is no check that the caller has to be the original owner of the minipool. Because it is an existing minipool, the if-part of the following code will be executed:

             if (minipoolIndex != -1) {
			requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
			resetMinipoolData(minipoolIndex);
			// Also reset initialStartTime as we are starting a whole new validation
			setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), 0);
             }
             else {
			minipoolIndex = int256(getUint(keccak256("minipool.count")));
			// The minipoolIndex is stored 1 greater than actual value. The 1 is subtracted in getIndexOf()
			setUint(keccak256(abi.encodePacked("minipool.index", nodeID)), uint256(minipoolIndex + 1));
			setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".nodeID")), nodeID);
			addUint(keccak256("minipool.count"), 1);
             }

		// Save the attrs individually in the k/v store
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Prelaunch));
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".duration")), duration);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".delegationFee")), delegationFee);
		setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")), msg.sender);

As a result, Bob stole the ownership of the minipool and Alice will lose the ownership as well as her AVAX staking and rewards .

Tools Used

Remix

Recommended Mitigation Steps

We need to check whether the caller msg.sender is the owner of the minipool or not so that the ownership can never be stolen.

               if (minipoolIndex != -1) {
                        address owner = onlyOwner(minipoolIndex); // @audit: only owner to prevent ownership stealing

			requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
			resetMinipoolData(minipoolIndex);
			// Also reset initialStartTime as we are starting a whole new validation
			setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), 0);
		} else {
			minipoolIndex = int256(getUint(keccak256("minipool.count")));
			// The minipoolIndex is stored 1 greater than actual value. The 1 is subtracted in getIndexOf()
			setUint(keccak256(abi.encodePacked("minipool.index", nodeID)), uint256(minipoolIndex + 1));
			setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".nodeID")), nodeID);
			addUint(keccak256("minipool.count"), 1);
		}

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L209-L217

Vulnerability details

Impact

Detailed description of the impact of this finding. when the new name of the contract is the same as the old name of a contract (which is necessary to move funding from old contracts to upgraded ones), the function upgradeExistingContract() will fail to register the new address of the contract and in particular, getContractAddress(newName) will return zero after the upgrade. The whole system will not work after that since modifiers guardianOrSpecificRegisteredContract() and onlySpecificRegisteredContract() will always revert for this contract.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L209-L217 The main problem of upgradeExistingContract() is that it performs registerContract() first and then performs unregisterContract(). The order should be reversed. Because of this, the following statement of registerContract()

setAddress(keccak256(abi.encodePacked("contract.address", name)), addr);

is overwritten by the following statement of unregisteredContract():

deleteAddress(keccak256(abi.encodePacked("contract.address", name)));

as a result, the registration of the address of the contract under the name is lost when oldName == newName. Moreover, getContractAddress(newName) will return zero after the upgrade. The whole system might fail after that.

Tools Used

Remix

Recommended Mitigation Steps

The fix is simple, just exchange the order of registerContract() and unregisterContract() in function upgradeExistingContract():

function upgradeExistingContract(
		address newAddr,
		string memory newName,
		address existingAddr
	) external onlyGuardian {
		unregisterContract(existingAddr);
		registerContract(newAddr, newName);
}

Lines of code

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

Vulnerability details

Impact

The new RewardsPool.InflationIntervalStartTime is set wrongly in inflate() as follows:

addUint(keccak256("RewardsPool.InflationIntervalStartTime"), inflationIntervalElapsedSeconds);

It is wrong because it effectively set the new RewardsPool.InflationIntervalStartTime as the current time since inflationIntervalElapsedSeconds is calculated in L84 as follows:

uint256 inflationIntervalElapsedSeconds = (block.timestamp - getInflationIntervalStartTime());

As a result, some remainder time as a result of division by dao.getInflationIntervalSeconds() (24 hrs) will be lost without inflation.

Proof of Concept

Consider the following scenario:

1. If the inflate() is called after every 1.999 days, then the total supply will be inflated for 1 day each time. Meanwhile, inflationIntervalElapsedSeconds is calculatedly as the amount of seconds in 1.999 days and used to set the new RewardsPool.InflationIntervalStartTime. As a result, the inflation will not be accounted for the 0.999 day, not even in the next inflation interval. Accumulatively, the annual inflation rate will be less than the designated inflation rate of 5%.
2. The problem lies in the inconsistency in calculation: getInflationIntervalsElapsed() performs a rounding down division while inflationIntervalElapsedSeconds does not perform such rounding but includes all the seconds in the partial day. As a result, we lose that part of the inflation in the partial day forever. We need to calculate RewardsPool.InflationIntervalStartTime excluding the seconds in the partial day so that the inflation during the partial day can be accounted for in the next inflation interval.

Tools Used

Remix

Recommended Mitigation Steps

The following implementation calculates the elapsed inflationIntervalElapsedSeconds by excluding the seconds in the partial day, which can be accounted for in the next inflation interval. As a result, both inflation rate and rewards will be ensured without accumulative rounding error.

function inflate() internal {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
               uint256 inflationIntervalsElapsed = getInflationIntervalsElapsed();          // @audit: calculate how many inflation intervals have elapsed, rounding down
		uint256 inflationIntervalElapsedSeconds = inflationIntervalsElapsed*dao.getInflationIntervalSeconds(); 
                 // @audit: effective inflalationIntervalElassedSeconds excluding partial interval (day)

		(uint256 currentTotalSupply, uint256 newTotalSupply) = getInflationAmt();

		TokenGGP ggp = TokenGGP(getContractAddress("TokenGGP"));
		if (newTotalSupply > ggp.totalSupply()) {
			revert MaximumTokensReached();
		}

		uint256 newTokens = newTotalSupply - currentTotalSupply;

		emit GGPInflated(newTokens);

		dao.setTotalGGPCirculatingSupply(newTotalSupply);

		addUint(keccak256("RewardsPool.InflationIntervalStartTime"), inflationIntervalElapsedSeconds);
		setUint(keccak256("RewardsPool.RewardsCycleTotalAmt"), newTokens);
	}

QA1: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L73 Instead of passing assetas an argument, it is better to define it as a constant since the address for the WAVAX contract can be predetermined.

QA2. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L2 Lock all files with the most recent version of Solidity 0.8.17.

QA3: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L225-L239 The four preview-X functions need to add onlyProxy() so that they can only be called through the proxy. Otherwise, functions such as depositAVAX() and withdrawAVAX() will not work properly if they are not called via the proxy since they modify the state variable totalReleasedAssets.

function previewDeposit(uint256 assets) public view override whenTokenNotPaused(assets) onlyProxy() returns (uint256) {
		return super.previewDeposit(assets);
	}

	function previewMint(uint256 shares) public view override whenTokenNotPaused(shares) onlyProxy() returns (uint256) {
		return super.previewMint(shares);
	}

	function previewWithdraw(uint256 assets) public view override whenTokenNotPaused(assets) onlyProxy() returns (uint256) {
		return super.previewWithdraw(assets);
	}

	function previewRedeem(uint256 shares) public view override whenTokenNotPaused(shares) onlyProxy() returns (uint256) {
		return super.previewRedeem(shares);
	}

QA4: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L106 We need to ensure that receiveWithdrawalAVAX() can only be called by the TokenggAVAX contract or the Vault contract.

function receiveWithdrawalAVAX() external payable {
      assert(msg.sender == address(ggAVAX) || msg.sender == getContractAddress("Vault"));
}

QA5: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L239-L246 The implementation fails to conform to the documentation that says "If nodeID exists, only allow overwriting if node is finished or canceled). It only checked the next status is MinipoolStatus.Prelaunch without checking the current status. For example, the current status might be MinipoolStatus.Withdrawable see the logic in requireValidStateTransition(). To fix, we need to explicitly check the current status as well:

if (minipoolIndex != -1) {
                       bytes32 statusKey = keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status"));
		       MinipoolStatus currentStatus = MinipoolStatus(getUint(statusKey));

                       if(currentStutus != MinipoolStaus.Finished && currentStutus != MinipoolStatus.Canceled) revert InvalidStateTransition(); // @audit: check current status explicitly!

			requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
        		resetMinipoolData(minipoolIndex);
			// Also reset initialStartTime as we are starting a whole new validation
			setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), 0);
		} else {
			minipoolIndex = int256(getUint(keccak256("minipool.count")));
			// The minipoolIndex is stored 1 greater than actual value. The 1 is subtracted in getIndexOf()
			setUint(keccak256(abi.encodePacked("minipool.index", nodeID)), uint256(minipoolIndex + 1));
			setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".nodeID")), nodeID);
			addUint(keccak256("minipool.count"), 1);
		}

QA6: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L509-L510 Fail to call staking.decreaseMinipoolCount(owner); to decrease the minipool count. The fix is:

                Staking staking = Staking(getContractAddress("Staking"));
		staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt);
		staking.decreaseMinipoolCount(owner);
 

QA7: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L165 The function increaseRewardsCycleCount() assumes that getRewardsCyclesElapsed() = 1, this assumption might not be true. We should implement `increaseRewardsCycleCount()`` such that its correctness does not depend on this assumption, and will work regardless how many reward cycles have passed.

function increaseRewardsCycleCount() internal {
		addUint(keccak256("RewardsPool.RewardsCycleCount"), getRewardsCyclesElapsed());
}

QA8: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L230 The number of enabled enabledMultisigs should be enabledCount instead of enabledMultisigs.length, which is equal to count.

for (uint256 i = 0; i < enabledCount; i++) {
			vault.withdrawToken(enabledMultisigs[i], ggp, tokensPerMultisig);
}

QA9: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L55-L68 Need to function to enable a particular multisig.

QA10. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L166 The following functions need to add modifier whenNotPaused: depositAVAX(), withdrawAVAX(), redeemAVAX().

QA11: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L108 After syncRewards(), the following invariant must hold, so it might be a good idea to assert it:

          assert(totalReleasedAssets+lastRewardsAmt == asset.balanceOf(address(this))+stakingTotalAssets;

QA12: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L69-L73 WE should check that assets != 0 and receiver != address(0x0).

QA13: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L93 Zero address check for receiver.

QA14: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L142-L151 Calling synRewards() from this function would be appropriate since rewards will be received from this function.

function depositFromStaking(uint256 baseAmt, uint256 rewardAmt) public payable onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
		uint256 totalAmt = msg.value;
		if (totalAmt != (baseAmt + rewardAmt) || baseAmt > stakingTotalAssets) {
			revert InvalidStakingDeposit();
		}

		emit DepositedFromStaking(msg.sender, baseAmt, rewardAmt);
		stakingTotalAssets -= baseAmt;
		IWAVAX(address(asset)).deposit{value: totalAmt}();

                if(block.timestamp.safeCastTo32() >= rewardsCycleEnd) synRewards(); // better to call here
	}

QA15. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L211 We need to consider the reservedAsset as well when calculating maxWithdraw() and maxRedeem():

function maxWithdraw(address _owner) public view override returns (uint256) {
		if (getBool(keccak256(abi.encodePacked("contract.paused", "TokenggAVAX")))) {
			return 0;
		}
		uint256 assets = convertToAssets(balanceOf[_owner]);

                ProtocolDAO protocolDAO = ProtocolDAO(getContractAddress("ProtocolDAO"));]
		uint256 targetCollateralRate = protocolDAO.getTargetGGAVAXReserveRate();]

		uint256 totalAssets_ = totalAssets();

		uint256 reservedAssets = totalAssets_.mulDivDown(targetCollateralRate, 1 ether);

		uint256 avail = totalAssets - stakingTotalAssets - reservedAssets; // @audit: we need to consider reservedAssets
		return assets > avail ? avail : assets;
	}

function maxRedeem(address _owner) public view override returns (uint256) {
		if (getBool(keccak256(abi.encodePacked("contract.paused", "TokenggAVAX")))) {
			return 0;
		}
		uint256 shares = balanceOf[_owner];
                ProtocolDAO protocolDAO = ProtocolDAO(getContractAddress("ProtocolDAO"));]
		uint256 targetCollateralRate = protocolDAO.getTargetGGAVAXReserveRate();]

		uint256 totalAssets_ = totalAssets();

		uint256 reservedAssets = totalAssets_.mulDivDown(targetCollateralRate, 1 ether);


		uint256 avail = convertToShares(totalAssets - stakingTotalAssets - reservedAssets); // audit: consider reservedAssets as well
		return shares > avail ? avail : shares;
}

QA16. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L198 The duration parameter needs to be checked to make sure it is long enough.

QA17. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L180 The withdrawAVAX() needs to check to ensure the amount of withdrawal is not great than maxWithdraw(address _owner):

function withdrawAVAX(uint256 assets) public returns (uint256 shares) {
              if(assets > maxWithdraw(msg.sender)) revert WithdrawTooMuch(); // @audit: check this        

		shares = previewWithdraw(assets); // No need to check for rounding error, previewWithdraw rounds up.
		beforeWithdraw(assets, shares);
		_burn(msg.sender, shares);

		emit Withdraw(msg.sender, msg.sender, msg.sender, assets, shares);

		IWAVAX(address(asset)).withdraw(assets);
		msg.sender.safeTransferETH(assets);
	}

QA18. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L216 The redeemAVAX() needs to check to ensure the amount of withdrawal is not great than maxRedeem(address _owner):

function redeemAVAX(uint256 shares) public returns (uint256 assets) {
              if(shares > maxRedeem(msg.sender)) revert WithdrawTooMuch(); // @audit: check this        


		// Check for rounding error since we round down in previewRedeem.
		if ((assets = previewRedeem(shares)) == 0) {
			revert ZeroAssets();
		}
		beforeWithdraw(assets, shares);
		_burn(msg.sender, shares);

		emit Withdraw(msg.sender, msg.sender, msg.sender, assets, shares);

		IWAVAX(address(asset)).withdraw(assets);
		msg.sender.safeTransferETH(assets);
	}

QA19. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimProtocolDAO.sol#L22 Zero address check for recipientAddress is necessary, also need to make sure that recipientAddress is not equal to the address of ClaimNodeOp.

QA20. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L444-L478 One needs to check If the duration still has time left to go when calling recreateMinipool():

function recreateMinipool(address nodeID) external whenNotPaused {
		int256 minipoolIndex = onlyValidMultisig(nodeID);

               Minipool m = getMinipool(minipoolINdex);
               if(block.timestamp > m.initialStartTime + m.duration) revert NoLeftTimeforDuration();
 
		requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
		Minipool memory mp = getMinipool(minipoolIndex);
		// Compound the avax plus rewards
		// NOTE Assumes a 1:1 nodeOp:liqStaker funds ratio
		uint256 compoundedAvaxNodeOpAmt = mp.avaxNodeOpAmt + mp.avaxNodeOpRewardAmt;
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), compoundedAvaxNodeOpAmt);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), compoundedAvaxNodeOpAmt);

		Staking staking = Staking(getContractAddress("Staking"));
		// Only increase AVAX stake by rewards amount we are compounding
		// since AVAX stake is only decreased by withdrawMinipool()
		staking.increaseAVAXStake(mp.owner, mp.avaxNodeOpRewardAmt);
		staking.increaseAVAXAssigned(mp.owner, compoundedAvaxNodeOpAmt);
		staking.increaseMinipoolCount(mp.owner);

		if (staking.getRewardsStartTime(mp.owner) == 0) {
			// Edge case where calculateAndDistributeRewards has reset their rewards time even though they are still cycling
			// So we re-set it here to their initial start time for this minipool
			staking.setRewardsStartTime(mp.owner, mp.initialStartTime);
		}

		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		uint256 ratio = staking.getCollateralizationRatio(mp.owner);
		if (ratio < dao.getMinCollateralizationRatio()) {
			revert InsufficientGGPCollateralization();
		}

		resetMinipoolData(minipoolIndex);

		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Prelaunch));

		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Prelaunch);
	}

QA21: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L236 createMinipool() will always use the SAME multisig for .multisigAddr since the implementation of multisigManager.requireNextActiveMultisig() always returns the next enabled multisig without considering the assignments. A new implementation is needed for multisigManager.requireNextActiveMultisig() so that we will not always assign the first enabled multisig to a new minipool for a fair load balancing.

	function enableMultisig(address addr) external onlyGuardian {

G1. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/BaseAbstract.sol#L25 NO need to compare a bool value with false

if (!booleanStorage[keccak256(abi.encodePacked("contract.exists", msg.sender))] && msg.sender != guardian) {
			revert InvalidOrOutdatedContract();
		}

G2. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/BaseAbstract.sol#L40-L48 Applying the short-circuit can save gas - the second condition might not need to be evaluated sometimes

modifier guardianOrRegisteredContract() {
		bool isContract = getBool(keccak256(abi.encodePacked("contract.exists", msg.sender)));
                if (!isContract)) {
			revert MustBeGuardianOrValidContract();
		}

		bool isGuardian = msg.sender == gogoStorage.getGuardian();
		if (!isGuardian) {
			revert MustBeGuardianOrValidContract();
		}
		_;
	}

G3. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/BaseAbstract.sol#L51-L59 Applying the short-circuit can save gas - the second condition might not need to be evaluated sometimes

modifier guardianOrSpecificRegisteredContract(string memory contractName, address contractAddress) {
		bool isContract = contractAddress == getAddress(keccak256(abi.encodePacked("contract.address", contractName)));
		if (!isContract) {
			revert MustBeGuardianOrValidContract();
		}

		bool isGuardian = msg.sender == gogoStorage.getGuardian();

		if (!isGuardian) {
			revert MustBeGuardianOrValidContract();
		}
		_;
	}

G4. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L152-L175 Moving revert stmt to the default case can save gas:

function requireValidStateTransition(int256 minipoolIndex, MinipoolStatus to) private view {
		bytes32 statusKey = keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status"));
		MinipoolStatus currentStatus = MinipoolStatus(getUint(statusKey));
		bool isValid;

		if (currentStatus == MinipoolStatus.Prelaunch) {
			isValid = (to == MinipoolStatus.Launched || to == MinipoolStatus.Canceled);
		} else if (currentStatus == MinipoolStatus.Launched) {
			isValid = (to == MinipoolStatus.Staking || to == MinipoolStatus.Error);
		} else if (currentStatus == MinipoolStatus.Staking) {
			isValid = (to == MinipoolStatus.Withdrawable || to == MinipoolStatus.Error);
		} else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
			isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch);
		} else if (currentStatus == MinipoolStatus.Finished || currentStatus == MinipoolStatus.Canceled) {
			// Once a node is finished/canceled, if they re-validate they go back to beginning state
			isValid = (to == MinipoolStatus.Prelaunch);
		} else {
			revert InvalidStateTransition();
		}
	}

G5. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L75 Impossible to underflow due to previous check, so we can put it inside the unchecked to reduce gas.

unchecked{
                avaxBalances[fromContractName] = avaxBalances[fromContractName] - amount;
		avaxBalances[toContractName] = avaxBalances[toContractName] + amount;
}

G6. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L155 Impossible to underflow due to previous check, so we can put it inside the unchecked to reduce gas.

unchecked{
           tokenBalances[contractKey] = tokenBalances[contractKey] - amount;
}

G7. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L187 Impossible to underflow due to previous check, so we can put it inside the unchecked to reduce gas.

unchecked{
         tokenBalances[contractKeyFrom] = tokenBalances[contractKeyFrom] - amount;
}

G8. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/BaseAbstract.sol#L107-L197 eliminating these unnecessary getters and setters can save gas, one can simply call gogoStorage.getX(), gogoStorage.setX(), gogoStorage.deleteX(), and gogoStorage.addX(), and only increase readability of the contracts

G9. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L157-L159 No need to introduce another variable tokenContract, here, a type casing is sufficient:

ERC20(tokenAddress).safeTransfer(withdrawalAddress, amount);

G10. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L353 Replacing this line with the following line can save gas since there is no need to check AGAIN check the validity of the stakerIndex:

addUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked")), amount);

G11 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L398-L400 Much gas is wasted because the stored index of a staker is added by 1 in the storage. AS a result, both reading and updating need to make adjustment and waste gas. To save gas, gettting rid of this add-by-1/subtract-by-1 operation and let the first staker have index 1 instead. We will not use the INDEX 0.

G12. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L566 Much gas is wasted because the stored index of a minipool is added by 1 in the storage. AS a result, both reading and updating need to make adjustment and waste gas. To save gas, gettting rid of this add-by-1/subtract-by-1 operation and the first minipool will have index 1 instead. We will not use the INDEX 0.

G13 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L149 Including the line inside unchecked can save gas since we know baseAmt <= stakingTotalAssets as a result of line 144.

unchecked{stakingTotalAssets -= baseAmt;}

G14: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L128 adding unchecked for the line can save gas as we know underflow and devide-by-zero are impossible.

unchecked{
      uint256 unlockedRewards = (lastRewardsAmt_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);
}

G15: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol#L166-L177 abi.encodePacked() can be used here, which saves gas.

function computeDomainSeparator() internal view virtual returns (bytes32) {
		return
			keccak256(
				abi.encodePacked(        // @use abi.encodePacked() saves gas here
					keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
					keccak256(bytes(name)),
					keccak256("1"),
					block.chainid,
					address(this)
				)
			);
	}

G16. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol#L154-L166 Dropping the first condition can save gas since the second condition implies the first condition, in addition, owner cannot be address 0x0 since ox0 cannot sign anything)

require(recoveredAddress == owner, "INVALID_SIGNER");

G17. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol#L138-L145 abi.encodePacked() can be used here, which saves gas.

abi.encodePacked(
								keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"),
								owner,
								spender,
								value,
								nonces[owner]++,
								deadline
							)

G18. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L139 Adding unchecked can save gas here, impossible for underflow

unchecked{
           return totalAssets_ - reservedAssets - stakingTotalAssets;
}

G19. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L99 Adding unchecked can save gas here, impossible for underflow

uint256 nextRewardsAmt = (asset.balanceOf(address(this)) + stakingTotalAssets_) - totalReleasedAssets_ - lastRewardsAmt_;

G20. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L162-L163 No need to introduce another variable withdrawer and the need of assignment stmt:

IWithdrawer(msg.sender).receiveWithdrawalAVAX{value: assets}();

G21: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L142-L151 There is no need to introduce the variable totalAmt and the assignment statement in line 143, we can save gas by deleting them

function depositFromStaking(uint256 baseAmt, uint256 rewardAmt) public payable onlySpecificRegisteredContract("MinipoolManager", msg.sender) 
		if (msg.value != (baseAmt + rewardAmt) || baseAmt > stakingTotalAssets) { // @audit: replace it with msg.value
			revert InvalidStakingDeposit();
		}

		emit DepositedFromStaking(msg.sender, baseAmt, rewardAmt);
		unchecked{stakingTotalAssets -= baseAmt;}
		IWAVAX(address(asset)).deposit{value: msg.value}();  // @audit: replace it with msg.value
	}

G22. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L153 Making the function payable can save gas, as this will eliminate the code to check whether msg.value == 0, and this function will be called by MinipoolManager. Therefore, no worry to send avax via this function mistakenly.

G23: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L276-L277 Moving these two lines as the first two lines of the function can save gas when the caller is not the owner for example:

function cancelMinipool(address nodeID) external nonReentrant {
		int256 index = requireValidMinipool(nodeID);
		onlyOwner(index);

		Staking staking = Staking(getContractAddress("Staking"));
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		// make sure they meet the wait period requirement
		if (block.timestamp - staking.getRewardsStartTime(msg.sender) < dao.getMinipoolCancelMoratoriumSeconds()) {
			revert CancellationTooEarly();
		}
		_cancelMinipoolAndReturnFunds(nodeID, index);
	}

G24. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L61 Adding payable to the function can save gas since it will eliminate the code to check whether msg.value == 0, and the modifier onlySpecificRegisteredContract will ensure no AVAX will be sent to this function by mistake, so there is no need to check whether msg.value == 0.

G25. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L341-L342 No need to introduce another variable totalAvaxAmt and the assignment statement.

msg.sender.safeTransferETH(avaxNodeOpAmt + avaxLiquidStakerAmt);

g26. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L400-L403 No need to introduce another variable totalAvaxAmt and the assignment statement. Change it to

		if (msg.value != avaxNodeOpAmt + avaxLiquidStakerAmt + avaxTotalRewardAmt) {
			revert InvalidAmount();
		}

G27. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L141-L146 This block can be simply replaced by the following

return claimContractPct.mulWadDown(currentCycleRewardsTotal);

G28. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L82-L100 We can simply replace L98 with the following and eliminate L84 to save gas.

setUint(keccak256("RewardsPool.InflationIntervalStartTime"), block.timestamp);

G29: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L92 Adding unchecked can save gas since underflow is impossible here:

uint256 newTokens = newTotalSupply - currentTotalSupply;

G29. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L102 Surrounding this with unchecked with save gas since underflow is impossible due to line 92.

unchecked{
       uint256 restakeAmt = ggpRewards - claimAmt;
}

G30. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L55-L67

The real implementation of disableAllMultisigs() should be in MultisigManager.sol to save gas:

function disableAllMultisigs() public onlyDefender {
		MultisigManager mm = MultisigManager(getContractAddress("MultisigManager"));
                mm.disableAllMultisigs();
}

// in MultisigManager.sol
function disableAllMultisig(r) external guardianOrSpecificRegisteredContract("Ocyticus", msg.sender) {
		uint256 total = getUint(keccak256("multisig.count"));
                address addr;
		bool enabled;
                for (uint256 i; i < total;) {
                           (addr, enabled) = getMultisig(i);
                           if(enabled) { 
                                setBool(keccak256(abi.encodePacked("multisig.item", multisigIndex, ".enabled")), false);
      		                emit DisabledMultisig(addr, msg.sender);
                            }
                           unchecked{++i;}
               }
}

G31. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L44 Changing it to a private immutable can save gas

uint32 private immutable rewardsCycleLength;

G32: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/utils/RialtoSimulator.sol#L109 No need to call staking.getEffectiveGGPStaked(allStakers[i].stakerAddr) again, and we can use shifting to save gas as well.

  effectiveGGPStaked = effectiveGGPStaked >> 1;

G33. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/utils/RialtoSimulator.sol#L118 Using shifting to save gas

nopClaim.calculateAndDistributeRewards(allStakers[i].stakerAddr, (totalEligibleStakedGGP << 1 ));

G34: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimProtocolDAO.sol#L28-L30 No need to check these conditions since they will be checked in function withdrawToken() anyway. Deleting them can save gas.