TokenggAVAX: actual reward streaming period can be as small as 1 second due to rounding

[code423n4]

Lines of code

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

Vulnerability details

Impact

The TokenggAVAX contract is supposed to stream rewards in AVAX to ggAVAX holders over a period of 14 days.

The 14 days period however is only an upper limit for the reward streaming period.

The way the reward streaming period is calculated can make is as small as 1 second (due to rounding).

This is clearly not intended and you should calculate nextRewardsCycleEnd differently such that the 14 day period is ensured.

Proof of Concept

Rewards are streamed during a period of length rewardsCycleEnd - lastSync in the totalAssets function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L113-L130).

rewardsCycleEnd and lastSync are calculated in the syncRewards function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L88-L109).

I will show with a simple example that rewardsCycleEnd - lastSync can be as small as 1 second meaning that all rewards that are supposed to be streamed over a 14 days period are streamed within 1 second:

syncRewards() is called with:

block.timestamp = 10099,
rewardsCycleLength = 100

nextRewardsCycleEnd is calculated:

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

timestamp + rewardsCycleLength is equal to 10199. Dividing this by rewardsCycleLength results in 101 due to rounding.
101 * 100 is equal to 10100.

So nextRewardsCycleEnd = 10100 and lastSync = 10099.

This means that after 1 second all rewards are streamed.

Tools Used

VSCode

Recommended Mitigation Steps

In the code it is stated that you calculate uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength; because nextRewardsCycleEnd should be divisible by rewardsCycleLength.

I see no reason for this requirement and instead propose to calculate nextRewardsCycleEnd like this:

uint32 nextRewardsCycleEnd = (timestamp + rewardsCycleLength);

[c4-judge]

GalloDaSballo marked the issue as duplicate of #478

[c4-judge]

GalloDaSballo marked the issue as satisfactory