Inaccuracy of share price calculation when depositing which causes unfairness

[hats-bug-reporter[bot]]

Github username: @koolexcrypto Submission hash (on-chain): 0x6c3df3b22dba0a43196420ab376acaa349eae6d70a7f87fcf3b800fb885e6bb2 Severity: high

Description:

Description

_deposit method is used to process user deposits. The user deposits ETH and receives shares. The received shares is calculated (with rounding up) as follows: shares = assets * totalShares / _totalAssets

This is done by calling _convertToShares

    // calculate amount of shares to mint
    shares = _convertToShares(assets, Math.Rounding.Up);

Code link

Please note that before calculating the amount of shares, _checkHarvested is called to check whether the vault is harvested.

  function _deposit(
    address to,
    uint256 assets,
    address referrer
  ) internal virtual returns (uint256 shares) {
    _checkHarvested();

Code link

This is to ensure an accurate and fair price of the share before the deposit as _totalAssets and _totalShares are likely to be updated when harvesting. However, _totalAssets and _totalShares could possibly be updated when updating the exit queue and there is no check if the exist queue can be updated. Therefore, the share price could possibly be inaccurate which is unfair. The only case where the issue doesn't occur is when calling updateStateAndDeposit. Otherwise, all deposits that come via calling deposit or by sending ETH directly to the contract (which triggers the fallback that calls _deposit) are vulnerable to inaccurate share price.

Attack Scenario

• Exit queue can be updated every 24 hours (or longer)
• Assume the exist queue can be updated at this moment.
• Bob sent ETH to the contract.
• Bob received shares. However, the calculated amount of shares is inaccurate due to the fact that exit queue is updatable but wasn't called.
• After a second, exit queue was updated. Therefore, _totalAssets and _totalShares were updated.
• Bob's shares now could possibly have less or more value (this depends on the calculations happening in _updateExitQueue) which results in an unfair distribution of shares.

Attachments

1. Proof of Concept (PoC)

• deposit method and fallback which calls _deposit

  function deposit(
  address receiver,
  address referrer
  ) public payable virtual override returns (uint256 shares) {
  return _deposit(receiver, msg.value, referrer);
  }
  .
  .
  .
  receive() external payable virtual {
  _deposit(msg.sender, msg.value, address(0));
  }

  Code link: deposit Code link: receive

• _deposit method where _checkHarvested is called but no check if exist queue can be updated

  function _deposit(
  address to,
  uint256 assets,
  address referrer
  ) internal virtual returns (uint256 shares) {
  _checkHarvested();
  if (to == address(0)) revert Errors.ZeroAddress();
  if (assets == 0) revert Errors.InvalidAssets();
  
  uint256 totalAssetsAfter;
  unchecked {
    // cannot overflow as it is capped with underlying asset total supply
    totalAssetsAfter = _totalAssets + assets;
  }
  if (totalAssetsAfter > capacity()) revert Errors.CapacityExceeded();
  
  // calculate amount of shares to mint
  shares = _convertToShares(assets, Math.Rounding.Up);

  Code link

• _updateExitQueue method where _totalShares and _totalAssets could possibly be updated

      .
      .
      .
  // update state
  _totalShares -= SafeCast.toUint128(burnedShares);
  _totalAssets -= SafeCast.toUint128(exitedAssets);

Code link

• _convertToShares method for conversion from assets to shares.

  function _convertToShares(
  uint256 assets,
  Math.Rounding rounding
  ) internal view returns (uint256 shares) {
  uint256 totalShares = _totalShares;
  // Will revert if assets > 0, totalShares > 0 and _totalAssets = 0.
  // That corresponds to a case where any asset would represent an infinite amount of shares.
  return
    (assets == 0 || totalShares == 0)
      ? assets
      : Math.mulDiv(assets, totalShares, _totalAssets, rounding);
  }

  Code link

Note: I've set the severity to high due to the fact that exist queue shares could be big within 24 hours. Therefore, causing unfairness in the protocol that's non-negligible.

Recommend mitigation

In _deposit, call _updateExitQueue if exist queue can be updated before any calculation of the amount of shares

An example:

if (canUpdateExitQueue()) {
  _updateExitQueue();
}

[tsudmi]

When exit queue update is called it decreases _totalAssets and _totalShares proportionally, so it shouldn't affect the conversion rate. It's like someone withdraws before you deposit, the rate should stay the same.