syncRewards must be called on all cycles. If one cycle is missed, the reward may be miscalculated.

[code423n4]

Lines of code

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

Vulnerability details

Impact

syncRewards must be called on all cycles. If one cycle is missed, the reward may be miscalculated.

Proof of Concept

    function syncRewards() public virtual {
        uint192 lastRewardAmount_ = lastRewardAmount;
        uint32 timestamp = block.timestamp.safeCastTo32();

        if (timestamp < rewardsCycleEnd) revert SyncError();

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

        storedTotalAssets = storedTotalAssets_ + lastRewardAmount_; // SSTORE

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

        // Combined single SSTORE
        lastRewardAmount = nextRewards.safeCastTo192();
        lastSync = timestamp;
        rewardsCycleEnd = end;

        emit NewRewardsCycle(end, nextRewards);
    }

lastRewardAmount is the amount of rewards distributed in the most recent cycle. If a cycle is skipped, the lastRewardAmount of skipped cycle won't be calculated. It May cause reward lost if asset balance is high and cause reward to be higher than expected if asset balance is low.

Recommended Mitigation Steps

Ensure that this function will always run on every cycle.

[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.

[FortisFortuna]

Duplicate of https://github.com/code-423n4/2022-09-frax-findings/issues/110