sherlock-audit / 2022-10-astaria-judging

6 stars 1 forks source link

Jeiwan - Liquidity providers can lose funds due to vault share price manipulation #271

Closed sherlock-admin closed 2 years ago

sherlock-admin commented 2 years ago

Jeiwan

high

Liquidity providers can lose funds due to vault share price manipulation

Summary

Liquidity providers can lose funds due to vault share price manipulation

Vulnerability Detail

A malicious actor can manipulate the price of a share in a vault and cause liquidity providers to lose funds. Consider this scenario:

  1. A malicious actor deposits 1 wei of liquidity in a vault via the deposit function (ERC4626-Cloned.sol#L305). They receive 1 wei of share.
  2. Then the attacker sends 10 ETH - 1 wei of liquidity, which inflates the price of a share to 10 ETH per 1 wei of share (the attacker can withdraw 1 share and get 10 ETH of underlying assets).
  3. Anyone who deposits after that will receive shares at the above price. This means that if less than 10 ETH is deposited, 0 shares will be issued (9e18 / 10e18 == 0, ERC4626-Cloned.sol#L400), i.e. the depositor won't be able to redeem their funds.
  4. The malicious actor can redeem their share and steal liquidity provided by the good liquidity provider.

    Impact

    Liquidity provided to vaults can be stolen by a malicious actor.

    Code Snippet

    ERC4626-Cloned.sol#L305:

    function deposit(uint256 assets, address receiver)
    public
    virtual
    override(IVault)
    returns (uint256 shares)
    {
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
    
    // Need to transfer before minting or ERC777s could reenter.
    ERC20(underlying()).safeTransferFrom(msg.sender, address(this), assets);
    
    _mint(receiver, shares);
    
    emit Deposit(msg.sender, receiver, assets, shares);
    
    afterDeposit(assets, shares);
    }

ERC4626-Cloned.sol#L392:

function convertToShares(uint256 assets)
  public
  view
  virtual
  returns (uint256)
{
  uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.

  return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
}

Tool used

Manual Review

Recommendation

Consider requiring a minimal initial deposit amount in a vault. Also, consider checking that the amount of issued shares is always positive.

Duplicate of #143