Astaria contest - unforgiven's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L169-L178 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L114-L167

Vulnerability details

Impact

Function safeTransferFrom() in ClearingHouse calls _execute() and pays the auction price and settles the auction when a collateral is liquidated. but there is two issue, first there is no check that who is calling safeTransferFrom() and anyone can call it, the second issue is that paymentToken is decode from caller provided info and there is no validation for it and code uses its value to transfer tokens. attacker can use this issues can call safeTransferFrom() for any ongoing auction and set paymentToken as fake one (he can create one) and then code would pay loans with this fake token and would settle auction and close loans all with a fake token. this would cause liquidity provider to lose funds and also to collateral owner to lose funds and attacker can break the basic feature of the protocol.

Proof of Concept

This is safeTransferFrom() code in ClearingHouse:

  function safeTransferFrom(
    address from, // the from is the offerer
    address to,
    uint256 identifier,
    uint256 amount,
    bytes calldata data //empty from seaport
  ) public {
    //data is empty and useless
    _execute(from, to, identifier, amount);
  }

As you can see there is no check and anyone can call this function. it calls execute() which is:

  function _execute(
    address tokenContract, // collateral token sending the fake nft
    address to, // buyer
    uint256 encodedMetaData, //retrieve token address from the encoded data
    uint256 // space to encode whatever is needed,
  ) internal {
    IAstariaRouter ASTARIA_ROUTER = IAstariaRouter(_getArgAddress(0)); // get the router from the immutable arg

    ClearingHouseStorage storage s = _getStorage();
    address paymentToken = bytes32(encodedMetaData).fromLast20Bytes();

    uint256 currentOfferPrice = _locateCurrentAmount({
      startAmount: s.auctionStack.startAmount,
      endAmount: s.auctionStack.endAmount,
      startTime: s.auctionStack.startTime,
      endTime: s.auctionStack.endTime,
      roundUp: true //we are a consideration we round up
    });
    uint256 payment = ERC20(paymentToken).balanceOf(address(this));

    require(payment >= currentOfferPrice, "not enough funds received");

    uint256 collateralId = _getArgUint256(21);
    // pay liquidator fees here

    ILienToken.AuctionStack[] storage stack = s.auctionStack.stack;

    uint256 liquidatorPayment = ASTARIA_ROUTER.getLiquidatorFee(payment);

    ERC20(paymentToken).safeTransfer(
      s.auctionStack.liquidator,
      liquidatorPayment
    );

    ERC20(paymentToken).safeApprove(
      address(ASTARIA_ROUTER.TRANSFER_PROXY()),
      payment - liquidatorPayment
    );

    ASTARIA_ROUTER.LIEN_TOKEN().payDebtViaClearingHouse(
      paymentToken,
      collateralId,
      payment - liquidatorPayment,
      s.auctionStack.stack
    );

    if (ERC20(paymentToken).balanceOf(address(this)) > 0) {
      ERC20(paymentToken).safeTransfer(
        ASTARIA_ROUTER.COLLATERAL_TOKEN().ownerOf(collateralId),
        ERC20(paymentToken).balanceOf(address(this))
      );
    }
    ASTARIA_ROUTER.COLLATERAL_TOKEN().settleAuction(collateralId);
  }

As you can see there is no check for msg.sender here too and the logics can be called by anyone. in the line address paymentToken = bytes32(encodedMetaData).fromLast20Bytes() code retrieve payment token address from user provided data in the call and then code uses paymentToken to pay back the loans and settle the auction and pay liquidator fee. attacker can use this issue and call safeTransferFrom() for any ongoing auction and send fake token as paymentToken and cause liquidity provider, liquidator and collateral owner to lose funds. there is no check in the code to see that paymentToken is valid. code calls LienToken.payDebtViaClearingHouse() to pay the loans in the functions payDebtViaClearingHouse() and _payDebt() and _paymentAH():

  function payDebtViaClearingHouse(
    address token,
    uint256 collateralId,
    uint256 payment,
    AuctionStack[] memory auctionStack
  ) external {
    LienStorage storage s = _loadLienStorageSlot();
    require(
      msg.sender == address(s.COLLATERAL_TOKEN.getClearingHouse(collateralId))
    );

    _payDebt(s, token, payment, msg.sender, auctionStack);
    delete s.collateralStateHash[collateralId];
  }

  function _payDebt(
    LienStorage storage s,
    address token,
    uint256 payment,
    address payer,
    AuctionStack[] memory stack
  ) internal returns (uint256 totalSpent) {
    uint256 i;
    for (; i < stack.length;) {
      uint256 spent;
      unchecked {
        spent = _paymentAH(s, token, stack, i, payment, payer);
        totalSpent += spent;
        payment -= spent;
        ++i;
      }
    }
  }
  function _paymentAH(
    LienStorage storage s,
    address token,
    AuctionStack[] memory stack,
    uint256 position,
    uint256 payment,
    address payer
  ) internal returns (uint256) {
    uint256 lienId = stack[position].lienId;
    uint256 end = stack[position].end;
    uint256 owing = stack[position].amountOwed;
    //checks the lien exists
    address payee = _getPayee(s, lienId);
    uint256 remaining = 0;
    if (owing > payment.safeCastTo88()) {
      remaining = owing - payment;
    } else {
      payment = owing;
    }
    if (payment > 0)
      s.TRANSFER_PROXY.tokenTransferFrom(token, payer, payee, payment);

    delete s.lienMeta[lienId]; //full delete
    delete stack[position];
    _burn(lienId);

    if (_isPublicVault(s, payee)) {
      IPublicVault(payee).updateAfterLiquidationPayment(
        IPublicVault.LiquidationPaymentParams({remaining: remaining})
      );
    }
    emit Payment(lienId, payment);
    return payment;
  }

As you can see there is no check that token is valid token and code trust the value ClearingHouse sends. to exploit this attacker need a simple call to ClearingHouse.safeTransferFrom() and provide a fake ERC20 token and cause fund loss and greifing for all actors of the protocol. This is critical issue because attacker can perform the attack for all auctions and cause fund loss for corresponding liquidity providers and collateral owners. it would break the basic feature of the protocol too. protocol would close loans and auctions by transferring fake token.

Tools Used

VIM

Recommended Mitigation Steps

validate token value

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L314-L335 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L479-L493

Vulnerability details

Impact

When users withdraw their vault tokens PublicVault mint WithdrawProxy's shares token for them and at the end of the epoch PublicVault would calculated WithdrawProxy's assets and update PublicVault assets and start the next epoch. if a lot of users withdraws their funds then the value of the totalAssets().mulDivDown(s.liquidationWithdrawRatio, 1e18) (the amount belongs to the WithdrawProxy) would be higher than yIntercept and code would revert because of the underflow when setting the new value of the yIntercept. This would cause last users to not be able to withdraw their funds and contract epoch system to be broken for a while.

Proof of Concept

This is part of processEpoch() code that calculates ratio between WithdrawProxy and PublicVault:

  function processEpoch() public {
.....
.....
    // reset liquidationWithdrawRatio to prepare for re calcualtion
    s.liquidationWithdrawRatio = 0;

    // check if there are LPs withdrawing this epoch
    if ((address(currentWithdrawProxy) != address(0))) {
      uint256 proxySupply = currentWithdrawProxy.totalSupply();

      s.liquidationWithdrawRatio = proxySupply
        .mulDivDown(1e18, totalSupply())
        .safeCastTo88();

      currentWithdrawProxy.setWithdrawRatio(s.liquidationWithdrawRatio);
      uint256 expected = currentWithdrawProxy.getExpected();

      unchecked {
        if (totalAssets() > expected) {
          s.withdrawReserve = (totalAssets() - expected)
            .mulWadDown(s.liquidationWithdrawRatio)
            .safeCastTo88();
        } else {
          s.withdrawReserve = 0;
        }
      }
      _setYIntercept(
        s,
        s.yIntercept -
          totalAssets().mulDivDown(s.liquidationWithdrawRatio, 1e18)
      );
      // burn the tokens of the LPs withdrawing
      _burn(address(this), proxySupply);
    }

As you can see in the line _setYIntercept(s, s.yIntercept - totalAssets().mulDivDown(s.liquidationWithdrawRatio, 1e18)) code tries to set new value for yIntercept but This is totalAssets() code:

  function totalAssets()
    public
    view
    virtual
    override(ERC4626Cloned)
    returns (uint256)
  {
    VaultData storage s = _loadStorageSlot();
    return _totalAssets(s);
  }

  function _totalAssets(VaultData storage s) internal view returns (uint256) {
    uint256 delta_t = block.timestamp - s.last;
    return uint256(s.slope).mulDivDown(delta_t, 1) + uint256(s.yIntercept);
  }

So as you can see totalAssets() can be higher than yIntercept and if most of the user withdraw their funds(for example the last user) then the value of liquidationWithdrawRatio would be near 1 too and the value of totalAssets().mulDivDown(s.liquidationWithdrawRatio, 1e18) would be bigger than yIntercept and call to processEpoch() would revert and code can't start the next epoch and user withdraw process can't be finished and funds would stuck in the contract.

Tools Used

VIM

Recommended Mitigation Steps

prevent underflow by calling accrue() in the begining of the processEpoch()

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/WithdrawProxy.sol#L268-L279

Vulnerability details

Impact

Function Claim() in WithdrawProxy return any excess funds to the PublicVault, according to the withdrawRatio between withdrawing and remaining LPs. but when code tries to calculating return amount according to the withdrawRatio it uses asset's decimals instead of 1e18 and if asset's decimals where different then the calculation would go wrong and multiple things can happen:

1. underflow would happens and calls would revert (when transferring assets) and users can't withdraw their funds and protocol would be in broken state.
2. rewards would get distributed wrongly
3. withdrawal funds would be transferred to PublicVault and users would lose their funds.

Proof of Concept

This is part of claim() function in WithdrawProxy which handles transferring excess funds to PublicVault:

  function claim() public {
....
....
    uint256 transferAmount = 0;
    uint256 balance = ERC20(asset()).balanceOf(address(this)) -
      s.withdrawReserveReceived; // will never underflow because withdrawReserveReceived is always increased by the transfer amount from the PublicVault

    if (balance < s.expected) {
      PublicVault(VAULT()).decreaseYIntercept(
        (s.expected - balance).mulWadDown(1e18 - s.withdrawRatio)
      );
    } else {
      PublicVault(VAULT()).increaseYIntercept(
        (balance - s.expected).mulWadDown(1e18 - s.withdrawRatio)
      );
    }

    if (s.withdrawRatio == uint256(0)) {
      ERC20(asset()).safeTransfer(VAULT(), balance);
    } else {
      transferAmount = uint256(s.withdrawRatio).mulDivDown(
        balance,
        10**ERC20(asset()).decimals()
      );

      unchecked {
        balance -= transferAmount;
      }

      if (balance > 0) {
        ERC20(asset()).safeTransfer(VAULT(), balance);
      }
    }
    s.finalAuctionEnd = 0;

    emit Claimed(address(this), transferAmount, VAULT(), balance);
  }

As you can see when code tries to calculate transferAmount it uses 10**ERC20(asset()).decimals() as dominator but withdrawRatio is calculated with 1e18 factor so the value of transferAmount would be wrong. This is where withdraw ratio is calculated in the PublicVault and 1e18 has been used:

      s.liquidationWithdrawRatio = proxySupply
        .mulDivDown(1e18, totalSupply())
        .safeCastTo88();

This issue can have multiple impact:

1. if asset's decimals were lower than 18 then the value of the transferAmount would be very higher than balance and unchecked { balance -= transferAmount; } would underflow and the value of balance would be very high and asset transfer would fail always and code would revert. so calls to claim() would fail and code would never set finalAuctionEnd = 0 and this would cause calls to PublicVault.processEpoch() to fail and PublicVault would be in broken state forever users would lose access to their funds.
2. if asset's decimals were higher than 18 then the value of the transferAmount would be very low and code would consider those funds as excess amount and withdrawal users funds would get transferred to PublicVault and withdrawal users would lose their funds.

(the bug and impact is obvious so the detailed POC is not included)

Tools Used

VIM

Recommended Mitigation Steps

use 1e18 to calculate transferAmount

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L229-L244 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L287-L306 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L379-L395

Vulnerability details

Impact

users can get loan for their collaterals by calling commitToLien() and code checks that caller is the holder of the collateral or receiver of the loan is holder or operator of the collateral (ERC721 operator). This would give attacker opportunity to issue unwanted loans(bad interest rate or other bad loan parameters) for collateral owners and cause collateral owner to lose funds because of the unfair loans and fee. in the extreme case where collateral owner sets another contract as operator (it's common to set AstariaRouter as collateral operator because users interact with router and router manage their funds in behalf of them) attacker can create a loan for user collateral for that operator and those loans would stuck in the operator address and user collateral would get liquidated after the loan duration. This is a critical issue because attacker can make all the collateral owners to lose their NFT and also break the protocol basic feature (attacker can create unwanted loans for everyone).

Proof of Concept

This is commitToLien() and _requestLienAndIssuePayout() code:

  function commitToLien(
    IAstariaRouter.Commitment calldata params,
    address receiver
  )
    external
    whenNotPaused
    returns (uint256 lienId, ILienToken.Stack[] memory stack, uint256 payout)
  {
    _beforeCommitToLien(params);
    uint256 slopeAddition;
    (lienId, stack, slopeAddition, payout) = _requestLienAndIssuePayout(
      params,
      receiver
    );
    _afterCommitToLien(
      stack[stack.length - 1].point.end,
      lienId,
      slopeAddition
    );
  }

  function _requestLienAndIssuePayout(
    IAstariaRouter.Commitment calldata c,
    address receiver
  )
    internal
    returns (
      uint256 newLienId,
      ILienToken.Stack[] memory stack,
      uint256 slope,
      uint256 payout
    )
  {
    _validateCommitment(c, receiver);
    (newLienId, stack, slope) = ROUTER().requestLienPosition(c, recipient());
    payout = _handleProtocolFee(c.lienRequest.amount);
    ERC20(asset()).safeTransfer(receiver, payout);
  }

As you can see this functions validate loan commitment and sends loan to receiver address which msg.sender specified. to validate commitment code calls _validateCommitment() which is:

  function _validateCommitment(
    IAstariaRouter.Commitment calldata params,
    address receiver
  ) internal view {
    uint256 collateralId = params.tokenContract.computeId(params.tokenId);
    ERC721 CT = ERC721(address(COLLATERAL_TOKEN()));
    address holder = CT.ownerOf(collateralId);
    address operator = CT.getApproved(collateralId);
    if (
      msg.sender != holder &&
      receiver != holder &&
      receiver != operator &&
      !CT.isApprovedForAll(holder, msg.sender)
    ) {
      revert InvalidRequest(InvalidRequestReason.NO_AUTHORITY);
    }
    VIData storage s = _loadVISlot();
    address recovered = ecrecover(
      keccak256(
        _encodeStrategyData(
          s,
          params.lienRequest.strategy,
          params.lienRequest.merkle.root
        )
      ),
      params.lienRequest.v,
      params.lienRequest.r,
      params.lienRequest.s
    );
    if (
      (recovered != owner() && recovered != s.delegate) ||
      recovered == address(0)
    ) {
      revert IVaultImplementation.InvalidRequest(
        InvalidRequestReason.INVALID_SIGNATURE
      );
    }
  }

As you can see code validates loan when receiver of the loan is holder of the collateral or operator of the collateral (to support AstariaRouter actions on behalf of the user). There is no other checks or limits to prevent attacker from calling commitToLien() and issue loan for other user's collateral. attacker can use this and create unwanted loans for user or operator and cause funds loss. these are the steps attacker would do:

1. user1 deposits his NFT as collateral and receives collateral token NFT.
2. user1 sets AstariaRouter as his collateral token operator (or any other contract) to call AstariaRouter functions and manage his funds.
3. attacker would call commitToLien(user1's collateral, AstariaRouter) for user1 collateral and set AstariaRouter as receiver of the loan and code would check and validate this call and would create a loan for user1's collateral and send tokens to AstariaRouter address.
4. AstariaRouter has no logic to handle loans and pay them back and after loan duration user1's collateral would get liquidated and user1 would lose his funds and receive no loan or token.
5. attacker can create unfair available loans for user1's collateral and set user1 as receiver and cause greifing, user1 has to pay fee and interest rate and would have unfair unwanted loan.

This is a critical issue because attacker can perform this attack on every collateral provider and cause basic functionality of the protocol to be broken. if attacker create loan for operator users lose their NFT for nothing.

Tools Used

VIM

Recommended Mitigation Steps

allow loan creation when msg.sender is operator or holder or approved for all.

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L21

Vulnerability details

Impact

Function ERC4626RouterBase.mint() calls vault.mint() and vault's mint function calculates asset's amount and transfers underlying asset from caller but function ERC4626RouterBase.mint() uses shares amount when setting the approval for vault. This would cause ERC4626RouterBase.mint() to when assets amount is bigger than shares amount and this is common situation (assets per share increase because of the interest rate over time). users can't use AstariaRouter's mint() function which supports slippage and call to the mint() would revert most of the times.

Proof of Concept

This is mint() code in the ERC4626RouterBase contract:

  function mint(
    IERC4626 vault,
    address to,
    uint256 shares,
    uint256 maxAmountIn
  ) public payable virtual override returns (uint256 amountIn) {
    ERC20(vault.asset()).safeApprove(address(vault), shares);
    if ((amountIn = vault.mint(shares, to)) > maxAmountIn) {
      revert MaxAmountError();
    }
  }

As you can see in the line ERC20(vault.asset()).safeApprove(address(vault), shares); code uses share amount to set spending allowance for vault address in underlying asset. shares shows vault token amount and code should have used maxAmountIn for setting allowance. but in fact vault's mint() function transfers asset's amount after calculating it based on specified share amount. This is mint() function in ERC4626Cloned contract:

  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 transfers assets from caller after calculating it based on shares and the assets amount can be bigger than shares. because most of the time shares amount is lower than assets amount (assets per share increase over time because of the interest rate) so this issue would cause calls to mint() function to revert. any other protocol integrating with Astaria using this function would have broken logic and also users would lose gas if they use this function. contract AstariaRouter inherits ERC4626RouterBase and uses mint() function so users can't call AstariaRouter.mint() which supports slippage allowance and they have to call vault's mint() directly and they may lose funds because of the slippage.

Tools Used

VIM

Recommended Mitigation Steps

use maxAmountIn for allowance because it's the max amount user gives allowance and vault should always use less than that amount and if vault tries to use more amount then call would revert as it should.

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L27 https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L43

Vulnerability details

Impact

Functions mint() and deposit() used for depositing user assets and minting vault-token shares for user. the deposited amount should be higher than minDepositAmount() but function deposit() compares shares amount with minDepositAmount() while function mint() compare assets amount with minDepositAmount(). this is inconsistent and function deposit() compare the wrong amount which can cause some calls to revert and users can't use deposit() for small amounts while they should be.

Proof of Concept

This is deposit() and mint() code in ERC4626Cloned:

  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 mint() checks assets amount and deposit() checks share amount. while minDepositAmount() is based on assets amount:

  function minDepositAmount()
    public
    view
    virtual
    override(ERC4626Cloned)
    returns (uint256)
  {
    if (ERC20(asset()).decimals() == uint8(18)) {
      return 100 gwei;
    } else {
      return 10**(ERC20(asset()).decimals() - 1);
    }
  }

and function deposit() checks the wrong amount. so if a user wants to deposit some amounts of underlying token that is higher than minDepositAmount() and calls deposit() it's possible for his call to revert if the equivalent share amount was lower than minDepositAmount(). this would cause other 3rd party protocol to not be able to work with Astaria properly and users would lose gas from time to time.

Tools Used

VIM

Recommended Mitigation Steps

check assets amount

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/WithdrawProxy.sol#L163-L175 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/WithdrawProxy.sol#L152-L161

Vulnerability details

Impact

after WithdrawProxy deployment and before any auction start time the value of the finalAuctionEnd for that WithdrawProxy would be 0 and users would be able to call withdraw() function but they would lose their funds if they do it by mistake. onlyWhenNoActiveAuction check only ensure that if there where an auction then users won't be able to withdraw but if there were no auction or before the first auction the value of the finalAuctionEnd would be 0 and onlyWhenNoActiveAuction would bypass and anyone calling withdraw() or redeem() of WithdrawProxy would lose their funds because assets may not yet transferred to WithdrawProxy.

Proof of Concept

This is onlyWhenNoActiveAuction modifier code:

  modifier onlyWhenNoActiveAuction() {
    WPStorage storage s = _loadSlot();
    // If auction funds have been collected to the WithdrawProxy
    // but the PublicVault hasn't claimed its share, too much money will be sent to LPs
    if (s.finalAuctionEnd != 0) {
      // if finalAuctionEnd is 0, no auctions were added
      revert InvalidState(InvalidStates.NOT_CLAIMED);
    }
    _;
  }

As you can see it only checks that finalAuctionEnd != 0 and the value of the finalAuctionEnd set in the handleNewLiquidation() function. so if handleNewLiquidation() is not yet get called or there were no auction at all then modifier onlyWhenNoActiveAuction() would be bypassed. This is withdraw() code:

  function withdraw(
    uint256 assets,
    address receiver,
    address owner
  )
    public
    virtual
    override(ERC4626Cloned, IERC4626)
    onlyWhenNoActiveAuction
    returns (uint256 shares)
  {
    return super.withdraw(assets, receiver, owner);
  }

As you can see the only check is onlyWhenNoActiveAuction modifier and it can be bypassed in some cases (before the first liquidation and if there was no liquidation at all) and if users calls withdraw() or redeem() by mistake in those times they would lose their funds. this is a important issue because there is no proper logic in the code to specify valid time for users to call withdraw() or redeem() to receive their funds and there is no logic or function to call and see if everything is ready to withdraw funds or not. the only protection is onlyWhenNoActiveAuction modifier which is not proper and gives false sense of protection and it can be bypassed and user would lose funds. users would have to wait undefined time and if they call withdraw() by mistake they will lose funds.

Tools Used

VIM

Recommended Mitigation Steps

properly check that if contract is ready for withdrawals or not and only allow withdraws when everything is ready.

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L48 https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L62

Vulnerability details

Impact

Functions withdraw() and redeem() in ERC4626RouterBase are used to withdraw user funds from vaults and they call vault.withdraw() and vault.redeem() and logics in vault transfer user shares and user required to give spending allowance for vault and there is no need for ERC4626RouterBase to set approval for vault and because those approved tokens won't be used and code uses safeApprove() so next calls to withdraw() and redeem() would revert because code would tries to change allowance amount while it's not zero. those functions would revert always and AstariaRouter uses them and user won't be able to use those function and any other protocol integrating with Astaria calling those function would have broken logic. also if UI interact with protocol with router functions then UI would have broken parts too. and functions in router support users to set slippage allowance and without them users have to interact with vault directly and they may lose funds because of the slippage.

Proof of Concept

This is withdraw() and redeem() code in ERC4626RouterBase:

  function withdraw(
    IERC4626 vault,
    address to,
    uint256 amount,
    uint256 maxSharesOut
  ) public payable virtual override returns (uint256 sharesOut) {

    ERC20(address(vault)).safeApprove(address(vault), amount);
    if ((sharesOut = vault.withdraw(amount, to, msg.sender)) > maxSharesOut) {
      revert MaxSharesError();
    }
  }

  function redeem(
    IERC4626 vault,
    address to,
    uint256 shares,
    uint256 minAmountOut
  ) public payable virtual override returns (uint256 amountOut) {

    ERC20(address(vault)).safeApprove(address(vault), shares);
    if ((amountOut = vault.redeem(shares, to, msg.sender)) < minAmountOut) {
      revert MinAmountError();
    }
  }

As you can see the code sets approval for vault to spend routers vault tokens and then call vault function. this is _redeemFutureEpoch() code in the vault which handles withdraw and redeem:

  function _redeemFutureEpoch(
    VaultData storage s,
    uint256 shares,
    address receiver,
    address owner,
    uint64 epoch
  ) internal virtual returns (uint256 assets) {
    // check to ensure that the requested epoch is not in the past

    ERC20Data storage es = _loadERC20Slot();

    if (msg.sender != owner) {
      uint256 allowed = es.allowance[owner][msg.sender]; // Saves gas for limited approvals.

      if (allowed != type(uint256).max) {
        es.allowance[owner][msg.sender] = allowed - shares;
      }
    }

    if (epoch < s.currentEpoch) {
      revert InvalidState(InvalidStates.EPOCH_TOO_LOW);
    }
    require((assets = previewRedeem(shares)) != 0, "ZERO_ASSETS");
    // check for rounding error since we round down in previewRedeem.

    //this will underflow if not enough balance
    es.balanceOf[owner] -= shares;

    // Cannot overflow because the sum of all user
    // balances can't exceed the max uint256 value.
    unchecked {
      es.balanceOf[address(this)] += shares;
    }

    emit Transfer(owner, address(this), shares);
    // Deploy WithdrawProxy if no WithdrawProxy exists for the specified epoch
    _deployWithdrawProxyIfNotDeployed(s, epoch);

    emit Withdraw(msg.sender, receiver, owner, assets, shares);

    // WithdrawProxy shares are minted 1:1 with PublicVault shares
    WithdrawProxy(s.epochData[epoch].withdrawProxy).mint(shares, receiver);
  }

As you can see this code only check spending allowance that real owner of shares gives to the msg.sender and there is no check or updating spending allowance of the router vaulttokens for vault. so those approvals in the withdraw() and redeem() are unnecessary and they would cause code to revert always because code tries to set approval with safeApprove() while the current allowance is not zero. this issue would cause calls to withdraw() and redeem() function to revert. any other protocol integrating with Astaria using those functions would have broken logic and also users would lose gas if they use those functions. contract AstariaRouter inherits ERC4626RouterBase and uses its withdraw() and redeem() function so users can't call AstariaRouter.withdraw() or AstariaRouter.redeem()which supports slippage allowance and they have to call vault's functions directly and they may lose funds because of the slippage.

Tools Used

VIM

Recommended Mitigation Steps

remove unnecessary code

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.