GoGoPool contest - __141345__'s results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L328-L332 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L89-L114 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimProtocolDAO.sol#L20-L35

Vulnerability details

Impact

The whenNotPaused modifier is used to pause minipool creation and staking/withdrawing GGP. However, there are several cases this modifier could be bypassed, which breaks the intended admin control function and special mode.

Proof of Concept

stake()

In paused mode, no more stakeGGP() is allowed,

File: contract/Staking.sol
319: 	function stakeGGP(uint256 amount) external whenNotPaused {
320: 		// Transfer GGP tokens from staker to this contract
321: 		ggp.safeTransferFrom(msg.sender, address(this), amount);
322: 		_stakeGGP(msg.sender, amount);
323: 	}

However, restakeGGP() is still available, which potentially violate the purpose of pause mode.

File: contract/Staking.sol
328: 	function restakeGGP(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) {
329: 		// Transfer GGP tokens from the ClaimNodeOp contract to this contract
330: 		ggp.safeTransferFrom(msg.sender, address(this), amount);
331: 		_stakeGGP(stakerAddr, amount);
332: 	}

withdraw()

In paused mode, no more withdrawGGP() is allowed,

File: contract/Staking.sol
358: 	function withdrawGGP(uint256 amount) external whenNotPaused {

373: 		vault.withdrawToken(msg.sender, ggp, amount);

However, claimAndRestake() is still available, which can withdraw from the vault.

File: contract/ClaimNodeOp.sol
089: 	function claimAndRestake(uint256 claimAmt) external {

103: 		if (restakeAmt > 0) {
104: 			vault.withdrawToken(address(this), ggp, restakeAmt);
105: 			ggp.approve(address(staking), restakeAmt);
106: 			staking.restakeGGP(msg.sender, restakeAmt);
107: 		}
108: 
109: 		if (claimAmt > 0) {
110: 			vault.withdrawToken(msg.sender, ggp, claimAmt);
111: 		}

THe function spend() can also ignore the pause mode to withdraw from the vault. But this is a guardian function. It could be intended behavior.

File: contract/ClaimProtocolDAO.sol
20: 	function spend(
21: 		string memory invoiceID,
22: 		address recipientAddress,
23: 		uint256 amount
24: 	) external onlyGuardian {

32: 		vault.withdrawToken(recipientAddress, ggpToken, amount);

Tools Used

Manual analysis.

Recommended Mitigation Steps

• add the whenNotPaused modifier to restakeGGP() and claimAndRestake()
• maybe also for guardian function spend().

Lines of code

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

Vulnerability details

Impact

The interval number rounding error due to round down could accumulate, in the long term, the arithmetic error can make the reward inflation amount deviate from expected growth rate. Some reward could be "lost", but deserve to be generated otherwise.

Proof of Concept

The inflationIntervalElapsedSeconds is calculated based on the timestamp difference between the spot timestamp and InflationIntervalStartTime. To get the number of Inflation Intervals, inflationIntervalElapsedSeconds is divided by InflationIntervalSeconds (which is set to be 1 days, or 86400 seconds). And round down to get the interval number.

File: contract/ProtocolDAO.sol
40: 		setUint(keccak256("ProtocolDAO.InflationIntervalSeconds"), 1 days);

File: contract/RewardsPool.sol
54: 	function getInflationIntervalsElapsed() public view returns (uint256) {

56: 		uint256 startTime = getInflationIntervalStartTime();

60: 		return (block.timestamp - startTime) / dao.getInflationIntervalSeconds();

Lastly, the way InflationIntervalStartTime updated is simply add the inflationIntervalElapsedSeconds to last InflationIntervalStartTime on record.

File: contract/RewardsPool.sol
82: 	function inflate() internal {
    
84: 		uint256 inflationIntervalElapsedSeconds = (block.timestamp - getInflationIntervalStartTime());

98: 		addUint(keccak256("RewardsPool.InflationIntervalStartTime"), inflationIntervalElapsedSeconds);

The issue is, the rounding down for the interval number could accumulate, over the long term making the reward calculation inaccurate. The inflation growth rate will not work as expected.

Imagine the following

• on day 0, InflationIntervalStartTime is on timestamp 1,000,000.
• after 1 reward cycle of 28 days (28 * 86,400 = 2,419,200 seconds), startRewardsCycle() is called, and InflationIntervalStartTime is updated to 3,419,480 (1,000,000 + 2,419,200 + 280), the 280 seconds is some delay of the function call, where the accumulated error comes from. During this reward cycle, the reward will be inflated by 28 inflation cycle.
• the next reward cycle will have InflationIntervalStartTime as 3,419,480. Each cycle, the inflation interval number will be 28 (inflated 28 times). Assume in each cycle, the delay is 280 seconds. This delay will not be counted in the inflation calculation due to the rounding down.
• after 309 inflation cycles, the InflationIntervalStartTime will be increased to 27,784,120 (1,000,000 + 309 * 86,680). Noted that here 86,680 is used instead of 86,400, because the 280 seconds delay is also included. However, the calendar days passed is 310 days (309 * 86,680 / 86,400 = 310), over the time, the 280 seconds delay accumulate to 1 whole day.

Also assuming the last startRewardsCycle() call is at the end of 310 days, not just pass the reward cycle timestamp.

According to the function getInflationAmt(), the inflation amount is calculated based on the interval numbers 309, but the actual calendar day passed is 310. The inflation amount will has a difference here for 1 inflation interval (1 day). The difference is 1.000133680617113500 (5% annual).

Effectively, the 0.0134% reward is lost due to the arithmetic error accumulation.

File: contract/RewardsPool.sol
66: 	function getInflationAmt() public view returns (uint256 currentTotalSupply, uint256 newTotalSupply) {

69: 		uint256 inflationIntervalsElapsed = getInflationIntervalsElapsed();

73: 		// Compute inflation for total inflation intervals elapsed
74: 		for (uint256 i = 0; i < inflationIntervalsElapsed; i++) {
75: 			newTotalSupply = newTotalSupply.mulWadDown(inflationRate);
76: 		}
77: 		return (currentTotalSupply, newTotalSupply);

Tools Used

Manual analysis.

Recommended Mitigation Steps

In function inflate(), when update InflationIntervalStartTime, instead of simply add the inflationIntervalElapsedSeconds, also round down the elapsed seconds, just like the interval elapsed calculation.

File: contract/RewardsPool.sol
82: 	function inflate() internal {
    
84: 		uint256 inflationIntervalElapsedSeconds = (block.timestamp - getInflationIntervalStartTime());

-98: 		addUint(keccak256("RewardsPool.InflationIntervalStartTime"), inflationIntervalElapsedSeconds);
+98: 		uint256 adjustElapsedSeconds = inflationIntervalElapsedSeconds * dao.getInflationIntervalSeconds() / dao.getInflationIntervalSeconds();
+99: 		addUint(keccak256("RewardsPool.InflationIntervalStartTime"), adjustElapsedSeconds);

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L278-L281 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L196-L199

Vulnerability details

Impact

Currently, the Moratorium waiting period to cancel minipool is set to be 5 days. However, the wait period checks only the msg.sender, the requirement could be bypassed if the node operator has multiple nodes, and some other nodeID has already passed the waiting period.

Proof of Concept

The getRewardsStartTime(msg.sender) only tracks the msg.sender, not the nodeID address.

File: contracts/contract/ProtocolDAO.sol
52: 		setUint(keccak256("ProtocolDAO.MinipoolCancelMoratoriumSeconds"), 5 days);

File: contracts/contract/MinipoolManager.sol
278: 		// make sure they meet the wait period requirement
279: 		if (block.timestamp - staking.getRewardsStartTime(msg.sender) < dao.getMinipoolCancelMoratoriumSeconds()) {
280: 			revert CancellationTooEarly();
281: 		}

File: contracts/contract/Staking.sol
196: 	function getRewardsStartTime(address stakerAddr) public view returns (uint256) {
197: 		int256 stakerIndex = getIndexOf(stakerAddr);
198: 		return getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".rewardsStartTime")));
199: 	}

The RewardsStartTime is only initialized if it is not set yet. So new created minipool for other nodeID will not be recorded.

File: contracts/contract/MinipoolManager.sol
225: 		if (staking.getRewardsStartTime(msg.sender) == 0) {
226: 			staking.setRewardsStartTime(msg.sender, block.timestamp);
227: 		}

Imagine, node operator addr(Alice) has 2 nodeID, addr(0x01) and addr(0x02). Alice creates minipool for nodeID addr(0x01). after 6 days, Alice creates another minipool for nodeID addr(0x02). According to the wait period, nodeID addr(0x02) should not be cancelled since it is still within the 5 day Moratorium range. But getRewardsStartTime(addr(Alice)) is set to 6 days ago by addr(0x01), which qualifies for cancellation. Then nodeID addr(0x02) can bypass the wait period and be cancelled.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Record the RewardsStartTime per nodeID, not just operator address.

Lines of code

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

Vulnerability details

Impact

In the current rewards accounting, vault shares in deposit() and redeem() can not correctly record the spot yields generated by the staked asset. Yields are released over the next rewards cycle. As a result, malicious users can steal yields from innocent users by picking special timing to deposit() and redeem().

Proof of Concept

The key in this issue is that the rewards release is not aligned with the actual staking period. Hence, inevitably there will be some mismatch between the rewards received and the true effective staking.

Although in the known issue section of the doc, the comment claims that some actions could be done for abuse case, there is some underlying potential unfairness of the mismatched mechanism. And sometimes it might not cause big steal of funds, however, overtime, some users could lose some portion of their rewards, some users might just be lucky to receive some extra rewards they do not deserve. And the difference could be small or medium depending on specific situation, the inaccurate release model unavoidably introduce some degree of unfairness.

In syncRewards(), the current asset balance is break into 2 parts: totalReleasedAssets_ and lastRewardsAmt_/nextRewardsAmt. The lastRewardsAmt_ is the surplus balance of the asset, or the most recent yields.

File: contract/tokens/TokenggAVAX.sol
88: 	function syncRewards() public {

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

102: 		uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;

104: 		lastRewardsAmt = nextRewardsAmt.safeCastTo192();

106: 		rewardsCycleEnd = nextRewardsCycleEnd;
107: 		totalReleasedAssets = totalReleasedAssets_ + lastRewardsAmt_;

And in the next rewards cycle, lastRewardsAmt_ will be linearly added to totalReleasedAssets_, their sum is the return value of totalAssets():

113: 	function totalAssets() public view override returns (uint256) {

120: 		if (block.timestamp >= rewardsCycleEnd_) {
121: 			// no rewards or rewards are fully unlocked
122: 			// entire reward amount is available
123: 			return totalReleasedAssets_ + lastRewardsAmt_;
124: 		}
125: 
126: 		// rewards are not fully unlocked
127: 		// return unlocked rewards and stored total
128: 		uint256 unlockedRewards = (lastRewardsAmt_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);
129: 		return totalReleasedAssets_ + unlockedRewards;
130: 	}

totalAssets() will be referred when deposit() and redeem().

File: contract/tokens/upgradeable/ERC4626Upgradeable.sol
42: 	function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {

44: 		require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");


091: 	function redeem(
092: 		uint256 shares,
093: 		address receiver,
094: 		address owner
095: 	) public virtual returns (uint256 assets) {

103: 		require((assets = previewRedeem(shares)) != 0, "ZERO_ASSETS");
104: 
105: 		beforeWithdraw(assets, shares);
106: 
107: 		_burn(owner, shares);

132: 	function previewDeposit(uint256 assets) public view virtual returns (uint256) {
133: 		return convertToShares(assets);
134: 	}

148: 	function previewRedeem(uint256 shares) public view virtual returns (uint256) {
149: 		return convertToAssets(shares);
150: 	}

120: 	function convertToShares(uint256 assets) public view virtual returns (uint256) {
121: 		uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
122: 
123: 		return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
124: 	}

126: 	function convertToAssets(uint256 shares) public view virtual returns (uint256) {
127: 		uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
128: 
129: 		return supply == 0 ? shares : shares.mulDivDown(totalAssets(), supply);
130: 	}

Based on the above rules, there are 2 potential abuse cases:

1. If withdraw just after the rewardsCycleEnd timestamp, a user can not get the yields from last rewards cycle. Since the totalAssets() only contain totalReleasedAssets_ but not the yields part. It takes 1 rewards cycle to linearly add to the totalReleasedAssets_.

Assume per 10,000 asset staking generate yields of 280 for 28 days, and the reward cycle is 14 day. A malicious user Alice can do the following:

• watch the mempool for withdraw(10,000) from account Bob, front run it with syncRewards(), so that the most recent yields of amount 140 from Bob will stay in the vault.
• Alice will also deposit a 10,000 to take as much shares as possible.
• after 1 rewards cycle of 14 day, redeem() to take the yields of 280.

Effectively steal the yields from Bob. The profit for Alice is not 280, because after 14 day, her own deposit also generates some yield, in this example this portion is 140. At the end, Alice steal yield of amount 140.

2. When the admin transfers new yields into the vault, the new yields will accumulate until syncRewards() is called. It is possible that yields from multiple rewards cycles accumulates, and being released in the next cycle.

Knowing that the yields has been accumulated for 3 rewards cycles, a malicious user can deposit() and call syncRewards() to trigger the release of the rewards. redeem() after 1 cycle.

Here the malicious user gets yields of 3 cycles, lose 1 in the waiting cycle. The net profit is 2 cycle yields, and the gained yields should belong to the other users in the vault.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• for the lastRewardsAmt_ not released, allow the users to redeem as it is linearly released later.
• for the accumulated yields, only allow users to redeem the yields received after 1 rewards cycle after the deposit.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L68-L76 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L55-L67 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L210-L229

Vulnerability details

Impact

The multisig could be disabled, if all the multisigs were disabled, the reward cycle will have Denial of Service, and become inoperable for some period. Although it is an edge case that all multisigs were disabled, some treatment for special cases will make the protocol more robust in operations.

Proof of Concept

Each multisig could be disabled, or even all multisigs can be disabled totally.

File: contract/MultisigManager.sol
68: 	function disableMultisig(address addr) external guardianOrSpecificRegisteredContract("Ocyticus", msg.sender) {
69: 		int256 multisigIndex = getIndexOf(addr);
70: 		if (multisigIndex == -1) {
71: 			revert MultisigNotFound();
72: 		}
73: 
74: 		setBool(keccak256(abi.encodePacked("multisig.item", multisigIndex, ".enabled")), false);
75: 		emit DisabledMultisig(addr, msg.sender);
76: 	}

File: contract/Ocyticus.sol
55: 	function disableAllMultisigs() public onlyDefender {
56: 		MultisigManager mm = MultisigManager(getContractAddress("MultisigManager"));
57: 		uint256 count = mm.getCount();
58: 
59: 		address addr;
60: 		bool enabled;
61: 		for (uint256 i = 0; i < count; i++) {
62: 			(addr, enabled) = mm.getMultisig(i);
63: 			if (enabled) {
64: 				mm.disableMultisig(addr);
65: 			}
66: 		}
67: 	}

If enabledCount is 0, function distributeMultisigAllotment() will revert in line 229. startRewardsCycle() will also revert and cause DoS. Because the multisigClaimContractAllotment is not dependent on the number of enabled multisigs.

File: contract/RewardsPool.sol
203: 	function distributeMultisigAllotment(
204: 		uint256 allotment,
205: 		Vault vault,
206: 		TokenGGP ggp
207: 	) internal {

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

215: 		for (uint256 i = 0; i < count; i++) {
216: 			(address addr, bool enabled) = mm.getMultisig(i);
217: 			if (enabled) {
218: 				enabledMultisigs[enabledCount] = addr;
219: 				enabledCount++;
220: 			}
221: 		}

229: 		uint256 tokensPerMultisig = allotment / enabledCount;

156: 	function startRewardsCycle() external {

171: 		uint256 multisigClaimContractAllotment = getClaimingContractDistribution("ClaimMultisig");

186: 		if (multisigClaimContractAllotment > 0) {
187: 			emit MultisigRewardsTransfered(multisigClaimContractAllotment);
188: 			distributeMultisigAllotment(multisigClaimContractAllotment, vault, ggp);
189: 		}

Tools Used

Manual analysis.

Recommended Mitigation Steps

Add checks for the enabledCount

    if (enabledCount > 0) {
        // ...
    } else {
        // ...
    }

If the enabledCount is 0, the corresponding rewards can be saved to somewhere else, instead of lost or locked in the contract.