Jeiwan - Gamer rewards are reduced due to fund pulls before accrual of rewards

[sherlock-admin]

Jeiwan

high

Gamer rewards are reduced due to fund pulls before accrual of rewards

Summary

During rebalancing, funds can be pulled from yield generating protocols before rewards (reward per DERBY token) were calculated. Gamers may receive reduced rewards even though the allocated funds remained in protocols over an entire period.

Vulnerability Detail

Gamers are participants who allocate funds to yield generating protocols; as a reward, they receive a share of the yield. Depositing and withdrawing of funds, as well as accruing of rewards, happens during rebalancing of vaults. Vaults can be rebalanced once in two weeks, and, between rebalancings, funds generate yield in third-party protocols.

To track how much yield was generated, and how much rewards gamers will receive, it's require to know the amount of funds deposited into a vault. When rebalancing of a vault starts, the vault sends its balances (including the funds deposited to third-party protocols) to XChainController via the MainVault.pushTotalUnderlyingToController function. The function calls setTotalUnderlying, which computes the funds deposited to protocols and caches the amount in the savedTotalUnderlying variable. Following the logic of yield generation and the accrual of gamer rewards, the share of yield their will receive should be generated from savedTotalUnderlying (it's the amount of funds that have been generating yield in protocols since the previous rebalancing). However, savedTotalUnderlying can be reduced before gamer rewards are calculated, which reduces the rewards.

After a vault has received re-allocated fund amounts from XChainController, it the MainVault.rebalanceXChain function must be called: the function will calculated and send the token amounts that the vault must remove from protocols and send to other vaults via XChainController. If there's not enough funds in the vault, it'll pull funds from protocols and reduce savedTotalUnderlying. Later, when gamer rewards are calculated at a later rebalancing step, savedTotalUnderlying will be smaller than it was when rebalancing started and gamer rewards will be calculated on that smaller savedTotalUnderlying.

In the current implementation, gamer rewards are calculated on the updated vault and protocol balances. The Vault.rebalance function is called at the end of a rebalancing of a vault, after the vault has already sent and received tokens from XChainController. This is wrong because these amount haven't yet generated yield:

1. if, as a result of a rebalancing, the vault has received more funds, the calculated game rewards will be bigger since the protocols haven't yet generated yield and yield may be smaller than expected (APY is calculated based on past results);
2. if the vault has received less funds, the calculated rewards will be smaller since the rewards are calculated on a reduced amount.

   Impact

   Gamers may receive reduced rewards when funds are pulled from yield-generating protocols after a rebalancing has started and before rewards where calculated. Depending on the amount pulled from protocols (which depends on current and new allocations, the cross-chain accounting in XChainController, and balances of other vaults) earning of gamers may be significantly reduced. In the worst case scenario, gamers may even be forced to pay the negative fee penalty when removing allocations.

   Code Snippet

3. savedTotalUnderlying is calculated and set in the beginning of a rebalancing: https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/MainVault.sol#L252
4. Before gamer rewards have been calculated, savedTotalUnderlying can be reduced: https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/MainVault.sol#L310
5. When calculating gamer rewards, a reduced value of savedTotalUnderlying may be used, instead of the value at the beginning of the rebalancing: https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Vault.sol#L144
6. Gamer rewards calculation requires the amount of funds that were used to generate yield: https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/Vault.sol#L234

   Tool used

   Manual Review

   Recommendation

   Consider calculating gamer rewards in the MainVault.pushTotalUnderlyingToController function, after calling setTotalUnderlying. This way, gamer rewards will be calculated on the exact amount of funds deposited into yield-generating protocols based on the allocations suggested by gamers.

This also fixes another problem: in the current implementation, protocol token price change is applied to the updated (rebalanced) vault balance, however the yield is generated on the previous balance (before it's rebalanced). When calculating gamer rewards, protocol token price change is multiplied by the updated vault balance but it should be multiplied by the balance the vault (specifically, a protocol the vault deposited funds to) had at the beginning of the rebalancing.

[mister0y]

No user funds are at risk, only the rewards will be somewhat lower than the percentage we set beforehand.

[mister0y]

Duplicate with #281