Incorrect staking rewards due to delay in calling `syncRewards()`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L48 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L48

Vulnerability details

Impact

When syncRewards() is not called immediatly after a cycle end or withdraws do not occurr immediatly after a cycle ends, then totalSupply() calculates a stale value. This causes incorrect calculations of staking rewards.

Proof of Concept

Assume that rewardsCycleLength = 1 week

1. user A deposits 1 frxETH at timestamp = rewardsCycleEnd
2. user B deposits 1 frxETH at timestamp = rewardsCycleEnd + 1 days
3. user A and B get the same amount of shares from their deposit since totalSupply() returns the same value.
4. no withdrawals have occurred and syncRewards has not been called from rewardsCycleEnd until now.
5. a withdrawal occurs or syncRewards is finally called
6. at timestamp rewardsCycleEnd + 1 week they both withdraw and get back the same amount of frxETH (staked amount + staking rewards)

The problem is that user B staked for 1 day less than user A, so her staking reward is bigger than expected while user A's staking reward is smaller than expected.

The same issue arises with withdrawals. If syncRewards() is called with a delay respect to rewardsCycleEnd, the first staker who withdraws after the end of a cycle does not receive the rewards corresponding to the time frame from rewardsCycleEnd to the moment he withdraws.

1. user holds some sfrxETH obtained from staking.
2. No one withdraws or calls syncRewards()
3. user calls withdraw/redeem at timestamp = end of last cycle + 1 days
   • the hook beforeWithdraw() is executed only after calculations of shares/assets to receive (ERC4626.sol#L78), so it only calls syncRewards() after the calculation. The last day since the end of the previous cycle is not taken into account in the amount received by the user.

Tools Used

Manual review

Recommended Mitigation Steps

In sfrxETH.sol override the functions that allow staking/unstaking. Put a call to syncRewards() before each operation.

    function deposit(uint256 assets, address receiver) public override returns (uint256 shares) {
        if (block.timestamp >= rewardsCycleEnd) { syncRewards(); } 
        return super.deposit(assets, receiver)
    }

    function mint(uint256 shares, address receiver) public override returns (uint256 assets) {
        if (block.timestamp >= rewardsCycleEnd) { syncRewards(); } 
        return super.mint(shares, receiver)
    }

    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public override returns (uint256 shares) {
        if (block.timestamp >= rewardsCycleEnd) { syncRewards(); } 
        return super.withdraw(assets, receiver, owner)
    }

    function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public override returns (uint256 assets) {
        if (block.timestamp >= rewardsCycleEnd) { syncRewards(); } 
        return super.redeem(assets, receiver, owner)
    }

With this, the overriding of beforeWithdraw in sfrxETH can be dropped.

[FortisFortuna]

From @denett If the rewards are significant users can call syncRewards() themselves, to protect others to gain an unfair advantage. We could add a call to syncRewards if not yet done before deposits/withdrawals.

[0xean]

dupe of #110