Attacker can withdraw asset without burning any shares

Title

Affected smart contract

https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/abstracts/BaseVault.sol#L271

Description

Function BaseVault.withdraw() is implemented as follows:

/// url = https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/abstracts/BaseVault.sol#L396-L422
  function withdraw(
    uint256 assets,
    address receiver,
    address owner
  )
    public
    override
    returns (uint256)
  {
    if (assets == 0 || receiver == address(0) || owner == address(0)) {
      revert BaseVault__withdraw_invalidInput();
    }

    if (assets > maxWithdraw(owner)) {
      revert BaseVault__withdraw_moreThanMax();
    }

    address caller = _msgSender();
    if (caller != owner) {
      _spendAllowance(owner, caller, receiver, convertToShares(assets));
    }

    /// [$audit-high] vulnerable here  
    uint256 shares = previewWithdraw(assets);
    _withdraw(caller, receiver, owner, assets, shares);

    return shares;
  }

After a lot of checks, function withdraw() calls to the previewWithdraw(assets) to get the shares coressponding to the amount of assets that the user want to withdraw. The vulnerability of this calculation is previewWithdraw() use the round-down instead of round-up.

/// url = https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/abstracts/BaseVault.sol#L270-L272
  function previewWithdraw(uint256 assets) public view virtual override returns (uint256) {
    return _convertToShares(assets, Math.Rounding.Down);
  }

/// url = https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/abstracts/BaseVault.sol#L492-L503
  function _convertToShares(
    uint256 assets,
    Math.Rounding rounding
  )
    internal
    view
    virtual
    returns (uint256 shares)
  {
    uint256 supply = totalSupply();
    return (assets == 0 || supply == 0) ? assets : assets.mulDiv(supply, totalAssets(), rounding);
  }

In case the assets * supply < totalAssets(), the shares will get the value 0. It creates a big flaw for the attacker to steal the tokens of the vault. See the section below for more details of the attacks.

Attack scenario

We shall assume the state of vault as follows:

totalSupply() = 100
totalAssets() = 1000

Attacker call BaseVault.withdraw(1, attacker_address, attacker_address). The corresponding shares to be burned is 1 * 100 / 1000 = 0. After the call, the attacker will get 1 asset tokens without burn any shares of him. The attacker can keep doing this until totalSupply() < totalAssets() and get a lot of money.

The PR to the attack test: https://github.com/Fujicracy/fuji-v2/pull/299

Recommendation

Calculate the burnedShares by using round-up instead of round-down calculation

function previewWithdraw(uint256 assets) public view virtual override returns (uint256) {
    return _convertToShares(assets, Math.Rounding.Up);
}

Fujicracy / fuji-v2