Rewards for initial period may be lost in BathBuddy contract

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/periphery/BathBuddy.sol#L191-L228

Vulnerability details

Impact

Rewards in the BathBuddy contract are initiated when the owner calls the notifyRewardAmount. This function calculates the reward rate per second (lines 196-198 and 207-208) and also records the start of the reward period (line 223):

https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/periphery/BathBuddy.sol#L191-L228

191:     function notifyRewardAmount(
192:         uint256 reward,
193:         IERC20 rewardsToken
194:     ) external onlyOwner updateReward(address(0), address(rewardsToken)) {
195:         if (block.timestamp >= periodFinish[address(rewardsToken)]) {
196:             rewardRates[address(rewardsToken)] = reward.div(
197:                 rewardsDuration[address(rewardsToken)]
198:             );
199:         } else {
200:             uint256 remaining = periodFinish[address(rewardsToken)].sub(
201:                 block.timestamp
202:             );
203:             uint256 leftover = remaining.mul(
204:                 rewardRates[address(rewardsToken)]
205:             );
206:             rewardRates[address(rewardsToken)] = reward.add(leftover).div(
207:                 rewardsDuration[address(rewardsToken)]
208:             );
209:         }
210: 
211:         // Ensure the provided reward amount is not more than the balance in the contract.
212:         // This keeps the reward rate in the right range, preventing overflows due to
213:         // very high values of rewardRate in the earned and rewardsPerToken functions;
214:         // Reward + leftover must be less than 2^256 / 10^18 to avoid overflow.
215:         // Note********** ERC20s must be here*************
216:         uint256 balance = rewardsToken.balanceOf(address(this));
217:         require(
218:             rewardRates[address(rewardsToken)] <=
219:                 balance.div(rewardsDuration[address(rewardsToken)]),
220:             "Provided reward too high"
221:         );
222: 
223:         lastUpdateTime[address(rewardsToken)] = block.timestamp;
224:         periodFinish[address(rewardsToken)] = block.timestamp.add(
225:             rewardsDuration[address(rewardsToken)]
226:         );
227:         emit RewardAdded(reward);
228:     }

The intention here is to calculate how many tokens should be rewarded by unit of time (second) and record the span of time for the reward cycle. However, this has an edge case where rewards are not counted for the initial period of time until there is at least one participant (in this case, a holder of BathTokens). During this initial period of time, the reward rate will still apply but as there isn't any participant, then no one will be able to claim these rewards. See PoC for a detailed example of the issue.

Proof of Concept

In the following test, we initiate the reward process by calling notifyRewardAmount. At the middle of the duration process, we mint BathTokens to Alice to represent her participant. At the end of the duration process, after Alice claims her rewards, half of them will still be held in the BathBuddy contract.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_BathBuddy_RewardsLostForInitialPeriod() public {
    // Setup rewards
    uint256 startTime = block.timestamp;
    uint256 duration = 10_000 seconds;
    vm.prank(bathBuddyOwner);
    bathBuddy.setRewardsDuration(duration, address(rewardToken));

    uint256 rewardAmount = 100 ether;
    rewardToken.mint(address(bathBuddy), rewardAmount);
    vm.prank(bathBuddyOwner);
    bathBuddy.notifyRewardAmount(rewardAmount, rewardToken);

    // Simulate half of the duration time passes
    vm.warp(startTime + duration / 2);

    // Trigger updateRewards to update state
    vm.prank(bathBuddyHouse);
    bathBuddy.getReward(rewardToken, address(0));

    // Mint bathTokens to Alice
    uint256 bathTokenAmount = 1 ether;
    bathToken.mint(alice, bathTokenAmount);

    // Simulate complete duration time passes
    vm.warp(startTime + duration);

    // Trigger getRewards for Alice
    vm.prank(bathBuddyHouse);
    bathBuddy.getReward(rewardToken, alice);

    // Alice will only get half of the rewards. The other half will be stuck in the contract.
    assertEq(rewardToken.balanceOf(alice), rewardAmount / 2);
    assertEq(rewardToken.balanceOf(address(bathBuddy)), rewardAmount / 2);
}

Recommendation

A possible solution to the issue would be to set the start and end time for the current reward cycle when the first participant joins the reward program, i.e. when the total supply is greater than zero, instead of starting the process in the notifyRewardAmount.

References

The following reports can be used as a reference for the described issue:

• https://0xmacro.com/blog/synthetix-staking-rewards-issue-inefficient-reward-distribution/
• Rewards are not rolled over 2022-09-y2k-finance-findings#93

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

daoio marked the issue as sponsor confirmed

[c4-judge]

HickupHH3 marked the issue as selected for report