Rewards delay release could cause yields steal and loss

[code423n4]

Lines of code

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L78-L97

Vulnerability details

Impact

In the current rewards accounting, vault shares in deposit() and redeem() can not correctly record the spot yields generated by the staked asset. Yields are released over the next rewards cycle. As a result, malicious users can steal yields from innocent users by picking special timing to deposit() and redeem().

Proof of Concept

In syncRewards(), the current asset balance is break into 2 parts: storedTotalAssets and lastRewardAmount/nextRewards. The lastRewardAmount is the surplus balance of the asset, or the most recent yields.

// lib/ERC4626/src/xERC4626.sol
    function syncRewards() public virtual {
        // ...

        uint256 nextRewards = asset.balanceOf(address(this)) - storedTotalAssets_ - lastRewardAmount_;

        storedTotalAssets = storedTotalAssets_ + lastRewardAmount_;

        uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;

        lastRewardAmount = nextRewards.safeCastTo192();
        // ...        
        rewardsCycleEnd = end;
    }

And in the next rewards cycle, lastRewardAmount will be linearly added to storedTotalAssets, their sum is the return value of totalAssets():

    function totalAssets() public view override returns (uint256) {
        // ...

        if (block.timestamp >= rewardsCycleEnd_) {
            // no rewards or rewards fully unlocked
            // entire reward amount is available
            return storedTotalAssets_ + lastRewardAmount_;
        }

        // rewards not fully unlocked
        // add unlocked rewards to stored total
        uint256 unlockedRewards = (lastRewardAmount_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);
        return storedTotalAssets_ + unlockedRewards;
    }

totalAssets() will be referred when deposit() and redeem().

// lib/solmate/src/mixins/ERC4626.sol

    function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {
        require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
        // ...
        _mint(receiver, shares);
        // ...
    }

    function redeem() public virtual returns (uint256 assets) {
        // ...
        require((assets = previewRedeem(shares)) != 0, "ZERO_ASSETS");

        beforeWithdraw(assets, shares);

        _burn(owner, shares);

        // ...

        asset.safeTransfer(receiver, assets);
    }

    function previewDeposit(uint256 assets) public view virtual returns (uint256) {
        return convertToShares(assets);
    }

    function previewRedeem(uint256 shares) public view virtual returns (uint256) {
        return convertToAssets(shares);
    }

    function convertToShares(uint256 assets) public view virtual returns (uint256) {
        uint256 supply = totalSupply; 

        return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
    }

    function convertToAssets(uint256 shares) public view virtual returns (uint256) {
        uint256 supply = totalSupply; 

        return supply == 0 ? shares : shares.mulDivDown(totalAssets(), supply);
    }

Based on the above rules, there are 2 potential abuse cases:

1. If withdraw just after the rewardsCycleEnd timestamp, a user can not get the yields from last rewards cycle. Since the totalAssets() only contain storedTotalAssets but not the yields part. It takes 1 rewards cycle to linearly add to the storedTotalAssets.

Assume per 10,000 asset staking generate yields of 70 for 7 days, and the reward cycle is 1 day. A malicious user Alice can do the following:

• watch the mempool for withdraw(10,000) from account Bob, front run it with syncRewards(), so that the most recent yields of amount 70 from Bob will stay in the vault.
• Alice will also deposit a 10,000 to take as much shares as possible.
• after 1 rewards cycle of 1 day, redeem() to take the yields of 70.

Effectively steal the yields from Bob. The profit for Alice is not 70, because after 1 day, her own deposit also generates some yield, in this example this portion is 1. At the end, Alice steal yield of amount 60.

2. When the Multisig Treasury transfers new yields into the vault, the new yields will accumulate until syncRewards() is called. It is possible that yields from multiple rewards cycles accumulates, and being released in the next cycle.

Knowing that the yields has been accumulated for 3 rewards cycles, a malicious user can deposit() and call syncRewards() to trigger the release of the rewards. redeem() after 1 cycle.

Here the malicious user gets yields of 3 cycles, lose 1 in the waiting cycle. The net profit is 2 cycle yields, and the gained yields should belong to the other users in the vault.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• for the lastRewardAmount not released, allow the users to redeem as it is linearly released later.
• for the accumulated yields, only allow users to redeem the yields received after 1 rewards cycle after the deposit.

[FortisFortuna]

From @denett
syncRewards should be called by us at the beginning of each period, or we need to automatically call it before deposits/withdrawals.

[0xean]

All of the duplicated issues reference a scenario where syncRewards isn't called at the appropriate time leading to the ability for users to steal yield from other users in some fashion. So while they are slightly different I do think grouping them together makes sense as the underlying root cause is the same.

M seems like the appropriate severity for this, as its requires some external factors and doesn't result in principal being lost, only yield.