Underflow causes users funds to be temporarily frozen until next rewards cycle

[code423n4]

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.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #317

[c4-judge]

GalloDaSballo marked the issue as satisfactory