GoGoPool contest - slowmoses's results

Lines of code

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

Vulnerability details

Error in Inflation Calculation

Consider the following three tests. You can run them yourself by copy-pasting them into test/unit/RewardsPool.t.sol.

function testTwoRewardCyclesExactly() public {
	vm.expectRevert(RewardsPool.UnableToStartRewardsCycle.selector);
	rewardsPool.startRewardsCycle();

	skip(dao.getRewardsCycleSeconds());
	rewardsPool.startRewardsCycle();

	skip(dao.getRewardsCycleSeconds());
	rewardsPool.startRewardsCycle();
	
	console.log("rewardsPool.getRewardsCycleStartTime():");
	console.log(rewardsPool.getRewardsCycleStartTime());
	console.log("");
	console.log("vault.balanceOfToken(ClaimNodeOp, ggp):");
	console.log(vault.balanceOfToken("ClaimNodeOp", ggp));
}

Output:

[PASS] testTwoRewardCyclesExactly() (gas: 506219)
Logs:
rewardsPool.getRewardsCycleStartTime():
4838401

vault.balanceOfToken(ClaimNodeOp, ggp):
94672638630713569578115

After two reward cycles the new start time is 4838401 and the token balance is 94...

Now if we add one more day, before we end the second reward cycle:

function testTwoRewardCyclesPlusOneDaySecond() public {
	vm.expectRevert(RewardsPool.UnableToStartRewardsCycle.selector);
	rewardsPool.startRewardsCycle();

	skip(dao.getRewardsCycleSeconds());
	rewardsPool.startRewardsCycle();

	skip(dao.getRewardsCycleSeconds()+86400);
	rewardsPool.startRewardsCycle();
	
	console.log("rewardsPool.getRewardsCycleStartTime():");
	console.log(rewardsPool.getRewardsCycleStartTime());
	console.log("");
	console.log("vault.balanceOfToken(ClaimNodeOp, ggp):");
	console.log(vault.balanceOfToken("ClaimNodeOp", ggp));
}

Output:

[PASS] testTwoRewardCyclesPlusOneDaySecond() (gas: 506606)
Logs:
rewardsPool.getRewardsCycleStartTime():
4924801

vault.balanceOfToken(ClaimNodeOp, ggp):
96369670303099586747731

Now after two reward cycles and one day we end up with a balance of 96...

This is the intended behaviour but this will only be observed when we start the reward calculations exactly at the first second of an inflation cycle.

If the reward calculation is started at a random time, let's say on average after half an inflation cycle (= 12 hours) we get the following:

function testTwoRewardCyclesPlusTwoHalfDays() public {
	vm.expectRevert(RewardsPool.UnableToStartRewardsCycle.selector);
	rewardsPool.startRewardsCycle();

	skip(dao.getRewardsCycleSeconds()+43200);
	rewardsPool.startRewardsCycle();

	skip(dao.getRewardsCycleSeconds()+43200);
	rewardsPool.startRewardsCycle();
	
	console.log("rewardsPool.getRewardsCycleStartTime():");
	console.log(rewardsPool.getRewardsCycleStartTime());
	console.log("");
	console.log("vault.balanceOfToken(ClaimNodeOp, ggp):");
	console.log(vault.balanceOfToken("ClaimNodeOp", ggp));
}

Output:

[PASS] testTwoRewardCyclesPlusTwoHalfDays() (gas: 506413)
Logs:
rewardsPool.getRewardsCycleStartTime():
4924801

vault.balanceOfToken(ClaimNodeOp, ggp):
94672638630713569578115

Now two cycles and one day have passed, as in the second test, but we only get the interest/inflation for two cycles exaclty, as we do in the first test.

The difference works out to 96.../94... = 1.79 percent of lost rewards.

The problem is in the inflate function:

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

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

"inflationIntervalElapsedSeconds" are calculated with block.timestamp in the second line of the function.

uint256 inflationIntervalElapsedSeconds = (block.timestamp - getInflationIntervalStartTime());

But the "newTotalSupply" is calculated in "getInflationAmt()" only for full one-day inflation-cycles.

Therefore we would need to modify the second to last line in the inflate function:

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

The following code should work to give us the same result for tests two and three and a correct result for all fractions of inflation cycles in general:

addUint(keccak256("RewardsPool.InflationIntervalStartTime"), (inflationIntervalElapsedSeconds / dao.getInflationIntervalSeconds())*dao.getInflationIntervalSeconds());