GoGoPool contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

Node operators can manipulate the assigned high water to be higher than the actual.

Proof of Concept

The protocol rewards node operators according to the AVAXAssignedHighWater that is the maximum amount assigned to the specific staker during the reward cycle. In the function MinipoolManager.recordStakingStart(), the AVAXAssignedHighWater is updated as below.

MinipoolManager.sol
349: 	function recordStakingStart(
350: 		address nodeID,
351: 		bytes32 txID,
352: 		uint256 startTime
353: 	) external {
354: 		int256 minipoolIndex = onlyValidMultisig(nodeID);
355: 		requireValidStateTransition(minipoolIndex, MinipoolStatus.Staking);
356: 		if (startTime > block.timestamp) {
357: 			revert InvalidStartTime();
358: 		}
359:
360: 		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Staking));
361: 		setBytes32(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".txID")), txID);
362: 		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".startTime")), startTime);
363:
364: 		// If this is the first of many cycles, set the initialStartTime
365: 		uint256 initialStartTime = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")));
366: 		if (initialStartTime == 0) {
367: 			setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), startTime);
368: 		}
369:
370: 		address owner = getAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")));
371: 		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")));
372: 		Staking staking = Staking(getContractAddress("Staking"));
373: 		if (staking.getAVAXAssignedHighWater(owner) < staking.getAVAXAssigned(owner)) {
374: 			staking.increaseAVAXAssignedHighWater(owner, avaxLiquidStakerAmt);//@audit wrong
375: 		}
376:
377: 		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Staking);
378: 	}

In the line #373, if the current assigned AVAX is greater than the owner's AVAXAssignedHighWater, it is increased by avaxLiquidStakerAmt. But this is supposed to be updated to staking.getAVAXAssigned(owner) rather than being increased by the amount.

Example: The node operator creates a minipool with 1000AVAX via createMinipool(nodeID, 2 weeks, delegationFee, 1000*1e18). On creation, the assigned AVAX for the operator will be 1000AVAX. If the Rialtor calls recordStakingStart(), AVAXAssignedHighWater will be updated to 1000AVAX. After the validation finishes, the operator creates another minipool with 1500AVAX this time. Then on recordStakingStart(), AVAXAssignedHighWater will be updated to 2500AVAX by increasing 1500AVAX because the current assigned AVAX is 1500AVAX which is higher than the current AVAXAssignedHighWater=1000AVAX. This is wrong because the actual highest assigned amount is 1500AVAX. Note that AVAXAssignedHighWater is reset only through the function calculateAndDistributeRewards which can be called after RewardsCycleSeconds=28 days.

Tools Used

Manual Review

Recommended Mitigation Steps

Call staking.resetAVAXAssignedHighWater(owner) instead of calling increaseAVAXAssignedHighWater().

MinipoolManager.sol
373: 		if (staking.getAVAXAssignedHighWater(owner) < staking.getAVAXAssigned(owner)) {
374: 			staking.resetAVAXAssignedHighWater(owner); //@audit update to the current AVAX assigned
375: 		}

Lines of code

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

Vulnerability details

Impact

Slashed GGP tokens are trapped and the victim liquid stakers do not get compensation.

Proof of Concept

If the node did not operate well (uptime less than 80%) the validation node does not get any rewards from Avalanche and the protocol tries to punish the node operator and compensate the victim liquid stakers through a slash process.

MinipoolManager.sol
670: 	function slash(int256 index) private {
671: 		address nodeID = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".nodeID")));
672: 		address owner = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".owner")));
673: 		uint256 duration = getUint(keccak256(abi.encodePacked("minipool.item", index, ".duration")));
674: 		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));
675: 		uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt);
676: 		uint256 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);
677: 		setUint(keccak256(abi.encodePacked("minipool.item", index, ".ggpSlashAmt")), slashGGPAmt);
678:
679: 		emit GGPSlashed(nodeID, slashGGPAmt);
680:
681: 		Staking staking = Staking(getContractAddress("Staking"));
682: 		staking.slashGGP(owner, slashGGPAmt);
683: 	}

Staking.sol
379: 	function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
380: 		Vault vault = Vault(getContractAddress("Vault"));
381: 		decreaseGGPStake(stakerAddr, ggpAmt);
382: 		vault.transferToken("ProtocolDAO", ggp, ggpAmt);//@audit slashed ggp should be used to compensate liquid stakers
383: 	}

But if we look at the function Staking.slashGGP(), the slashed GGP tokens are transferred to ProtocolDao contract and there is no function exposed in ProtocolDao contract (or any other place) to get these GGP tokens out. Furthermore, these GGP tokens are supposed to be used to compensate the liquid stakers, so I believe the protocol intends to exchange GGP into AVAX in some way and transfers to TokenggAVAX contract so that the liquid stakers can claim later.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a functionality to exchange GGP into AVAX and put them into the TokenggAVAX contract.

Lines of code

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

Vulnerability details

Impact

Slashed GGP tokens are trapped in the ProtocolDAO contract and liquid stakers are not compensated.

Proof of Concept

The protocol has a mechanism to slash GGP tokens when the nodes did not operate well (uptime less than 80%). But looking at the function slashGGP(), the slashed GGP tokens are transferred to the ProtocolDAO contract and there is no way to transfer/spend those. I believe it is supposed to be sent to ClaimProtocolDAO contract, not the ProtocolDAO, so that it can be spent later. Furthermore, I don't see any mechanism to compensate the victim liquid stakers. I believe the protocol intends to exchange the slashed GGP tokens into AVAX and put into the TokenggAVAX so that liquid stakers can claim.

Staking.sol
379: 	function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
380: 		Vault vault = Vault(getContractAddress("Vault"));
381: 		decreaseGGPStake(stakerAddr, ggpAmt);
382: 		vault.transferToken("ProtocolDAO", ggp, ggpAmt);//@audit wrong contract?
383: 	}

Tools Used

Manual Review

Recommended Mitigation Steps

• Transfer the slashed GGP tokens to ClaimProtocolDAO contract so that the protocol can spend later.
• Implement a mechanism to compensate the liquid stakers when the node operator's GGP collateral is slashed.

Lines of code

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

Vulnerability details

Impact

First depositor can manipulate the share price and prevent small deposits for other LPs

Proof of Concept

TokenggAVAX implemented ERC4626 and there is a well-known attack vector for ERC4626 (See here) The first depositor puts 1wei of AVAX as the first deposit and gets 1wei of ggAVAX share. Then he sends an enormous amounts of AVAX and the price of share becomes very big. Although the impact is not immediate becuase of the xERC4626 implementation, it will affect slowly through the 14 days.(rewardsCycleLength) So following depositors needs to put high amounts to avoid getting zero shares. This protocol has a restriction to allow only positive shares so the attacker does not get profits from the following depositors. But it's still problematic because the LPs are limited in the deposit amount.

Tools Used

Manual Review

Recommended Mitigation Steps

• Require the first deposit amount to be more than some thresholds.
• Consider minting some ggAVAX to zero address on initialization. (Like Uniswap V2)

Lines of code

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

Vulnerability details

Impact

GGP tokens will be inflated more slowly than planned

Proof of Concept

The protocol is going to inflate GGP tokens 5% per year and reward the node operators, multisig operators and the protocol DAO. For this, the protocol calculates the daily inflation rate and use that for inflation.

ProtocolDAO.sol
39: 		// GGP Inflation
40: 		setUint(keccak256("ProtocolDAO.InflationIntervalSeconds"), 1 days);
41: 		setUint(keccak256("ProtocolDAO.InflationIntervalRate"), 1000133680617113500); // 5% annual calculated on a daily interval

The inflation interval is set to 1 day (default) and the public function startRewardsCycle() is supposd to be called (by anyone) to cause inflation. Looking at the function inflate(), we can see that the InflationIntervalStartTime is updated to the current block.timestamp.

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);//@audit equal to setting as block.timestamp
  setUint(keccak256("RewardsPool.RewardsCycleTotalAmt"), newTokens);
}

But the actual reward is calculated based on the number of intervals, not the exact seconds.

function getInflationIntervalsElapsed() public view returns (uint256) {
  ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
  uint256 startTime = getInflationIntervalStartTime();
  if (startTime == 0) {
    revert ContractHasNotBeenInitialized();
  }
  return (block.timestamp - startTime) / dao.getInflationIntervalSeconds();//@audit-info rounded to days
}

It means the actual inflation was done for the time getInflationIntervalsElapsed() * getInflationIntervalSeconds() which is different from (always equal or less than) block.timestamp - getInflationIntervalStartTime().

So the InflationIntervalStartTime should be increased to the actual time used for calculation (getInflationIntervalsElapsed() * getInflationIntervalSeconds()) or else the inflation will happen more slowly than planned.

Tools Used

Manual Review

Recommended Mitigation Steps

Change the function inflate() as below.

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"), getInflationIntervalsElapsed() * getInflationIntervalSeconds()); //@audit fix here
  setUint(keccak256("RewardsPool.RewardsCycleTotalAmt"), newTokens);current cycle
}

Lines of code

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

Vulnerability details

Impact

Minipools can be created using other operator's AVAX deposit via recreateMinipool

Proof of Concept

This issue is related to state transition of Minipools. According to the implementation, the possible states and transitions are as below.

The Rialto may call recreateMinipool when the minipool is in states of Withdrawable, Finished, Error, Canceled. The problem is that these four states are not the same in the sense of holding the node operator's AVAX. If the state flow has followed Prelaunch->Launched->Staking->Error, all the AVAX are still in the vault. If the state flow has followed Prelaunch->Launched->Staking->Error->Finished (last transition by withdrawMinipoolFunds), all the AVAX are sent back to the node operator. So if the Rialto calls recreateMinipool for the second case, there are no AVAX deposited from the node operator at that point but there can be AVAX from other mini pools in the state of Prelaunch. Because there are AVAX in the vault (and these are not managed per staker base), recreatePool results in a new mini pool in Prelaunch state and it is further possible to go through the normal flow Prelaunch->Launched->Staking->Withdrawable->Finished. And the other minipool that was waiting for launch will not be able to launch because the vault is lack of AVAX.

Below is a test case written to show an example.

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

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

  // Node Op 2
  address nodeOp2 = getActorWithTokens("nodeOp2", MAX_AMT, MAX_AMT);
  vm.startPrank(nodeOp2);
  ggp.approve(address(staking), MAX_AMT);
  staking.stakeGGP(ggpStakeAmt);
  MinipoolManager.Minipool memory mp2 = createMinipool(
    depositAmt,
    avaxAssignmentRequest,
    duration
  );
  vm.stopPrank();

  int256 minipoolIndex = minipoolMgr.getIndexOf(mp1.nodeID);

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

  // Node Op 1: Prelaunch->Launched
  vm.prank(address(rialto));
  minipoolMgr.claimAndInitiateStaking(mp1.nodeID);

  bytes32 txID = keccak256("txid");
  vm.prank(address(rialto));
  // Node Op 1: Launched->Staking
  minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);

  vm.prank(address(rialto));
  // Node Op 1: Staking->Error
  bytes32 errorCode = "INVALID_NODEID";
  minipoolMgr.recordStakingError{ value: validationAmt }(mp1.nodeID, errorCode);

  // There are 2*depositAmt AVAX in the pool manager
  assertEq(vault.balanceOf("MinipoolManager"), depositAmt * 2);

  // Node Op 1: Staking->Finished withdrawing funds
  vm.prank(nodeOp);
  minipoolMgr.withdrawMinipoolFunds(mp1.nodeID);
  assertEq(staking.getAVAXStake(nodeOp), 0);
  mp1 = minipoolMgr.getMinipool(minipoolIndex);
  assertEq(mp1.status, uint256(MinipoolStatus.Finished));

  // Rialto recreate a minipool for the mp1
  vm.prank(address(rialto));
  // Node Op 1: Finished -> Prelaunch
  minipoolMgr.recreateMinipool(mp1.nodeID);

  assertEq(vault.balanceOf("MinipoolManager"), depositAmt);

  // Node Op 1: Prelaunch -> Launched
  vm.prank(address(rialto));
  minipoolMgr.claimAndInitiateStaking(mp1.nodeID);

  // Node Op 1: Launched -> Staking
  vm.prank(address(rialto));
  minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);

  assertEq(staking.getAVAXStake(nodeOp), 0);
  assertEq(staking.getAVAXAssigned(nodeOp), depositAmt);
  assertEq(staking.getAVAXAssignedHighWater(nodeOp), depositAmt);

  // now try to launch the second operator's pool, it will fail with InsufficientContractBalance
  vm.prank(address(rialto));
  vm.expectRevert(Vault.InsufficientContractBalance.selector);
  minipoolMgr.claimAndInitiateStaking(mp2.nodeID);
}

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Make sure to keep the node operator's deposit status the same for all states that can lead to the same state. For example, for all states that can transition to Prelaunch, make sure to send the AVAX back to the user and get them back on the call recreateMiniPool().

Lines of code

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

Vulnerability details

Impact

Recreated mini pools can be canceled before MinipoolCancelMoratoriumSeconds

Proof of Concept

The protocol disallows cancelation of the mini pools within the MinipoolCancelMoratoriumSeconds=5 days. But the actual implementation uses staking.getRewardsStartTime to check this restriction and getRewardsStartTime does not always reflect the pool creation time. For example, recreated mini pools still have the previously set getRewardsStartTime and as a result, they can be canceled 5 days since the re-creation.

function cancelMinipool(address nodeID) external nonReentrant {
  Staking staking = Staking(getContractAddress("Staking"));
  ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
  int256 index = requireValidMinipool(nodeID);
  onlyOwner(index);
  // make sure they meet the wait period requirement
  if (
    block.timestamp - staking.getRewardsStartTime(msg.sender) <
    dao.getMinipoolCancelMoratoriumSeconds()
  ) {
    revert CancellationTooEarly();
  }
  _cancelMinipoolAndReturnFunds(nodeID, index);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Add a new state variable to store the pool creation time and use that to check the cancelation moratorium.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L46 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L56

Vulnerability details

Impact

Errored mini pools still can get rewards

Proof of Concept

The Rialto calls calculateAndDistributeRewards() to distribute the rewards to the node operators. It is likely that the Rialto checks if a node op is eligible for rewards by calling isEligible().

According to the isEligible(), the staker is eligible for rewards if rewardsStartTime!=0 and block.timestamp - rewardsStartTime>=RewardsEligibilityMinSeconds (14 days).

As I mentioned in the other issue, rewardsStartTime is set to the current block timestamp on creation of the pools (if not set yet).

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

And looking at the function calculateAndDistributeRewards(), the rewards are decided by the EffectiveGGPStaked and it is based on the AVAXAssignedHighWater of the staker in that cycle.

function calculateAndDistributeRewards(
  address stakerAddr,
  uint256 totalEligibleGGPStaked
) external onlyMultisig {
  Staking staking = Staking(getContractAddress("Staking"));
  staking.requireValidStaker(stakerAddr);

  RewardsPool rewardsPool = RewardsPool(getContractAddress("RewardsPool"));
  if (rewardsPool.getRewardsCycleCount() == 0) {
    revert RewardsCycleNotStarted();
  }

  if (
    staking.getLastRewardsCycleCompleted(stakerAddr) ==
    rewardsPool.getRewardsCycleCount()
  ) {
    revert RewardsAlreadyDistributedToStaker(stakerAddr);
  }
  staking.setLastRewardsCycleCompleted(
    stakerAddr,
    rewardsPool.getRewardsCycleCount()
  );
  uint256 ggpEffectiveStaked = staking.getEffectiveGGPStaked(stakerAddr);
  uint256 percentage = ggpEffectiveStaked.divWadDown(totalEligibleGGPStaked);
  uint256 rewardsCycleTotal = getRewardsCycleTotal();
  uint256 rewardsAmt = percentage.mulWadDown(rewardsCycleTotal);
  if (rewardsAmt > rewardsCycleTotal) {
    revert InvalidAmount();
  }

  staking.resetAVAXAssignedHighWater(stakerAddr);
  staking.increaseGGPRewards(stakerAddr, rewardsAmt);

  // check if their rewards time should be reset
  uint256 minipoolCount = staking.getMinipoolCount(stakerAddr);
  if (minipoolCount == 0) {
    staking.setRewardsStartTime(stakerAddr, 0);
  }
}

The problem is that both rewardsStartTime and AVAXAssignedHighWater are not reset when the pool is transitioned to Error state. So the errored mini pools still can get the rewards and this means the other honest node operators will get less rewards. Below is a test to show that the errored pool still gets the reward.

function testAudit() public {
  skip(dao.getRewardsCycleSeconds());
  rewardsPool.startRewardsCycle();
  // Node Op 1
  address nodeOp1 = getActorWithTokens("nodeOp1", MAX_AMT, MAX_AMT);
  vm.startPrank(nodeOp1);
  ggAVAX.depositAVAX{ value: 2000 ether }();
  ggp.approve(address(staking), MAX_AMT);
  staking.stakeGGP(100 ether);
  MinipoolManager.Minipool memory mp1 = createMinipool(
    1000 ether,
    1000 ether,
    2 weeks
  );
  rialto.processMinipoolStart(mp1.nodeID);
  vm.stopPrank();

  // Node Op 2
  address nodeOp2 = getActorWithTokens("nodeOp2", MAX_AMT, MAX_AMT);
  vm.startPrank(nodeOp2);
  ggAVAX.depositAVAX{ value: 2000 ether }();
  ggp.approve(address(staking), MAX_AMT);
  staking.stakeGGP(100 ether);
  MinipoolManager.Minipool memory mp2 = createMinipool(
    1000 ether,
    1000 ether,
    2 weeks
  );
  rialto.processMinipoolStart(mp2.nodeID);
  vm.stopPrank();
  emit log_named_uint(
    "effectiveGGPStaked before error",
    staking.getEffectiveGGPStaked(nodeOp1)
  );

  // Error occured in the first node
  vm.startPrank(address(rialto));
  bytes32 errorCode = "INVALID_NODE";
  minipoolMgr.recordStakingError{ value: 2000 ether }(mp1.nodeID, errorCode);
  vm.stopPrank();

  // getEffectiveGGPStaked(nodeOp1) still returns valid value
  emit log_named_uint(
    "effectiveGGPStaked after error",
    staking.getEffectiveGGPStaked(nodeOp1)
  );

  // Claim
  vm.startPrank(address(rialto));
  nopClaim.calculateAndDistributeRewards(nodeOp1, 200 ether);
  assertEq(
    staking.getGGPRewards(nodeOp1),
    (nopClaim.getRewardsCycleTotal()) / 2
  );
  vm.stopPrank();
}

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Consider updating the AVAXAssignedHighWater only when the validation succeeds with rewards.

Lines of code

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

Vulnerability details

Impact

Wrong amount of GGP tokens are slashed from the node operators

Proof of Concept

When the node didn't operate well, the protocol punishes the node operator by slashing the GGP collateral. The slash amount is calculated based on the expected reward amount that is calculated as below.

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;//@audit rate is not linear
}

Looking at the function getExpectedAVAXRewardsAmt(), the expected rate is being used linearly. But that rate is specified as an annual rate in the ProtocolDAO and we should calculate the daily reward rate and use exponential terms instead of linear calculation.

In the default scenario, ProtocolDAO.ExpectedAVAXRewardsRate is set to 10% annual rate for each AVAX staked.

With the current implementation, the reward rate for a day is 10% / 365=0.02739726027397260273972602739726% / day. But the actual daily reward rate is 1.1^(1/365)=1.0002611578760678121616866817054.

For example, for (1000 AVAX, 100 days) :

The current implementation: 1000 AVAX * 10% * 100 days / 365 days = 27.39726027397260273972602739726 AVAX The correct amount: 1000 AVAX * (1.0002611578760678121616866817054^100 - 1) = 26.456293126865688807002170685795 AVAX

I found test cases written in MinipoolManager.t.sol and those are wrong as well. 10% increase a year is not equal to 5% increase for 6 months.

    // `MinipoolManager.t.sol
	function testExpectedRewards() public {
		uint256 amt = minipoolMgr.getExpectedAVAXRewardsAmt(365 days, 1_000 ether);
		assertEq(amt, 100 ether);
		amt = minipoolMgr.getExpectedAVAXRewardsAmt((365 days / 2), 1_000 ether);
		assertEq(amt, 50 ether);
        ...
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Calculate the daily reward rate or specify the daily reward rate in the ProtocolDAO contract level and use that.