A big fraction of reward WETH can get locked with a donation attack

[codehawks-bot]

A big fraction of reward WETH can get locked with a donation attack

Severity

High Risk

Relevant GitHub Links

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

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

Summary

A donation of staking tokens to the Staking.sol contract makes a proportion of all WETH fill-ups be locked forever due to the accounting being done with the real balance instead of with a tracking number.

Vulnerability Details

The calculation of how shares of each WETH refill should be split between stakers has a fundamental flaw. It resides in the use of TKN.balanceOf(address(this)) instead of using a tracking variable in the same manner as balance.

function update() public {
        uint256 totalSupply = TKN.balanceOf(address(this));
        if (totalSupply > 0) {
            uint256 _balance = WETH.balanceOf(address(this));
            if (_balance > balance) {
                uint256 _diff = _balance - balance;
                if (_diff > 0) {
										// @audit using totalSupply to divide the amount leads to funds being allocated to donated tokens
                    uint256 _ratio = _diff * 1e18 / totalSupply;
                    if (_ratio > 0) {
                      index = index + _ratio;
                      balance = _balance;
                    }
                }
            }
        }
    }

Using the actual balance instead of the variable allows malicious users to donate to the contract, thus making it allocate rewards for tokens, which do not belong to anyone, thus locking them from ever being claimed.

Consider the following PoC as it demonstrates the vector:
https://gist.github.com/CrisCodesCrap/05775e07dd98e2673d0196d6c41b3773

Impact

A portion of all of the upcoming fill-ups will be locked forever.

Tools Used

Manual Review

Recommendations

Consider implementing the accounting and calculations of the whole contract with tracking variables instead of with real balances. Keep the amount of staked tokens in a tracking variable in the same manner as the balance variable used for keeping track of WETH.

[KristianApostolov]

I believe this issue should be the same issue as #1226. They should be medium-severity due to any amount forfeited by the attacker once will continue causing damage indefinitely. Also, the judging criteria seems to be pointing to the same due to funds theoretically being at risk. https://docs.codehawks.com/rewards-and-judging

• Medium:
  • Funds are indirectly at risk
  • Disruption of protocol functionality or availability

I believe it should not be a duplicate of #2092 because #2092 is about when update() is called and this issue touches on how update()'s logic is flawed. In conclusion, the two issues have different root causes from my point of view.