GoGoPool contest - RaymondFam's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L379-L383 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Vault.sol#L166-L189

Vulnerability details

Impact

When calling slashGGP() in Staking.sol, ggpAmt is transferred from Staking.sol to ProtocolDAO.sol in Vault.sol. However, there is no withdraw() in ProtocolDAO.sol that could facilitate vault.transferToken() or vault.withdrawToken().

Proof of Concept

File: Staking.sol#L379-L383

	function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
		Vault vault = Vault(getContractAddress("Vault"));
		decreaseGGPStake(stakerAddr, ggpAmt);
		vault.transferToken("ProtocolDAO", ggp, ggpAmt);
	}

File: Vault.sol#L166-L189

	function transferToken(
		string memory networkContractName,
		ERC20 tokenAddress,
		uint256 amount
	) external onlyRegisteredNetworkContract {
		// Valid Amount?
		if (amount == 0) {
			revert InvalidAmount();
		}
		// Make sure the network contract is valid (will revert if not)
		getContractAddress(networkContractName);
		// Get contract keys
		bytes32 contractKeyFrom = keccak256(abi.encodePacked(getContractName(msg.sender), tokenAddress));
		bytes32 contractKeyTo = keccak256(abi.encodePacked(networkContractName, tokenAddress));
		// emit token transfer event
		emit TokenTransfer(contractKeyFrom, contractKeyTo, address(tokenAddress), amount);
		// Verify there are enough funds
		if (tokenBalances[contractKeyFrom] < amount) {
			revert InsufficientContractBalance();
		}
		// Update Balances
		tokenBalances[contractKeyFrom] = tokenBalances[contractKeyFrom] - amount;
		tokenBalances[contractKeyTo] = tokenBalances[contractKeyTo] + amount;
	}

Devoid of the capability to withdraw or transfer tokens in ProtocolDAO.sol, the slashed amount would be stuck in Vault.sol and not retrievable.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider refactoring slashGGP() that will have ggpAmt transferred to ClaimProtocolDAO.sol which has a spend() capable of externally calling vault.withdrawToken():

	function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
		Vault vault = Vault(getContractAddress("Vault"));
		decreaseGGPStake(stakerAddr, ggpAmt);
-		vault.transferToken("ProtocolDAO", ggp, ggpAmt);
+		vault.transferToken("ClaimProtocolDAO", ggp, ggpAmt);
	}

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L424-L426 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L557-L561 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L670-L683

Vulnerability details

Impact

According to the sponsor's clarification on discord, when creating a minipool,

"... a nodeOp can choose a duration they want, from 14 days to 365 days. But behind the scenes, Rialto will only create a validator for 14 days. At the end of the 14 days, the Avalanche protocol pays out the validation rewards and all funds are returned to the contracts, which divvy up the rewards between liq stakers and nodeops. If the duration still has time left to go, Rialto will call recreateMinipool which will do another 14 days cycle( if it can considering GGP collat and available liq staker funds), compounding the rewards. In this way, the protocol recvs yield from every minipool every 14 days, and the collat ratio of GGP can never get more than 14 days out of whack ..."

This could catch the node operator off guard who might be slashed pre-maturely for a GGP amount based off the duration.

Proof of Concept

Here is a typical secnario:

1. Bob created a minipool with an input duration of 365 days.
2. Everything went well as Rialto kept recreating the minipool for Bob with compoundedAvaxNodeOpAmt.
3. On the tenth cycle, Bob's node did not perform and ended up with avaxTotalRewardAmt == 0.
4. slash() was invoked basing off getExpectedAVAXRewardsAmt() that would link up the duration inputted by the node operator.

This is totally unfair to the node operator who should have been slashed for the amount based off 14 days instead of duration.

Tools Used

Manual inspection

Recommended Mitigation Steps

Conside refactoring the affected function as follows:

File: MinipoolManager.sol#L670-L683

	function slash(int256 index) private {
		address nodeID = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".nodeID")));
		address owner = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".owner")));
-		uint256 duration = getUint(keccak256(abi.encodePacked("minipool.item", index, ".duration")));
		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));
-		uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt);
+		uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(14 days, avaxLiquidStakerAmt);
		uint256 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);
		setUint(keccak256(abi.encodePacked("minipool.item", index, ".ggpSlashAmt")), slashGGPAmt);

		emit GGPSlashed(nodeID, slashGGPAmt);

		Staking staking = Staking(getContractAddress("Staking"));
		staking.slashGGP(owner, slashGGPAmt);
	}

Note: The hard coded 14 days parameter may have to be further refactored to a settable variable if need be.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L319-L323 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L328-L332 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L337-L354 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ClaimNodeOp.sol#L106

Vulnerability details

Impact

In Staking.sol, the modifier, whenNotPaused is applied on stakeGGP() and not on restakeGGP(). These two functions share the same function logic, but restakeGGP() is granted special privilege (via onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) modifier) capable of staking GGP tokens all the time while stakeGGP() is unable to execute if it has been paused. This creates unfair treatments in the protocol with the seasoned stakers granted the special path via claimAndRestake() in ClaimNodeOp.sol, particularly when:

• getGGPStake() < getMinimumGGPStake() due to GGP price drop and the 28 days reward cycle is about due.
• getGGPStake() < getMinimumGGPStake() due to GGP price drop and the 14 days validation cycle is about due for the next minipool recreation.
• ggAVAX.amountAvailableForStaking() could not meet the requested demand.

Specifically, it was going to be a time sensitive issue regardless of what situation would entail then.

Proof of Concept

File: Staking.sol

	function stakeGGP(uint256 amount) external whenNotPaused {
		// Transfer GGP tokens from staker to this contract
		ggp.safeTransferFrom(msg.sender, address(this), amount);
		_stakeGGP(msg.sender, amount);
	}

	function restakeGGP(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) {
		// Transfer GGP tokens from the ClaimNodeOp contract to this contract
		ggp.safeTransferFrom(msg.sender, address(this), amount);
		_stakeGGP(stakerAddr, amount);
	}

	function _stakeGGP(address stakerAddr, uint256 amount) internal {
		emit GGPStaked(stakerAddr, amount);

		// Deposit GGP tokens from this contract to vault
		Vault vault = Vault(getContractAddress("Vault"));
		ggp.approve(address(vault), amount);
		vault.depositToken("Staking", ggp, amount);

		int256 stakerIndex = getIndexOf(stakerAddr);
		if (stakerIndex == -1) {
			// create index for the new staker
			stakerIndex = int256(getUint(keccak256("staker.count")));
			addUint(keccak256("staker.count"), 1);
			setUint(keccak256(abi.encodePacked("staker.index", stakerAddr)), uint256(stakerIndex + 1));
			setAddress(keccak256(abi.encodePacked("staker.item", stakerIndex, ".stakerAddr")), stakerAddr);
		}
		increaseGGPStake(stakerAddr, amount);
	}

As can be seen from the code blocks above, _stakeGGP() will be internally invoked in stakeGGP() and restakeGGP() that are not equally privileged. Specifically, new node operators may be denied entries while those who have staked and earned rewards can snap up chances via the privileged path to start queuing and creating more minipools in advance for the next avaxLiquidStakerAmt match. Additionally, the latter get to top up the deficiency in GGP staked to make up for and/or increase the next rewards payout other than making sure recreateMinipool() is going to execute. Those who have opted to fulfill dao.getMinCollateralizationRatio() might be at a disadvantage when the critical moment of contract pausing took place.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider adding whenNotPaused to restakeGGP() in order to be fair to everyone. In fact, in my gas optimization report, I did suggest doing away with the redundant codes of stakeGGP() and restakeGGP() and making _stakeGGP() external with the unanimous whenNotPaused added.

Here is the suggested refactoring of _stakeGGP() and the associated claimAndRestake() in ClaimNodeOp.sol:

File: Staking.sol#L337-L354

-	function _stakeGGP(address stakerAddr, uint256 amount) internal {
+	function _stakeGGP(address stakerAddr, uint256 amount) external whenNotPaused {
+		ggp.safeTransferFrom(msg.sender, address(this), amount);
		emit GGPStaked(stakerAddr, amount);
                ...

File: ClaimNodeOp.sol#L106

-			staking.restakeGGP(msg.sender, restakeAmt);
+			staking._stakeGGP(msg.sender, restakeAmt);

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L152-L175 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L444-L478 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L298

Vulnerability details

Impact

recreateMinipool() in MinipoolManager.sol could successfully be executed with either currentStatus == MinipoolStatus.Withdrawable or currentStatus == MinipoolStatus.Finished when requireValidStateTransition() is invoked on line 446.

		requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);

If recreateMinipool() were to be called when currentStatus == MinipoolStatus.Finished, the function logic would still run till the end where MinipoolStatusChanged() was emitted. This was because none of the variables in Minipool struct was reset when withdrawMinipoolFunds() had been called by the node operator. Specifically, mp.avaxNodeOpRewardAmt which was way less than compoundedAvaxNodeOpAmt originated an issue that was going to be shown up later:

File: MinipoolManager.sol#L457

		staking.increaseAVAXStake(mp.owner, mp.avaxNodeOpRewardAmt);

Subsequently, claimAndInitiateStaking(), recordStakingStart(), and recordStakingEnd() could all be called by Rialto to complete another cycle of 14-day-validation.

Proof of Concept

Supposing the duration specified could only feature 2 cycles of validation.

And, here comes the problem.

If this was the only minipool the owner had, there would not be much of an issue when calling withdrawMinipoolFunds() since the function was bound to revert on line 298 when externally calling staking.decreaseAVAXStake() due to a negative integer returned.

                // @ audit mp.avaxNodeOpRewardAmt < avaxNodeOpAmt
		staking.decreaseAVAXStake(owner, avaxNodeOpAmt); 

However, if the owner possessed more than 1 minipool, say 2 in this case, and the other minipool was still running, withdrawMinipoolFunds() could successfully be executed with the owner transferred totalAvaxAmt. The consequence is the accounting of the other minipool would thus be messed up when attempting to call withdrawMinipoolFunds() later. If both minipools were of the same size of AVAX staked, it would still not be a big issue since there was presumably no fund loss incurred to the node operator. But if the first minipool was associated with 2_000 ether of AVAX staked whilst the second minippol entailed 20,000 ether of AVAX staked, there would be an 8,000 ether of AVAX loss to the owner.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider refactoring recreateMinipool() as follows:

   function recreateMinipool(address nodeID) external whenNotPaused {
   	int256 minipoolIndex = onlyValidMultisig(nodeID);
+		bytes32 statusKey = keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status"));
+		MinipoolStatus currentStatus = MinipoolStatus(getUint(statusKey));
+		if (currentStatus != MinipoolStatus.Withdrawable) {
+			revert InvalidStateTransition();
+		}
-		requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
   	Minipool memory mp = getMinipool(minipoolIndex);

               ...

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L271-L274 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L358-L374

Vulnerability details

Impact

As denoted by Known Issues pertaining to GGP Rewards Manipulation Before Distribution:

"This can only be taken advantage of when signing up for 2-4 week validation periods. Our protocol is incentivizing nodes to sign up for 3-12 month validation periods. ..."

Apparently, the protocol has implemented the 150% collateralization rule when node operators attempt to withdraw their GGP, serving as a measure to lock their GGP staked throughout the duration of the minipool. However, it could be timed such that withdrawGGP() is called when avaxAssigned == 0 before the minipool(s) is/are recreated. Hence, GGP rewards could be manipulated with nodes that are signed up for 14 - 365 days.

Proof of Concept

Here is the typical scenario:

1. Alice created 10 minipools that would have avaxAssignmentRequest == dao.getMinipoolMaxAVAXAssignment() on each createMinipool() call.
2. All 10 minipools were created at the same time so that they would start validating at the same time too assuming that ggAVAX.amountAvailableForStaking() was readily available.
3. She also made sure they were all created when startRewardsCycle() was successfully called in RewardsPool.sol.
4. 28 days later just before rewards would be distributed, she made sure dao.getMaxCollateralizationRatio() was fully utilized by increasing her GGP staked.
5. As soon as rewards calculateAndDistributeRewards() was successfully executed by Rialto in ClaimNodeOp.sol, she would call withdrawGGP() in Staking.sol while avaxAssigned == 0 before the minipools were yet to be recreated.

File: Staking.sol#L271-L274

		if (avaxAssigned == 0) {
			// Infinite collat ratio
			return type(uint256).max;
		}

File: Staking.sol#L368-L370

		if (getCollateralizationRatio(msg.sender) < dao.getMaxCollateralizationRatio()) {
			revert CannotWithdrawUnder150CollateralizationRatio();
		}

Note that with avaxAssigned == 0, getCollateralizationRatio(msg.sender) == type(uint256).max. This would allow Alice withdraw as much as 140% collateralization while keeping the other 10% for continuing her node validations.

Tools Used

Manual inspection

Recommended Mitigation Steps

This is going to be tricky, but consider refactoring the affected withdrawGGP() by limiting the withdrawal also based on minipool duration. This would involve adding nodeID or minipoolIndex to the Staker struct that could entail quite a lot of redesigning. But even that, this could only be minipool index specific which again would require a for loop to handle all indices entailed.

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

Unusual multisig logic

Open TODOs

Open TODOs can point to architecture or programming issues that still need to be resolved. Consider resolving them before deploying.

Here are the instances entailed:

File: BaseAbstract.sol#L6-L8

// TODO remove this when dev is complete
// import {console} from "forge-std/console.sol";
// import {format} from "sol-utils/format.sol";

File: Staking.sol#L203

	// TODO cant use onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) since we also call from increaseMinipoolCount. Wat do?

File: MinipoolManager.sol#L412

		// TODO Revisit this logic if we ever allow unequal matched funds

Modularity on import usages

For cleaner Solidity code in conjunction with the rule of modularity and modular programming, use named imports with curly braces instead of adopting the global import approach. Throughout the code bases, most of the imports are adopting the recommended approach but some are not. Consider synchronizing them to the recommended method.

Here are the instances entailed:

File: Base.sol#L4 File: BaseUpgradeable.sol#L4

- import "./BaseAbstract.sol";
+ import { BaseAbstract} from "./BaseAbstract.sol";

File: ClaimNodeOp.sol

- import "./Base.sol";
+ import {Base} from "./Base.sol";

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Here is a contract instance with missing NatSpec in its entirety:

File: BaseUpgradeable.sol

Typo mistakes

File: MinipoolManager.sol#L309

-	/// @notice Verifies that the minipool related the the given node ID is able to a validator
+	/// @notice Verifies that the minipool related to the given node ID is able to become a validator

File: MinipoolManager.sol#L556

-	/// @return The approximate rewards the node should recieve from Avalanche for beign a validator
+	/// @return The approximate rewards the node should recieve from Avalanche for being a validator

5% annual calculated on a daily interval not fully precised

File: ProtocolDAO.sol#L41

The max safe integer for JavaScript without losing precision is (2^53 – 1), which is around 9 quadrillion. As such, the code line linked above that entails 19 digits will not be fully accurate.

The calculated number should be 1,000,133,680,617,113,440 instead of 1,000,133,680,617,113,500.

Note: BigInt((1 + 0.05) ** (1 / (365)) * 1e18) would also yield an inaccurate figure, 1000133680617113472n.

dao.getInflationIntervalRate(), i.e. 1000133680617113500, is currently used to calculate the inflation amount in the code line below:

File: RewardsPool.sol#L75

			newTotalSupply = newTotalSupply.mulWadDown(inflationRate);

It has no impact in the protocol with the last three differing digits since the total supply of GGP is comparatively lowly capped at 22,500,000. It could be an issue elsewhere if the a different reward token were to be implemented with its total supply capped at a much higher value, e.g. in quadrillion like Shiba Inu.

Hard coded initialization

In ProtocolDAO.sol, initialize() could only be called once by onlyGuardian because of the bool logic in lines 24 - 27. All storage variables are hard coded with their values retrievable via the getters. Only TotalGGPCirculatingSupply, ClaimingContractPct, and ExpectedAVAXRewardsRate have their respective setters implemented in the contract.

Consider implementing setters for all of the storage variables initialized where possible so that the community will have more proposal options when the protocol go full DAO. Otherwise, a proposal for changing dao.getInflationIntervalRate(), currently hard coded to 1000133680617113500, for an example, would not be viable if there was no setter function catered for it.

Inexpedient ternary logic

The ternary code line below:

File: ProtocolDAO.sol#L118

		return rate < 1 ether ? 1 ether : rate;

is deemed impractical considering 1 ether, which is 1e18, is going to make the fraction of the following arithmetic calculation always equal to 1:

File: RewardsPool.sol#L75

			newTotalSupply = newTotalSupply.mulWadDown(inflationRate);

With 1 ether / WAD == 1, GGP would never be able to inflate and ggp.totalSupply() would forever stay at the initialized value of 18,000,000. Additionally, running a GGP rewards cycle via startRewardsCycle() would also be impossible because the function would readily revert on the second if block due to newTokens == 0.

There is no risk foreseeable since InflationIntervalRate is currently hard coded to 5 % annual. It would have to be refactored to assume something higher than 1 ether if (rate < 1 ether) == false if a setter for InflationIntervalRate was going to be implemented in the contract.

Custom contract pauser and resumer

First off, pauseEverything() and resumeEverything() have both their names belie their function logic catering selectively to important contracts instead of everything (or as it implies, every contract). Consider implementing a custom contract pauser and resumer in Ocyticus.sol so that the defenders get to work on other contracts rather than just "TokenggAVAX", "MinipoolManager", and "Staking".

Here is what the suggested functions could look like:

	function pauseContract (string memory _contract) external onlyDefender {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		dao.pauseContract(_contract);
		disableAllMultisigs();
	}

	function resumeContract (string memory _contract) external onlyDefender {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		dao.resumeContract(_contract);
	}

Unusual multisig logic

Conventionally, multi-signature requires the agreement of multiple people to perform an action, e.g. 6 out of 10 in the case associated with the protocol. However, requireNextActiveMultisig() in MultisigManager.sol only requires one valid, i.e registered and enabled Multisig, address to interact with MinipoolManager.sol.

Additionally, the function logic of requireNextActiveMultisig() seems to be mostly (if not only) dealing with the first enabled multisig's index, making the other enabled indices never reachable unless the first one has been compromised and disabled.

File: MultisigManager.sol#L80-L91

	function requireNextActiveMultisig() external view returns (address) {
		uint256 total = getUint(keccak256("multisig.count"));
		address addr;
		bool enabled;
		for (uint256 i = 0; i < total; i++) {
			(addr, enabled) = getMultisig(i);
			if (enabled) {
				return addr;
			}
		}
		revert NoEnabledMultisigFound();
	}

Comments and codes mismatch

The comment lines below said that 2% is 0.2 ether. It should be 2% is 0.02 ether or 20% is 0.2 ether.

File: MinipoolManager.sol#L35

-	minipool.item<index>.delegationFee = node operator specified fee (must be between 0 and 1 ether) 2% is 0.2 ether
+	minipool.item<index>.delegationFee = node operator specified fee (must be between 0 and 1 ether) 2% is 0.02 ether

File: MinipoolManager.sol#L194

-	/// @param delegationFee Percentage delegation fee in units of ether (2% is 0.2 ether)
+	/// @param delegationFee Percentage delegation fee in units of ether (20% is 0.2 ether)

Similarly, the comment below said that it is looking up multisig index by minipool nodeID when only the multisig address assigned to the minipool is entailed:

File: MinipoolManager.sol#L124

-	/// @dev Look up multisig index by minipool nodeID
+	/// @dev Look up minipool index via assigned multisig address by minipool nodeID

GGP token exchange

GGP is initialized with TotalGGPCirculatingSupply == 18_000_000 ether. However, an exchange is needed to at least allow node operators to swap AVAX or ggAVAX into GGP. Devoid of this, no one would be able to secure any GGP to stake prior to creating a minipool.

Additionally, with an exchange in place, liquid stakers would be able to sell the slashed GGP awarded at their discretion instead of being dictated by Protocol DAO.

GGP tokens circulated distributions

It is not absolutely clear where and how GGP tokens are being circulated. Upon having TokenGGP.sol deployed, a fixed supply of 22_500_000 tokens were minted to the deployer. The protocol should clearly document when and how the initial 18_000_000 tokens were being distributed, e.g. pre-sales, airdrops, deFi exchange etc. And when the tokens got inflated, it should be transparent how the protocol was going to have proper accounting catered for contracts holding the additional GGP tokens. For instance, upon successfully executing startRewardsCycle() in RewardsPool.sol, how would Vault.sol GGP contract balance be updated to allow the transfer/withdrawal pertaining to "ClaimMultisig", "ClaimNodeOp", and "ClaimProtocolDAO"?

Missing use for delegationFee

delegationFee is introduced in MinipoolManager.sol. However, no where in the contract or the protocol could be found any use for this variable that has been included in the struct, Minipool. Additionally, there seems to be no boundary control on the input parameter of delegationFee in createMinipool(). This could impose a problem devoid of a yardstick to determine what percentage delegation fee in units of ether is deemed fair and appropriate on avaxAssignmentRequest. If delegationFee is not ready to be fully introduced yet, consider elaborating a brief structured plan for it in the NatSpec or the documentations since this constitutes one of the triple incentives to the node operators.

Ironically, MinipoolNodeCommissionFeePct is found to be deducting a cut from avaxHalfRewards prior to getting avaxLiquidStakerRewardAmt assigned. Apparently, the documentation might have to be updated to reflect a four fold incentives to the node operators.

No storage gap for upgradeable contracts

Consider adding a storage gap at the end of an upgradeable contract, just in case it would entail some child contracts in the future. This would ensure no shifting down of storage in the inheritance chain.

Here is the contract instance entailed:

File: TokenggAVAX.sol

+ uint256[50] private __gap;

Variable assignment in conditional checks

Making a variable assignment in a conditional statement deviates from the standard use and intention of the check and can easily lead to confusion.

Consider moving the needed assignment before the conditional statement by having the code lines below rewritten as follows:

File: TokenggAVAX.sol#L169

-		if ((shares = previewDeposit(assets)) == 0) {
+		shares = previewDeposit(assets);
+		if (shares == 0) {

File: TokenggAVAX.sol#L193

-		if ((assets = previewRedeem(shares)) == 0) {
+		assets = previewRedeem(shares);
+		if (assets == 0) {

Zero value check on `withdrawAVAX() in TokenggAVAX.sol

Although previewWithdraw() does round up, it could still assign a zero value to shares if the input parameter, assets is accidentally entered as zero. Consider having a zero value check implemented just as it has been done so on redeemAVAX() which is nonetheless a side effect zero value check arising from rounding error check associated with round down issue in previewRedeem().

File: TokenggAVAX.sol#L180-L189

	function withdrawAVAX(uint256 assets) public returns (uint256 shares) {
+		if (assets == 0) {
+			revert ZeroAssets();

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

Parameterized custom errors

Consider parameterizing custom errors where deemed fit to make them more distinct.

For an example, the following custom error instance may be refactored as follows:

File: BaseAbstract.sol#L17

-	error MustBeGuardianOrValidContract();
+	error MustBeGuardianOrValidContract(address caller, bool authorization);

File: BaseAbstract.sol#L40-L48

	modifier guardianOrRegisteredContract() {
		bool isContract = getBool(keccak256(abi.encodePacked("contract.exists", msg.sender)));
		bool isGuardian = msg.sender == gogoStorage.getGuardian();

		if (!(isGuardian || isContract)) {
-			revert MustBeGuardianOrValidContract();
+			revert MustBeGuardianOrValidContract(msg.sender, isGuardian);
		}
		_;
	}

bytes32 over bytes

bytes32 typed variables are fixed-sized and can be passed between contracts. bytes typed variables, on the contrary, are dynamically sized and special array, i.e. a shorthand for byte[] according to Solidity Documentation. They are not value type, but can entail push (append a byte to the end), pop, and length.

Some possibly disorienting situations are possible if bytes is used as a function argument and the contract successfully compiles The instances entailed in the protocol are not at risk, but care will have to be given elsewhere by always using fixed length types for any function that will be called from outside:

File: Storage.sol#L80

	function getBytes(bytes32 key) external view returns (bytes memory) {

Empty blocks

Function with empty block should have a comment explaining why it is empty. For instance, receiveWithdrawalAVAX() in MiniPoolManager.sol should be commented as receiving AVAX from TokenggAVAX.sol and Vault.sol to better portray its intended purpose:

File: MinipoolManager.sol#L106

+	/// @notice Receive AVAX from TokenggAVAX.sol and Vault.sol
	function receiveWithdrawalAVAX() external payable {}