Jeiwan - An inactive vault can disrupt rebalancing of active vaults

[sherlock-admin]

Jeiwan

medium

An inactive vault can disrupt rebalancing of active vaults

Summary

An inactive vault can send its total underlying amount to the XChainController and disrupt rebalancing of active vaults by increasing the underlyingReceived counter:

1. if pushVaultAmounts is called before underlyingReceived overflows, rebalancing of one of the active vault may get stuck since the vault won't receive XChain allocations;
2. if pushVaultAmounts after all active vaults and at least one inactive vault has reported their underlying amounts, rebalancing of all vaults will get stuck.

   Vulnerability Detail

   Rebalancing of vaults starts when Game.pushAllocationsToController is called. The function sends the allocations made by gamers to the XChainController. XChainController receives them in the receiveAllocationsFromGame function. In the settleCurrentAllocation function, a vault is marked as inactive if it has no allocations and there are no new allocations for the vault. receiveAllocationsFromGameInt remembers the number of active vaults.

The next step of the rebalancing process is reporting vault underlying token balances to the XChainController by calling MainVault.pushTotalUnderlyingToController. As you can see, the function can be called in an inactive vault (the only modifier of the function, onlyWhenIdle, doesn't check that vaultOff is false). XChainController receives underlying balances in the setTotalUnderlying function: notice that the function increases the number of balances it has received.

Next step is the XChainController.pushVaultAmounts function, which calculates how much tokens each vault should receive after gamers have changed their allocations. The function can be called only when all active vaults have reported their underlying balances:

modifier onlyWhenUnderlyingsReceived(uint256 _vaultNumber) {
  require(
    vaultStage[_vaultNumber].underlyingReceived == vaultStage[_vaultNumber].activeVaults,
    "Not all underlyings received"
  );
  _;
}

However, as we saw above, inactive vaults can also report their underlying balances and increase the underlyingReceived counter–if this is abused mistakenly or intentionally (e.g. by a malicious actor), vaults may end up in a corrupted state. Since all the functions involved in rebalancing are not restricted (including pushTotalUnderlyingToController and pushVaultAmounts), a malicious actor can intentionally disrupt accounting of vaults or block a rebalancing.

Impact

1. If an inactive vault reports its underlying balances instead of an active vault (i.e. pushVaultAmounts is called when underlyingReceived is equal activeVaults), the active vault will be excluded from rebalancing and it won't receive updated allocations in the current period. Since the rebalancing interval is 2 weeks, the vault will lose the increased yield that might've been generated thanks to new allocations.
2. If an inactive vault reports its underlying balances in addition to all active vaults (i.e. pushVaultAmounts is called when underlyingReceived is greater than activeVaults), then pushVaultAmounts will always revert and rebalancing will get stuck.

   Code Snippet

3. An inactive vault can send its underlying balance to the XChainController: https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/MainVault.sol#L249
4. The XChainController can receive underlying balances from inactive vaults: https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/XChainController.sol#L264
5. underlyingReceived is increased when underlying balances are received: https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/XChainController.sol#L288
6. pushVaultAmounts can only be executed when the number of vaults that have reported their balances equals the number of active vaults: https://github.com/sherlock-audit/2023-01-derby/blob/main/derby-yield-optimiser/contracts/XChainController.sol#L298

   Tool used

   Manual Review

   Recommendation

   In the MainVault.pushTotalUnderlyingToController function, consider disallowing inactive vaults (vaults that have vaultOff set to true) report their underlying balances.

[mister0y]

Low severity because funds are not at risk and the guardian can reset this.

[Jeiwan]

Escalate for 10 USDC

This is a medium severity issue. It's true that the guardian can reset underlyingReceived, however, the attacker can call XChainController.pushVaultAmounts in the same transaction and proceed rebalancing to the next stage–this cannot be reset by the guardian. I.e. the attacker would report the balance of an inactive vault and call XChainController.pushVaultAmounts in the same transaction to apply the effect immediately.

The XChainController.pushVaultAmounts function rebalances vault amounts using the balances they reported (including balances reported by inactive vaults) and sends cross-chain messages to the vaults to let them know their updated balances. The guardian cannot revert rebalancing, thus it won't be able to reset the balance of an inactive vault after XChainController.pushVaultAmounts was called.

[sherlock-admin]

Escalate for 10 USDC

This is a medium severity issue. It's true that the guardian can reset underlyingReceived, however, the attacker can call XChainController.pushVaultAmounts in the same transaction and proceed rebalancing to the next stage–this cannot be reset by the guardian. I.e. the attacker would report the balance of an inactive vault and call XChainController.pushVaultAmounts in the same transaction to apply the effect immediately.

The XChainController.pushVaultAmounts function rebalances vault amounts using the balances they reported (including balances reported by inactive vaults) and sends cross-chain messages to the vaults to let them know their updated balances. The guardian cannot revert rebalancing, thus it won't be able to reset the balance of an inactive vault after XChainController.pushVaultAmounts was called.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation accepted

After further discussions with the Sponsor considering this issue a valid medium

[sherlock-admin]

Escalation accepted

After further discussions with the Sponsor considering this issue a valid medium

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[Theezr]

Fix: https://github.com/derbyfinance/derby-yield-optimiser/pull/197