code-423n4 / 2023-07-pooltogether-findings

12 stars 7 forks source link

Vault.sol is vulnerable to the classic vault's first depositor issue #347

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L857-L874

Vulnerability details

Impact

Users may suffer loss, could be in the form of the first depositor manipulating the assets per share ratio or even front running the first depositor

Proof of Concept

Vault.sol#L857-L874)

  /**
   * @inheritdoc ERC4626
   * @param _assets Amount of assets to convert
   * @param _rounding Rounding mode (i.e. down or up)
   * @return uint256 Amount of shares for the assets
   */
  function _convertToShares(
    uint256 _assets,
    Math.Rounding _rounding
  ) internal view virtual override returns (uint256) {
    uint256 _exchangeRate = _currentExchangeRate();

    return
      (_assets == 0 || _exchangeRate == 0)
        ? _assets
        : _assets.mulDiv(_assetUnit, _exchangeRate, _rounding);
  }

And Vault.sol#L876-L887

  /**
   * @inheritdoc ERC4626
   * @param _shares Amount of shares to convert
   * @param _rounding Rounding mode (i.e. down or up)
   * @return uint256 Amount of assets represented by the shares
   */
  function _convertToAssets(
    uint256 _shares,
    Math.Rounding _rounding
  ) internal view virtual override returns (uint256) {
    return _convertToAssets(_shares, _currentExchangeRate(), _rounding);
  }

Tool used

Recommended Mitigation Steps

Implement a minimum share or curb this the way Uniswap does, by burning the first set of tokens.

Assessed type

Context

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Insufficient proof