PublicVault logics doesn't support deflationary or rebasing tokens and if attacker create Vault with those type of tokens as underlying token then users would lose funds

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L384-L387
https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L29
https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L45
https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/WithdrawProxy.sol#L254-L284

Vulnerability details

Impact

Liquidity providers deposits their underlying asset into the PublicVaults and receive vault-token instead. in all the logics of the code where it tries to transfer underlying asset from user or between contracts code assumes that real transferred amount is what specified in the transfer and perform all calculation based on this amount but if the transferred amount was lower than specified amount (deflationary tokens) then most of the calculation would go wrong and users would lose funds.

Proof of Concept

These are some of the transfer logics in deposit() and mint() code:

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

  function mint(
    uint256 shares,
    address receiver
  ) public virtual returns (uint256 assets) {
    assets = previewMint(shares); // No need to check for rounding error, previewMint rounds up.
    require(assets > minDepositAmount(), "VALUE_TOO_SMALL");
    // Need to transfer before minting or ERC777s could reenter.
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), assets);
    _mint(receiver, shares);
    emit Deposit(msg.sender, receiver, assets, shares);
    afterDeposit(assets, shares);
  }

As you can see code calculates shares based on the amount user specified and not the real amount of the tokens transferred to the contract so contract would receive less tokens than user specified if asset was deflationary token while user would receive higher share amount (stealing other users shares).

This is where PublicVault sends assets to WithdrawProxy in transferWithdrawReserve() function:

      ERC20(asset()).safeTransfer(currentWithdrawProxy, withdrawBalance);
      WithdrawProxy(currentWithdrawProxy).increaseWithdrawReserveReceived(
        withdrawBalance
      );

As you can see code assumes that WithdrawProxy would receive the exact amount of assets that are specified in transfer but if token was deflationary WithdrawProxy can receive less amount and WithdrawReserveReceived would show wrong amount in the WithdrawProxy (higher than real amount received) which can cause underflow in some logics and funds can be locked in WithdrawProxy.

Tools Used

VIM

Recommended Mitigation Steps

calculate real transferred amount or whitelist supported tokens or blacklist popular deflationary tokens.

[c4-judge]

Picodes marked the issue as duplicate of #51

[c4-judge]

Picodes marked the issue as satisfactory