update() not getting called right after a WETH amount has been sent will cause users to lose staking rewards

[codehawks-bot]

update() not getting called right after a WETH amount has been sent will cause users to lose staking rewards

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/Staking.sol#L61

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/Fees.sol#L26

Summary

Update not getting called right after a WETH reward addition to Staking.sol will make old stakers lose a portion of their staked amounts.

Vulnerability Details

Staking.sol relies on a measuring system that factors in how many indexes ago a user has staked. When update() gets called after WETH gets added to the system index gets updated with an amount that is going to get distributed to each and every user based on how much they have staked. The formula used to determine the multiplier for each user's staked tokens is the following: amount of new weth * 1e18 / current supply of staking tokens.

Then based on that multiplier the following calculation gets done for each address when updateFor() gets called on it: amount supplied * the current multiplier amount / 1e18.

The issue arises due to update() on Staking.sol not getting called right after WETH gets sent from Fees.sol's sellProfits(). The reward calculations only give users the right amount if balances got updated right after WETH being deposited. This opens the door for such cases:

1. WETH gets sent to Staking.sol. update() doesn't get called.
2. Someone depositing tokens into Staking.sol.
3. then WETH getting sent again, right after that update update() getting called.

This will make everyone's who deposited before the new depositor get less rewards because the total supply of tokens in the contract being different than when update should have been called.

Here is a PoC demonstrating the issue:

https://gist.github.com/CrisCodesCrap/a312eded3c4b57231af1a1df71f7a3be

Impact

Stakers, who have staked before the new staker will lose a portion of their tokens due to the offset staking token balance.

Tools Used

Manual Review

Recommendations

Consider calling Staking.sol's update in Fees.sol's sellProfits().

function sellProfits(address _profits) public {
	...
	IERC20(WETH).transfer(staking, IERC20(WETH).balanceOf(address(this)));
	Staking(staking).update();
}

[qckhp96565463]

[ESCALATE]
This is duplicate of #1223
See Recommendations for both.

[KristianApostolov]

[ESCALATE] This is duplicate of #1223 See Recommendations for both.

I believe the only similarity between the two findings is them recommending update on Staking.sol be called in Fees.sol's WETH transfer function.
Though the recommendation is the same the root cause of the two issues isn't the same.

[PatrickAlphaC]

Other way around, I think this is a dup of #1223