Deposit backrunning a WETH transaction to the contract with a deposit locks a fraction of those funds

[codehawks-bot]

Deposit backrunning a WETH transaction to the contract with a deposit locks a fraction of those funds

Severity

High Risk

Relevant GitHub Links

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

Summary

A CEI violation and bad state update sequence in Staking.sol's deposit() allows for the locking of reward WETH.

Vulnerability Details

The staking protocol gets WETH used for rewarding stakers by it getting sent through an ERC20 transfer. The issue arises due to deposit() first taking in the staking tokens instead of first updating the balances.

function deposit(uint _amount) external {
        TKN.transferFrom(msg.sender, address(this), _amount); // @audit tokens getting to the contract first
        updateFor(msg.sender);
        balances[msg.sender] += _amount;
    }

This leads to an offset in the delta calculation in the update right after the transfer.

// @audit totalSupply is now bigger because of the deposit of user tokens just before that
uint256 _ratio = _diff * 1e18 / totalSupply;

This will make the protocol calculation lock a specific amount of WETH for the deposited amount of staking tokens as well.

Here is a PoC demonstrating the issue at hand:
https://gist.github.com/CrisCodesCrap/38cb06e3864ace97734a1d86a61de733

Impact

The delta ratio calculation will be offset, which will lead to some amount staking reward loss for every staker in the contract.

Tools Used

Manual Review

Recommendations

Consider keeping the balances of tokens in the protocol only as accounting variables, not as the real values the contract owns. A good example would the how the WETH balance in the contract is handled. Also, consider following the CEI pattern in this function and in every other one in the protocol. https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html

[PatrickAlphaC]

I do see a similar contract here: https://etherscan.io/token/0xc5bddf9843308380375a611c18b50fb9341f502a#code

Need project approval

[KristianApostolov]

I believe this is an issue with a different root cause compared to #2092. The issue here is that there is a CEI violation in deposit(), which leads to the protocol doing calculations with the old state, and #2092 is about update() got getting explicitly called and how it becomes an issue. Though the issues look similar they are completely different in nature and in my opinion should not be duplicates of each other.

[PatrickAlphaC]

I think they are one and the same. When update gets call and whether it gets called.