Unbounded loop in RewardsPool#getInflationAmt

[code423n4]

Lines of code

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

Vulnerability details

Impact

the function getInflationAmt will be more and more expensive to call.

Proof of Concept

note the function

/// @notice Function to compute how many tokens should be minted
/// @return currentTotalSupply current total supply
/// @return newTotalSupply supply after mint
function getInflationAmt() public view returns (uint256 currentTotalSupply, uint256 newTotalSupply) {
	ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
	uint256 inflationRate = dao.getInflationIntervalRate();
	uint256 inflationIntervalsElapsed = getInflationIntervalsElapsed();
	currentTotalSupply = dao.getTotalGGPCirculatingSupply();
	newTotalSupply = currentTotalSupply;

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

note the for loop

// Compute inflation for total inflation intervals elapsed
for (uint256 i = 0; i < inflationIntervalsElapsed; i++) {
	newTotalSupply = newTotalSupply.mulWadDown(inflationRate);
}

This for loop has no upper bound and can grow and grow infinitely and consume more and more gas.

We need to look into getInflationIntervalsElapsed

/// @notice Inflation intervals that have elapsed since inflation was last calculated
/// @return Number of intervals since last inflation cycle (0, 1, 2, etc)
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();
}

startTime is set when the function is initiliazed.

setUint(keccak256("RewardsPool.RewardsCycleStartTime"), block.timestamp);
setUint(keccak256("RewardsPool.InflationIntervalStartTime"), block.timestamp);

block.timestamp only grows as time passes

the dao.getInflationIntervalSeconds() parameter is 1 day.

This is set in the ProtocolDAO.sol setting contract.

// GGP Inflation
setUint(keccak256("ProtocolDAO.InflationIntervalSeconds"), 1 days);
setUint(keccak256("ProtocolDAO.InflationIntervalRate"), 1000133680617113500); // 5% annual calculated on a daily interval - Calculate in js example: let dailyInflation = web3.utils.toBN((1 + 0.05) ** (1 / (365)) * 1e18);

According to

(block.timestamp - startTime) / dao.getInflationIntervalSeconds();

the getInflationIntervalsElapsed value incresae by 1 per day. After 1 years, the loop needs to run 365 times, after 10 years, the look needs to run 3650 times, which is costly, which make user not incentived to call the function

/// @notice Public function that will run a GGP rewards cycle if possible
function startRewardsCycle() external {

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project refractor the implementation the cache the looped amount to not make the for loop increasing infinitely.

[GalloDaSballo]

Primary because longer description, but only marginally better

[c4-judge]

GalloDaSballo marked the issue as primary issue

[0xju1ie]

primary for the unbounded loop

[GalloDaSballo]

I have already judged a similar report for Gas, and because the variables are in memory, believe that the finding's impact is heavily diminished

Let's assume each loop costs us:
100 for Looping
200 for the math operation

(both exaggerated)

We'd need over 26k iterations before getting to 8MLN gas (AVAX cap)

Additionally, the function is called by startRewardsCycle which is public and unprotected to avoid it never being called.

For this reason, I believe the finding to be valid, but to be of Low Severity

L

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-c