GoGoPool contest - Franfran's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L673 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L34 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ProtocolDAO.sol#L30

Vulnerability details

Impact

In the current implementation of the MinipoolManager, when a minipool node gets slahed because it didn't had a sufficient uptime during validation, the slashing amount calculation is based on the full node validation duration while it should only be based on the time interval of the cycles, meaning 14 days as per the Notion documentation:

Flow 

1. Liquid stakers deposit AVAX in exchange for ggAVAX
2. When there are sufficient funds deposited and a minipool to match, AVAX is withdrawn by the minipool for staking. 
3. After the staking period, AVAX used for staking and Avalanche rewards are deposited back to the ggAVAX contract. 
4. Rewards are reflected in the exchange rate of ggAVAX → AVAX and are streamed over a 14 day period. 

In the slash() function, it seems like the duration that is taken to estimate the slashing amount based on the expected reward rate of the node is the one concerning the full validation period of the node. This is one comment that seems to confirm this concern in the MinipoolManager:

`minipool.item<index>.duration = requested validation duration in seconds (performed as 14 day cycles)`

As well as this comment:

/// @param duration Requested validation period in seconds

In the current state, that means that if the operator is staking for a duration of 1 year or 14 days, in the case of slashing, they won't be slashed the same amount. It is an issue because rather than incentivizing minipool operators to stake for a long period, it is rather penalizing them in the case their node does not maintain a sufficient uptime. Also, this slashing amount gets exponentially higher as the validation duration is bigger, because it is going to be called more times (a bigger validation duration can fit more streamed 14 days period), with a more painful slash (calculated from the whole duration rather than the constant 14 days).

Proof of Concept

1. Bob registers a minipool for 1 year
2. Bob does not maintain a sufficient node uptime for the first reward cycle
3. Bob expect to be slashed for this 14 days period, but instead is slashed for the whole year, which is about 26 times too much

Tools Used

Manual Inspection

Recommended Mitigation Steps

There could be a constant registered in the ProtocolDAO that store this 14 days constant, and the duration used to calculate the amount of ggp to slash should be always using this constant. The RewardsEligibilityMinSeconds seems to be related, albeit doesn't seem to fully match the purpose of this validation cycle interval.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L241-L253 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L163-L164 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L261-L263

Vulnerability details

Impact

A minipool creator can overwrite a minipool that has finished his last validation cycle and that is yet to be recreated.

When creating a new minipool, this one can either be a new one (if the nodeID has never been used before) or overwrite an existing one (if the nodeID has been used before). This works by getting the minipoolIndex for the nodeID in the createMinipool() function.

int256 minipoolIndex = getIndexOf(nodeID);
if (minipoolIndex != -1) {
	// Existing, so overwrite the old pool
} else {
	// It's a new one so we can safely add it
}

We can then effectively overwrite an existing pool if the nodeID is the same, and that is because it is valid for a minipool to go from the Withdrawable status to the Prelaunch status.

function requireValidStateTransition(int256 minipoolIndex, MinipoolStatus to) private view {
  /* snip */
		
	} else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
	  isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch);
	}
	
	/* snip */

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

Later, the pool is reset by calling resetMinipoolData() and this is where things get lost.

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

But more importantly (stake wise), the staking amounts are completely flushed in the following lines:

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

From the docs:

- Minipool is now available for node operator funds to be withdrawn. 
- Node operator cannot create a new validation with us for this nodeID until they have withdrawn all funds.

This appears to be false given the following POC.

Proof of concept

From the protocol flowchart (made with drawio):

We can see that the following steps are possible:

1. A minipool is created
2. The staking starts and the node validates transactions for this validation cycle
3. The staking then ends
4. A malicious user registers a minipool with the same nodeID as this one even before that the minipool distributes any rewards, or that the owner can withdraw their funds
5. Old minipool stake is lost in its entierty

See the following foundry test in order to illustrate:

function testAuditOverwrite() public {
  uint256 duration = 4 weeks;
  uint256 depositAmt = 1000 ether;
  uint256 avaxAssignmentRequest = 1000 ether;
  uint256 validationAmt = depositAmt + avaxAssignmentRequest;
  uint128 ggpStakeAmt = 200 ether;

  uint256 validationRewards = 10 ether;

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

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

  vm.startPrank(address(rialto));
  minipoolMgr.claimAndInitiateStaking(mp.nodeID);
  minipoolMgr.recordStakingStart(mp.nodeID, mp.txID, block.timestamp);
  skip(14 days);
  minipoolMgr.recordStakingEnd{value: validationAmt + validationRewards}(mp.nodeID, block.timestamp, validationRewards);
  // the minipool is ready to be "recreated" ...
  vm.stopPrank();

  // avoid mismatch with the old amount
  uint256 newDepositAmt = 1001 ether;
  uint256 newAvaxAssignmentRequest = 1001 ether;
  address otherNodeOp = address(123456789);
  vm.deal(otherNodeOp, newDepositAmt + 10 ether);

  // stake some ggp
  dealGGP(otherNodeOp, ggpStakeAmt);
  vm.startPrank(otherNodeOp);
  ggp.approve(address(staking), MAX_AMT);
  staking.stakeGGP(ggpStakeAmt);
	
	// overwrite the old pool
  MinipoolManager.Minipool memory newMp = createMinipool(newDepositAmt, newAvaxAssignmentRequest, duration);
  vm.stopPrank();
	
	// make sure that it's really overwritten
	newMp = minipoolMgr.getMinipool(newMp.index);

  assertEq(newMp.avaxNodeOpAmt, newDepositAmt);
  assertEq(newMp.avaxLiquidStakerAmt, newAvaxAssignmentRequest);
}

Which passes correctly:

Running 1 test for test/audit/Recreate.t.sol:AuditMinipoolManagerTest
[PASS] testAuditOverwrite() (gas: 1904520)
Test result: ok. 1 passed; 0 failed; finished in 7.87ms

Tools Used

Manual inspection

Recommended Mitigation Steps

When calling recordStakingEnd(), don't tie the stakers rewards to the minipool index, but rather to a an immutable ID that cannot be overwritten. Another solution would be to make the minipool creation more asynchronous, requiring Rialto approval in order to launch.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ProtocolDAO.sol#L48 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L444 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L329-L333

Vulnerability details

Impact

When the staking ends, the fees (denominated as MinipoolNodeCommissionFeePct from the ProtocolDAO) are amputated from the liquid staker rewards. These fees are set as a fixed rate of 15% in the constructor of the ProtocolDAO. This means that ultimately, the node operator and the liquid stakers will not have the same amount of AVAX staked because of these fees.

When a minipool is "recreated" while calling recreateMinipool (most likely when a minipool cycle is finished), the rewards are collected and coumpounded for the next staking round. The issue lies in these lines of the recreateMinipool() function:

This snippet gets the amount of AVAX staked in the minipool currently as well as the rewards yielded by the validation period. They are then added in order to get the compoundedAvaxNodeOpAmt, but both the avaxNodeOpAmt and avaxLiquidStakerAmt are set to this same value, defeating the fees paid by the liquid stakers to the minipool's node operator.

uint256 compoundedAvaxNodeOpAmt = mp.avaxNodeOpAmt + mp.avaxNodeOpRewardAmt;
setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), compoundedAvaxNodeOpAmt);
setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), compoundedAvaxNodeOpAmt);

Minipool memory mp = getMinipool(minipoolIndex);
// Compound the avax plus rewards
// NOTE Assumes a 1:1 nodeOp:liqStaker funds ratio
uint256 compoundedAvaxNodeOpAmt = mp.avaxNodeOpAmt + mp.avaxNodeOpRewardAmt;
setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), compoundedAvaxNodeOpAmt);
setUint(
	keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), compoundedAvaxNodeOpAmt
);

As you can see, the coumpounded rewards are added at the same rate for both the node operators and liquid stakers, thus not taking into account the node commission fees. This is probably related to the comments written at the top of the calculation, implying that both should be equal because in the current contract design, the fund ratio between the node operator and liquid stakers is 1:1.

Proof of concept

Evaluate this following test:

function testAuditRecreateFees() public {
  uint256 duration = 4 weeks;
  uint256 depositAmt = 1000 ether;
  uint256 avaxAssignmentRequest = 1000 ether;
  uint256 validationAmt = depositAmt + avaxAssignmentRequest;
  uint128 ggpStakeAmt = 200 ether;

  uint256 validationRewards = 10 ether;

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

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

  vm.startPrank(address(rialto));
  minipoolMgr.claimAndInitiateStaking(mp.nodeID);
  minipoolMgr.recordStakingStart(mp.nodeID, mp.txID, block.timestamp);
  skip(14 days);
  minipoolMgr.recordStakingEnd{value: validationAmt + validationRewards}(mp.nodeID, block.timestamp, validationRewards);
  minipoolMgr.recreateMinipool(mp.nodeID);
  vm.stopPrank();

  uint256 opRewards = 5.75 ether;
  uint256 liqRewards = validationRewards - opRewards;

  MinipoolManager.Minipool memory newMp = minipoolMgr.getMinipoolByNodeID(mp.nodeID);

  assertEq(newMp.avaxNodeOpAmt, depositAmt + opRewards);
  assertEq(newMp.avaxLiquidStakerAmt, avaxAssignmentRequest + liqRewards);
}

And the output:

Running 1 test for test/audit/Recreate.t.sol:AuditMinipoolManagerTest
[FAIL. Reason: Assertion failed.] testAuditRecreateFees() (gas: 1379862)
Logs:
  Error: a == b not satisfied [uint]
    Expected: 1004250000000000000000
      Actual: 1005750000000000000000

Test result: FAILED. 0 passed; 1 failed; finished in 7.64ms

Failing tests:
Encountered 1 failing test in test/audit/Recreate.t.sol:AuditMinipoolManagerTest
[FAIL. Reason: Assertion failed.] testAuditRecreateFees() (gas: 1379862)

Encountered a total of 1 failing tests, 0 tests succeeded

As we can see, it's failing at the very last assertion:

assertEq(newMp.avaxLiquidStakerAmt, avaxAssignmentRequest + liqRewards);

In the "actual" assertion equality, we can also add that it's equal to the operator stake, which have half the rewards (10 AVAX + operator fees). As a result, this shows a leak of value in the protocol. The Rialto multisig will then probably call the function claimAndInitiateStaking() and this will make available in the staking the avaxLiquidStakerAmt, previously over-inflated.

Tools Used

Manual inspection

Recommended Mitigation Steps

In order to correct this, it should calculate the assigned AVAX taking in account the liquid stakers fees, similar to how it's done in the recordStakingEnd() function.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L287-L303 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L444-L478 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L293-L302 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L457

Vulnerability details

Impact

By frontrunning the Rialto multisig just before that it calls recreateMinipool(), a malicious minipool owner can call the withdrawMinipoolFunds() in order to receive their validation rewards twice.

1. withdrawMinipoolFunds() is going to put the state into Finished mode, and then remove the minipool owner stake as well rewards that have been distributed by the Avalanche network during the validation period. The whole amount (stake + rewards) is then transferred to the minipool owner.

2. The Rialto multisig then calls recreateMinipool(). This function has the effect to compound the minipool rewards in order to set the new compounded amount to both the minipool operator and liquid stakers to stream a continuous flow of rewards to both the operator and the liquid stakers (with a 14 days interval). The same fee, called avaxNodeOpRewardAmt is compounded for the minipool operator, while it has already been distributed.

At the end of the execution, the node operator put their funds out of the protocol because they are not in staking anymore, but rather in their wallet The issue is that the amount representing the validation fees are incremented in the Staking contract after that the multisig would have called recreateMinipool()

As a result, the fees are distributed twice, while they should only be distributed once.

Proof of Concept

This following foundry test case can be plugged into the MinipoolManager.t.sol.

function testRewardsTwice() public {
	uint256 duration = 4 weeks;
	uint256 depositAmt = 1000 ether;
	uint256 avaxAssignmentRequest = 1000 ether;
	uint256 validationAmt = depositAmt + avaxAssignmentRequest;
	// Enough to start but not to re-stake, we will add more later
	uint128 ggpStakeAmt = 200 ether;

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

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

	vm.startPrank(address(rialto));
	minipoolMgr.claimAndInitiateStaking(mp.nodeID);
	minipoolMgr.recordStakingStart(mp.nodeID, mp.txID, block.timestamp);
	skip(14 days);
	minipoolMgr.recordStakingEnd{value: validationAmt + 10 ether}(mp.nodeID, block.timestamp, 10 ether);
	vm.stopPrank();

	uint256 opBeforeWithdraw = nodeOp.balance;

	// still has the 1000 AVAX staked
	assertEq(staking.getAVAXStake(mp.owner), depositAmt);

	vm.startPrank(nodeOp);
	minipoolMgr.withdrawMinipoolFunds(mp.nodeID);
	vm.stopPrank();

	// now has withdrawn everything from staking
	assertEq(staking.getAVAXStake(mp.owner), 0);

	vm.startPrank(address(rialto));
	minipoolMgr.recreateMinipool(mp.nodeID);
	vm.stopPrank();

	// 50% + operator fees
	uint256 opRewards = 5.75 ether;
	// correctly received the rewards ...
	assertEq(nodeOp.balance, opBeforeWithdraw + depositAmt + opRewards);
	// ... but got the rewards twice
	assertEq(staking.getAVAXStake(mp.owner), opRewards);
}

Tools Used

Manual inspection

Recommended Mitigation Steps

In order to fix this, make sure to decrease the minipool rewards once they have been distributed.

function withdrawMinipoolFunds(address nodeID) external nonReentrant {
    int256 minipoolIndex = requireValidMinipool(nodeID);
    address owner = onlyOwner(minipoolIndex);
    requireValidStateTransition(minipoolIndex, MinipoolStatus.Finished);
    setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Finished));

    uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")));
    uint256 avaxNodeOpRewardAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpRewardAmt")));
    uint256 totalAvaxAmt = avaxNodeOpAmt + avaxNodeOpRewardAmt;
    
    // decrease the minipool operator rewards as they have been distributed. Move the state before external calls to avoid reentrancy.
    subUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpRewardAmt")));

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

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

Lines of code

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

Vulnerability details

Impact

The cancellation of a minipool can happen very early. While it first appears to be enforced a 5 days waiting period in order to cancel the minipool, it can in reality be bypassed if a minipool owner waits enough time, and then registers a new minipool from the same public address. This will be true for any amount of minipool added, making the "early cancellation" protection useless.

Proof of Concept

The reason of that is the following piece of code:

if (staking.getRewardsStartTime(msg.sender) == 0) {
  staking.setRewardsStartTime(msg.sender, block.timestamp);
}

As you can see, the getRewardsStartTime() will always return the same timestamp because it is only set if it was equal to 0 before. Meaning that any new minipool registered by the same owner will have a staking start time equal to the very first one, effectively making the owner able to bypass the early cancellation protection.

See the following scenario:

1. On day1, Bob registers his first minipool using createMinipool()
2. Bob won't be able to cancel his minipool before waiting at least 5 days
3. Some stakers replies to his assignement request to fund his node, then Rialto calls claimAndInitiateStaking()
4. The node validation goes on, staking starts on the Gogopool protocol
5. On day5, Bob registers a new minipool using again createMinipool()
6. Bob won't be able to cancel his minipool right now, but only has to wait 1 day in order to cancel it while it should be 5 days

Please note that the waiting period is absolutely arbitrary. If bob would have been waiting more than 5 days, then the minipool cancellation would have been instant.

Tools Used

Manual Inspection

Recommended Mitigation Steps

In the createMinipool(), it should perhaps do:

staking.setRewardsStartTime(msg.sender, block.timestamp);

rather than:

if (staking.getRewardsStartTime(msg.sender) == 0) {
  staking.setRewardsStartTime(msg.sender, block.timestamp);
}

One issue arising from this change is that if an user creates a minipool on day1, then create another on day3, this user won't be able to cancel the first pool before day8 because it's going to be set again on day3. It may be a feature, or this specific variable could be tied to the minipool in the storage if not desired.

Lines of code

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

Vulnerability details

Impact

The TokenggAVAX is a yield bearing vault based on the EIP-4626. It was introduced to Gogopool in order to account for the validation rewards yielded by the minipools, in order to distribute their shares to the liquid stakers. When rewards are distributed, the actual shares of the stakers (in ggAVAX) won't change, but these shares will be worth more underlying token (AVAX) over time as the rewards coming from the minipools are streamed to the liquid staking pool. There is an issue in the design of the contract for the rewards distribution causing the stakers to never being able to withdraw their rewards, but will only be able to withdraw the amount that they have deposited.

Proof of Concept

When stakers are depositing some AVAX to the TokenggAVAX pool, a variable called totalReleasedAssets is updated. This variable is used to avoid taking distributing stakers AVAX to stakers as rewards in the syncRewards() function. The issue is that as this variable is tied to the deposited amount, it will devaluate over time compared to the shares because share value in AVAX is only going to grow bigger and bigger. Shares is an invariant for stakers in this case. Similarly to when a staker deposits some AVAX, when this staker is going to withdraw them, the totalReleasedAssets will be this time decremented. If rewards have been distributed by the minipools, the user would expect to withdraw more AVAX than before as an incentive of staking in the protocol, but the function is going to revert as this totalReleasedAssets is going to cause an underflow.

Additionally, this breaks the EIP-4626 compliance because according to the EIP:

maxWithdraw

Maximum amount of the underlying asset that can be withdrawn from the owner balance in the Vault, through a withdraw call.

MUST return the maximum amount of assets that could be transferred from owner through withdraw and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).

MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0.

MUST NOT revert.

After a rewards distribution, the user could send a static call to this maxWithdraw() function in order to get back their stake + their share of the rewards. But calling withdrawAVAX() with the full amount is going to revert.

See the following foundry test showing the issue:

	function testNoRewards() public {
		uint256 depositAmount = 2000 ether;
		uint256 totalStakedAmount = 2000 ether;

		uint256 rewardsAmount = 10 ether;
		uint256 liquidStakerRewards = 5 ether - ((5 ether * 15) / 100);

		uint256 rialtoInitBal = address(rialto).balance;

		// 1. Bob mints 1000 shares
		vm.deal(bob, depositAmount);
		vm.prank(bob);
		ggAVAX.depositAVAX{value: depositAmount}();

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

		// 3. 1000 rewards are deposited
		// None of these rewards should be distributed yet
		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();

		// 4. Skip ahead one rewards cycle
		// Still no rewards should be distributed
		skip(ggAVAX.rewardsCycleLength());

		// 5. Sync rewards and see an update to half the rewards
		ggAVAX.syncRewards();

		// recreate and reward a minipool
		vm.startPrank(address(rialto));
		minipoolMgr.recreateMinipool(nodeID);
		minipoolMgr.claimAndInitiateStaking(nodeID);
		minipoolMgr.recordStakingStart(nodeID, txID, block.timestamp);
		endTime = block.timestamp + mp.duration;
		skip(mp.duration);
		MinipoolManager.Minipool memory compMp = minipoolMgr.getMinipool(mp.index);
		minipoolMgr.recordStakingEnd{value: compMp.avaxNodeOpAmt + compMp.avaxLiquidStakerAmt + rewardsAmount}(nodeID, endTime, rewardsAmount);
		vm.stopPrank();

		// wait for the cycle to end
		skip(ggAVAX.rewardsCycleLength());

		// make sure that bob yielded some rewards
		uint256 maxWith = ggAVAX.maxWithdraw(bob);
		assertEq(maxWith, depositAmount + liquidStakerRewards);

		vm.prank(bob);
		// this reverts because of an underflow
		ggAVAX.withdrawAVAX(depositAmount + liquidStakerRewards);
	}

And the output of the test:

Running 1 test for test/audit/Tokengg.t.sol:TokenggAVAXTest
[FAIL. Reason: Arithmetic over/underflow] testNoRewards() (gas: 1226266)
Test result: FAILED. 0 passed; 1 failed; finished in 8.45ms

Failing tests:
Encountered 1 failing test in test/audit/Tokengg.t.sol:TokenggAVAXTest
[FAIL. Reason: Arithmetic over/underflow] testNoRewards() (gas: 1226266)

Encountered a total of 1 failing tests, 0 tests succeeded

Which is indeed failing because of the underflow caused by totalReleasedAssets.

Tools Used

Manual inspection

Recommended Mitigation Steps

Replace the variable totalReleasedAssets by totalReleasedShares and convert it to assets whenever needed.

This would be an example of refactoring:

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

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

		uint192 lastRewardsAmt_ = lastRewardsAmt;
		// Convert shares to assets
		uint256 totalReleasedAssets_ = previewMint(totalReleasedShares);
		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;
		totalReleasedShares = totalReleasedShares + previewWithdraw(lastRewardsAmt_);
		emit NewRewardsCycle(nextRewardsCycleEnd, nextRewardsAmt);
	}

	function beforeWithdraw(
		uint256 /* amount */,
		uint256 shares
	) internal override {
		// Account for shares here
		totalReleasedShares -= amount;
	}
	
	function afterDeposit(
		uint256 /* amount */,
		uint256 shares
	) internal override {
		// Account for shares here
		totalReleasedShares += amount;
	}