GoGoPool contest - rvierdiiev's results

Lines of code

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

Vulnerability details

Impact

Slashed ggp tokens should be distributed with liquidity providers, but they are sent to ProtocolDAO. As result, liquidity providers are not compensated and received no rewards for lending avax to slashed minipool.

Proof of Concept

By documentation of protocol if validator is slashed, then some amount of his locked GGP tokens should be distributed to liquidity providers to compensate their lost rewards.

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/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);
	}

As you can see currently slashed amount is sent directly to vault and controlled by ProtocolDAO. And there is no place in the code where this slashed ggp tokens are distributed among liquidity providers, there is no even calculation of total slashed amount that should be used to distribute tokens.

As result, liquidity providers are not compensated and received no rewards for lending avax to minipool.

Tools Used

VsCode

Recommended Mitigation Steps

You need to add mechanism to distribute slashed ggp awards among liquidity providers.

Lines of code

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

Vulnerability details

Impact

Because it's possible for owner of minipool to call createMinipool function after recordStakingEnd was called and minipool status became Withdrawable, owner can loose earned validator rewards and operator staked amount.

Proof of Concept

createMinipool function can be called if current minipool status is Withdrawable, Error, Finished or Canceled. It's checked using requireValidStateTransition function. https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L152-L175

	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 {
			isValid = false;
		}


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

It's possible to call createMinipool function for both existing and new nodeId. https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L242-L263

		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);
		setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".multisigAddr")), multisig);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpInitialAmt")), msg.value);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), msg.value);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), avaxAssignmentRequest);

In case if it was called for existing minipool then resetMinipoolData will be called, which will reset some minipool stored data. https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L687-L696

	function resetMinipoolData(int256 index) private {
		setBytes32(keccak256(abi.encodePacked("minipool.item", index, ".txID")), 0);
		setUint(keccak256(abi.encodePacked("minipool.item", index, ".startTime")), 0);
		setUint(keccak256(abi.encodePacked("minipool.item", index, ".endTime")), 0);
		setUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxTotalRewardAmt")), 0);
		setUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxNodeOpRewardAmt")), 0);
		setUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerRewardAmt")), 0);
		setUint(keccak256(abi.encodePacked("minipool.item", index, ".ggpSlashAmt")), 0);
		setBytes32(keccak256(abi.encodePacked("minipool.item", index, ".errorCode")), 0);
	}

As you can see avaxNodeOpRewardAmt value will be reset to 0. Later avaxNodeOpAmt param will be set to msg.value.

We will consider case when createMinipool for existing minipool is called after recordStakingEnd function set minipool status to Withdrawable. Also this function set avaxNodeOpRewardAmt param to the value that should be returned to owner as reward.

By design, after recordStakingEnd function is called, then owner should call withdrawMinipoolFunds function which will send him rewards and staked avax amount. But if owner will call createMinipool function for any reason then as result his avaxNodeOpRewardAmt param will be reset and avaxNodeOpAmt will be set for new value. As result user can't call withdrawMinipool anymore and also his rewards and previously staked avax amount are cleared, he lost them.

This is a test that creates minipool, then after it's withdrawable creates it again and lose rewards and previous avax stake. Add this test to MinipoolManager.t.sol file.

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

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

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

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

		skip(duration);

		uint256 rewards = 10 ether;
		uint256 halfRewards = rewards / 2;
		deal(address(rialto), address(rialto).balance + rewards);
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, rewards);
		uint256 percentage = dao.getMinipoolNodeCommissionFeePct();
		uint256 commissionFee = (percentage).mulWadDown(halfRewards);
		vm.stopPrank();

		int256 index = minipoolMgr.getIndexOf(mp1.nodeID);
		mp1 = minipoolMgr.getMinipool(index);
		//owner provided avaxNodeOpAmt and got 5750000000000000000 as reward
		assertEq(mp1.avaxNodeOpAmt, depositAmt);
		assertEq(mp1.avaxNodeOpRewardAmt, 5750000000000000000);

		vm.startPrank(nodeOp);
		//now owner creates minipool again with same node and deposited same amount
		minipoolMgr.createMinipool{value: depositAmt}(mp1.nodeID, duration, 0.02 ether, avaxAssignmentRequest);
		index = minipoolMgr.getIndexOf(mp1.nodeID);
		MinipoolManager.Minipool memory mp2 = minipoolMgr.getMinipool(index);

		assertEq(mp1.nodeID, mp2.nodeID);
		//his avaxNodeOpAmt is depositAmt, but should be 2 * depositAmt
		assertEq(mp2.avaxNodeOpAmt, depositAmt);
		//his rewards are 0 now
		assertEq(mp2.avaxNodeOpRewardAmt, 0);
		
		vm.expectRevert();
		//he can't withdraw anymore
		minipoolMgr.withdrawMinipoolFunds(mp1.nodeID);
	}

Tools Used

VsCode

Recommended Mitigation Steps

Do not allow transition to Prelaunch from Withdrawable status.

Lines of code

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

Vulnerability details

Impact

Share price manipulation by first depositor of TokenggAVAX is possible. Next depositors will be losing funds.

Proof of Concept

When TokenggAVAX is deployed first depositor can deposit 1 wei using depositAVAX function. As result 1 share will be minted for him. The price of share will be 1 wei. Then he will transfer big amount of WAVAX directly to TokenggAVAX contract. As result price of that 1 share will increase to the amount that he donated.

Then all next depositors should deposit big amount of funds to be able to get shares. And also all amount that is victimDepositAmount%sharePrice != 0 will be lost by depositors, because of rounding while calculating shares, while attacker will benefit of it, as his assets count will be increasing.

Tools Used

VsCode

Recommended Mitigation Steps

First depositor should not be able to call depositAVAX function with small msg.value.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L528-L533

Vulnerability details

Impact

Rialto can call finishFailedMinipoolByMultisig when minipool is not in Error state. As result minipool owner will not be able to withdraw minipool funds.

Proof of Concept

finishFailedMinipoolByMultisig is going to be used to change state of minipool that faced with error to Finished. https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L528-L533

	function finishFailedMinipoolByMultisig(address nodeID) external {
		int256 minipoolIndex = onlyValidMultisig(nodeID);
		requireValidStateTransition(minipoolIndex, MinipoolStatus.Finished);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Finished));
		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Finished);
	}

requireValidStateTransition(minipoolIndex, MinipoolStatus.Finished) is checking if such transition is allowed. And indeed it is allowed when minipool is in Error state, but it is allowed for Withdrawable state as well.

So it's possible that rialto will call finishFailedMinipoolByMultisig function for minipool that has no error and earned rewards. Because of that owner of minipool will not be able to call withdrawMinipoolFunds and return staked amount and rewards.

This test shows that indeed it's possible for rialto to call finishFailedMinipoolByMultisig when minipool is in Withdrawable state. Add it to MinipoolManager.t.sol.

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

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

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

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

		skip(duration);

		uint256 rewards = 10 ether;
		uint256 halfRewards = rewards / 2;
		deal(address(rialto), address(rialto).balance + rewards);
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, rewards);
		uint256 percentage = dao.getMinipoolNodeCommissionFeePct();
		uint256 commissionFee = (percentage).mulWadDown(halfRewards);
		vm.stopPrank();

		vm.prank(address(rialto));
		//it's withdrawable now, but rialto marked it as finished
		minipoolMgr.finishFailedMinipoolByMultisig(mp1.nodeID);
		
		vm.prank(address(nodeOp));
		vm.expectRevert();
		//he can't withdraw anymore
		minipoolMgr.withdrawMinipoolFunds(mp1.nodeID);
	}

Tools Used

VsCode

Recommended Mitigation Steps

Check that minipool state is Error when calling finishFailedMinipoolByMultisig.

Lines of code

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/ClaimNodeOp.sol#L106

Vulnerability details

Impact

Staking.restakeGGP should not be allowed when contract is paused. Even that it's only callable by ClaimNodeOp, it's still can be directly called by user through ClaimNodeOp.claimAndRestake.

Proof of Concept

When Staking contract is paused, then staking and withdrawing of GGP is disallowed. So both stakeGGP and withdrawGGP functions have whenNotPaused modifier. Also there is another possibility to stake GGP using Staking.restakeGGP function. It's only callable by ClaimNodeOp. And this function doesn't have whenNotPaused modifier. https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L328-L332

	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);
	}

restakeGGP can be called only by ClaimNodeOp. It is called inside ClaimNodeOp.claimAndRestake function. https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ClaimNodeOp.sol#L89-L114

	function claimAndRestake(uint256 claimAmt) external {
		Staking staking = Staking(getContractAddress("Staking"));
		uint256 ggpRewards = staking.getGGPRewards(msg.sender);
		if (ggpRewards == 0) {
			revert NoRewardsToClaim();
		}
		if (claimAmt > ggpRewards) {
			revert InvalidAmount();
		}


		staking.decreaseGGPRewards(msg.sender, ggpRewards);


		Vault vault = Vault(getContractAddress("Vault"));
		uint256 restakeAmt = ggpRewards - claimAmt;
		if (restakeAmt > 0) {
			vault.withdrawToken(address(this), ggp, restakeAmt);
			ggp.approve(address(staking), restakeAmt);
			staking.restakeGGP(msg.sender, restakeAmt);
		}


		if (claimAmt > 0) {
			vault.withdrawToken(msg.sender, ggp, claimAmt);
		}


		emit GGPRewardsClaimed(msg.sender, claimAmt);
	}

As you can see this function can be called by anyone who has ggp rewards. So using ClaimNodeOp.claimAndRestake user can dismiss restriction for staking ggp when Staking contract is paused.

Tools Used

VsCode

Recommended Mitigation Steps

Add whenNotPaused modifier to Staking.restakeGGP function.

Lines of code

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

Vulnerability details

Impact

RewardsPool.inflate sets wrong value to RewardsPool.InflationIntervalStartTime variable. As result inflation logic will be broken and GGP token will inflate much faster than it was expected.

Proof of Concept

GGP token has limited total supply that is minted on deploy. But at the beginning not all tokens are available. Token amount should inflate during the time to distribute its total supply.

To increase amount of tokens in circulation RewardsPool.startRewardsCycle function is called. This function then calls inflate function.

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/RewardsPool.sol#L82-L100

	function inflate() internal {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		uint256 inflationIntervalElapsedSeconds = (block.timestamp - getInflationIntervalStartTime());
		(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);
	}

This function calculates how many periods has passed since last inflate and then increases token's total supply. It uses getInflationIntervalStartTime function to know when inflation was done last time. getInflationIntervalStartTime function just checks InflationIntervalStartTime variable.

Later inflate function is updating InflationIntervalStartTime variable to provide time when last inflation was done. addUint(keccak256("RewardsPool.InflationIntervalStartTime"), inflationIntervalElapsedSeconds); The problem is that inflationIntervalElapsedSeconds is set instead of block.timestamp. inflationIntervalElapsedSeconds is a time in seconds between the last inflation was done. It's not a timestamp. As a result when inflate will be called next time, then getInflationAmt function will calculate new tokens incorrectly and will distribute all total supply more faster.

Example. 1.RewardsPool is deployed at time 1000. Then InflationIntervalStartTime == 1000. 2.Inflation interval is 10. 3.inflate is called for first time at time 1010. As result price has increased once. InflationIntervalStartTime == inflationIntervalElapsedSeconds == 10. 4.inflate is called next time at time 1020. inflationIntervalElapsedSeconds = 1020 - 10 = 1010. And price has changed 101 times instead of 1 as getInflationAmt function decided that 101 period has passed since last inflation.

Tools Used

VsCode

Recommended Mitigation Steps

Set block.timestamp to inflationIntervalElapsedSeconds variable.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L665 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ClaimNodeOp.sol#L56-L85

Vulnerability details

Impact

MinipoolManager._cancelMinipoolAndReturnFunds doesn't reset rewardsStartTime for staker if it has no more minipool

Proof of Concept

When new minipool is created for staker then his minipoolCount is increased and rewardsStartTime is set to current time if was not set before. This rewardsStartTime is then used in ClaimNodeOp.isEligible function which allows to calculate if staker should receive rewards. https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ClaimNodeOp.sol#L46-L52

	function isEligible(address stakerAddr) external view returns (bool) {
		Staking staking = Staking(getContractAddress("Staking"));
		uint256 rewardsStartTime = staking.getRewardsStartTime(stakerAddr);
		uint256 elapsedSecs = (block.timestamp - rewardsStartTime);
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		return (rewardsStartTime != 0 && elapsedSecs >= dao.getRewardsEligibilityMinSeconds());
	}

Later, based on result of isEligible, rialto will distribute rewards to eligible stakers. When staker cancels minipool then _cancelMinipoolAndReturnFunds function is called which decreases minipool count, but do not reset rewardsStartTime for staker if it was his only minipool. https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L646-L665

	function _cancelMinipoolAndReturnFunds(address nodeID, int256 index) private {
		requireValidStateTransition(index, MinipoolStatus.Canceled);
		setUint(keccak256(abi.encodePacked("minipool.item", index, ".status")), uint256(MinipoolStatus.Canceled));


		address owner = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".owner")));
		uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxNodeOpAmt")));
		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));


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


		staking.decreaseMinipoolCount(owner);


		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Canceled);


		Vault vault = Vault(getContractAddress("Vault"));
		vault.withdrawAVAX(avaxNodeOpAmt);
		owner.safeTransferETH(avaxNodeOpAmt);
	}

As result rewardsStartTime is not 0 and after 14 days staker that doesn't stake really will receive rewards. After that because it was last minipool, rewardsStartTime will be reset to 0.

So this is how staker can exploit this. 1.Attacker creates minipool. 2.After some time attacker cancels minipool. 3.Attacker receives rewards for 1 period, without staking.

Tools Used

VsCode

Recommended Mitigation Steps

Inside _cancelMinipoolAndReturnFunds function check that this is last minipool. And if yes, then set rewardsStartTime to 0 as user should not receive rewards for the period.

Lines of code

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

Vulnerability details

Impact

Users can sandwich syncRewards function to deposit, receive rewards and then withdraw.

Proof of Concept

Liquidity providers of TokenggAVAX are rewarded during the time. When syncRewards function is called then portion of rewards is added to circulation. So in case if some funds were distributed that means that if you had 1 share in TokenggAVAX then after syncRewards was called the amount of assets that you can get for this share has increased.

This allows attacker to earn rewards even without locking his funds.

Attack scenario. 1.syncRewards is called and new rewards will be distributed. 2.attacker front runs syncRewards and calls depositAVAX with big amount of funds X. 3.then he calls withdrawAVAX and as result he receives X+pertOfRewards. 4.as result attacker didn't do nothing but received rewards.

Tools Used

VsCode

Recommended Mitigation Steps

You need to have mechanism that will push depositors to lock their funds for a some minimum time. So they can't receive rewards for nothing.

Lines of code

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

Vulnerability details

Impact

TokenggAVAX.totalAssets calculates assets incorrectly when lastSync != previous rewardsCycleEnd.

Proof of Concept

TokenggAVAX.syncRewards is going to distribute new rewards during rewardsCycleLength period. Anyone can call syncRewards and the time when it was called will be set as lastSync variable.

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

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


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


		uint192 lastRewardsAmt_ = lastRewardsAmt;
		uint256 totalReleasedAssets_ = totalReleasedAssets;
		uint256 stakingTotalAssets_ = stakingTotalAssets;


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


		// Ensure nextRewardsCycleEnd will be evenly divisible by `rewardsCycleLength`.
		uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;


		lastRewardsAmt = nextRewardsAmt.safeCastTo192();
		lastSync = timestamp;
		rewardsCycleEnd = nextRewardsCycleEnd;
		totalReleasedAssets = totalReleasedAssets_ + lastRewardsAmt_;
		emit NewRewardsCycle(nextRewardsCycleEnd, nextRewardsAmt);
	}

It's not guaranteed that syncRewards function will be called exactly at rewardsCycleEnd time. That means that it's possible that at the time when syncRewards function was called lastSync is set to time that is bigger than previous rewardsCycleEnd. For example rewardsCycleEnd is 100, rewardsCycleLength is 10. So if syncRewards is called in time 109, then lastSync is set to 109, and nextRewardsCycleEnd is 110.

Function totalAssets is used in contract to calculate amount of assets that are available at the moment. This function is used in many another functions that work with shares. https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/TokenggAVAX.sol#L113-L130

		function totalAssets() public view override returns (uint256) {
		// cache global vars
		uint256 totalReleasedAssets_ = totalReleasedAssets;
		uint192 lastRewardsAmt_ = lastRewardsAmt;
		uint32 rewardsCycleEnd_ = rewardsCycleEnd;
		uint32 lastSync_ = lastSync;


		if (block.timestamp >= rewardsCycleEnd_) {
			// no rewards or rewards are fully unlocked
			// entire reward amount is available
			return totalReleasedAssets_ + lastRewardsAmt_;
		}


		// rewards are not fully unlocked
		// return unlocked rewards and stored total
		uint256 unlockedRewards = (lastRewardsAmt_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);
		return totalReleasedAssets_ + unlockedRewards;
	}

In case if block.timestamp >= rewardsCycleEnd_ then all rewards that are prepared for this period are added to total assets. But in case if condition is false then next formula is used (lastRewardsAmt_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_). The problem is that this formula takes into account lastSync_ param and not previous rewardsCycleEnd_. As result function calculates smaller amount of totalAssets that it should.

For example if rewardsCycleEnd is 110, lastSync is set to 109, and totalAssets function is called at time 109 that means that 9/10 of lastRewardsAmt_ should be added to totalAssets, but formula will add nothing in this case.

So here is the result of such calculations. When user calls withdrawAVAX at time 109 he receives less amount of tokens as rewards that are calculated during this cycle are not added to total assets. When syncRewards is called exactly in rewardsCycleEnd timestamp then user that withdraws funds will receive more assets.

Let's consider 2 scenarios. Input: rewardsCycleEnd = 100, rewardsCycleLength = 10. 1.syncRewards is called at sync=100. then at time 109 function totalAssets will calculate unlockedRewards as 9/10 of lastRewardsAmt_. 2.syncRewards is called at sync=109. then at time 109 function totalAssets will calculate unlockedRewards as 0. In second scenario totalAssets is less then in 1 scenario. But they should be equal.

Tools Used

VsCode

Recommended Mitigation Steps

You need to linearly increase rewards from cycle start, not sync call.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L484-L515 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ClaimNodeOp.sol#L56-L85

Vulnerability details

Impact

MinipoolManager.recordStakingError doesn't decrease staker's minipool count which allows staker to receive ggp rewards forever.

Proof of Concept

When new minipool is created for staker then his minipoolCount is increased and rewardsStartTime is set to current time if was not set before. This rewardsStartTime is then used in ClaimNodeOp.isEligible function which allows to calculate if staker should receive rewards.

While minipoolCount variable is used inside ClaimNodeOp.calculateAndDistributeRewards function to set rewardsStartTime to 0 in case if staker doesn't have more minipools. Once it is set to 0 then isEligible function will return false and staker stops receiving rewards.

When minipool is finished(recorded end) then staker's minipoolCount is decreased. This means that user will be able to receive rewards for that period and in case if it was last minipool, then rewardsStartTime will be set to 0, so he will not receive rewards for next period.

Now, let's consider recordStakingError function which is called by rialto to signal that staking failed and return funds to staker. https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L484-L515

	function recordStakingError(address nodeID, bytes32 errorCode) external payable {
		int256 minipoolIndex = onlyValidMultisig(nodeID);
		requireValidStateTransition(minipoolIndex, MinipoolStatus.Error);


		address owner = getAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")));
		uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")));
		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")));


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


		setBytes32(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".errorCode")), errorCode);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Error));
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxTotalRewardAmt")), 0);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpRewardAmt")), 0);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerRewardAmt")), 0);


		// Send the nodeOps AVAX to vault so they can claim later
		Vault vault = Vault(getContractAddress("Vault"));
		vault.depositAVAX{value: avaxNodeOpAmt}();


		// Return Liq stakers funds
		ggAVAX.depositFromStaking{value: avaxLiquidStakerAmt}(avaxLiquidStakerAmt, 0);


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


		subUint(keccak256("MinipoolManager.TotalAVAXLiquidStakerAmt"), avaxLiquidStakerAmt);


		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Error);
	}

You will notice that this function doesn't decreased minipool count for staker and as result he will be able to get rewards for this minipool and also as this minipool "is lost", his minipool count will never be 0, so rewardsStartTime will never be set to 0 and he will receive rewards forever.

Problem scenario. 1.Staker created minipool. Minipool count is 1. 2.Error occured when staking, so recordStakingError was called and user received funds back. Minipool count is 1. 3.Staker never tried to create new minipool again, but was eligible for rewards always.

Tools Used

VsCode

Recommended Mitigation Steps

Inside recordStakingError function decrease minipool count for owner of minipool.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/RewardsPool.sol#L188 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/RewardsPool.sol#L229

Vulnerability details

Impact

RewardsPool.startRewardsCycle function will revert when there is no enabled multisig because of division by 0. As result it will be not possible to distribute rewards.

Proof of Concept

RewardsPool.startRewardsCycle function is called to distribute rewards among dao, multisig and operators.

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/RewardsPool.sol#L156-L197

	function startRewardsCycle() external {
		if (!canStartRewardsCycle()) {
			revert UnableToStartRewardsCycle();
		}


		emit NewRewardsCycleStarted(getRewardsCycleTotalAmt());


		// Set start of new rewards cycle
		setUint(keccak256("RewardsPool.RewardsCycleStartTime"), block.timestamp);
		increaseRewardsCycleCount();
		// Mint any new tokens from GGP inflation
		// This will always 'mint' (release) new tokens if the rewards cycle length requirement is met
		// 		since inflation is on a 1 day interval and it needs at least one cycle since last calculation
		inflate();


		uint256 multisigClaimContractAllotment = getClaimingContractDistribution("ClaimMultisig");
		uint256 nopClaimContractAllotment = getClaimingContractDistribution("ClaimNodeOp");
		uint256 daoClaimContractAllotment = getClaimingContractDistribution("ClaimProtocolDAO");
		if (daoClaimContractAllotment + nopClaimContractAllotment + multisigClaimContractAllotment > getRewardsCycleTotalAmt()) {
			revert IncorrectRewardsDistribution();
		}


		TokenGGP ggp = TokenGGP(getContractAddress("TokenGGP"));
		Vault vault = Vault(getContractAddress("Vault"));


		if (daoClaimContractAllotment > 0) {
			emit ProtocolDAORewardsTransfered(daoClaimContractAllotment);
			vault.transferToken("ClaimProtocolDAO", ggp, daoClaimContractAllotment);
		}


		if (multisigClaimContractAllotment > 0) {
			emit MultisigRewardsTransfered(multisigClaimContractAllotment);
			distributeMultisigAllotment(multisigClaimContractAllotment, vault, ggp);
		}


		if (nopClaimContractAllotment > 0) {
			emit ClaimNodeOpRewardsTransfered(nopClaimContractAllotment);
			ClaimNodeOp nopClaim = ClaimNodeOp(getContractAddress("ClaimNodeOp"));
			nopClaim.setRewardsCycleTotal(nopClaimContractAllotment);
			vault.transferToken("ClaimNodeOp", ggp, nopClaimContractAllotment);
		}
	}

To send rewards to multisig, distributeMultisigAllotment function is called. https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/RewardsPool.sol#L203-L233

	function distributeMultisigAllotment(
		uint256 allotment,
		Vault vault,
		TokenGGP ggp
	) internal {
		MultisigManager mm = MultisigManager(getContractAddress("MultisigManager"));


		uint256 enabledCount;
		uint256 count = mm.getCount();
		address[] memory enabledMultisigs = new address[](count);


		// there should never be more than a few multisigs, so a loop should be fine here
		for (uint256 i = 0; i < count; i++) {
			(address addr, bool enabled) = mm.getMultisig(i);
			if (enabled) {
				enabledMultisigs[enabledCount] = addr;
				enabledCount++;
			}
		}


		// Dirty hack to cut unused elements off end of return value (from RP)
		// solhint-disable-next-line no-inline-assembly
		assembly {
			mstore(enabledMultisigs, enabledCount)
		}


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

This function will collect all enabled multisigs using MultisigManager contract. Then it will distribute rewards equally to multisigs. To calculate amount that will be sent to each multisig next formula is used. uint256 tokensPerMultisig = allotment / enabledCount;

The problem here is that in case when no enabled multisig is present enabledCount will be 0 and the function will revert with division by 0. As result the whole startRewardsCycle function will revert and dao and node operators will not receive rewards.

Tools Used

VsCode

Recommended Mitigation Steps

In case when no enabled multisigs at the moment send tokens to the vault to special contract.