Astaria contest - obront's results

Lines of code

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

Vulnerability details

Impact

If the first depositor to a vault calls mint(), previewMint() automatically returns 10e18 as the amount of assets they'll need to deposit, regardless of the number of shares.

The check for minimum deposit amount which is intended to stop this first depositor attack accidentally checks assets rather than shares:

require(assets > minDepositAmount(), "VALUE_TOO_SMALL");

This is ineffective, as we can still perform the attack by having a small number of shares and a large number of assets.

Proof of Concept

• first user mints 1 token
• assets = 10e18, so the minDepositAmount() check passes
• second user deposits 19 WETH and also receives 1 token
• the first user withdraws their 1 token for 14.5 WETH, earning a 4.5 WETH profit at the second user's expense

Because this doesn't require the first user to make an additional transfer straight to the vault, this setup leads to a situation where this attack could be performed accidentally by an innocent first depositor.

Tools Used

Manual Review

Recommended Mitigation Steps

Change the minDepositAmount() check to check shares rather than assets:

require(shares > minDepositAmount(), "VALUE_TOO_SMALL");

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L220-L231

Vulnerability details

Impact

When an auction is ended successfully, the Collateral#settleAuction() function is called, which zeros out many of the important values to signify that the auction (and liens) are no longer active.

The flow for a liquidator claiming an NFT after a failed auction is missing many of these same updates, which can cause downstream problems.

Proof of Concept

This flow is kicked off by any user calling liquidatorNFTClaim(). Assuming there is in fact a failed auction, this calls ClearingHouse#settleLiquidatorNFTClaim(), which pays off the debts with a total amount of 0.

However, since settleAuction() is never called, the following updates are skipped:

• s.idToUnderlying[collateralId] isn't deleted
• s.collateralIdToAuction[collateralId] isn't deleted
• the collateral ID token isn't burned

This can cause many downstream problems. One significant example is that, if the same collateral is ever used in the future, the transfer will revert because we will not be able to _mint() the same collateral ID, since it will still exist.

Tools Used

Manual Review

Recommended Mitigation Steps

At the end of ClearingHouse#settleLiquidatorNFTClaim(), we should call Collateral#settleAuction().

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L122-L231 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L522-L525

Vulnerability details

Impact

When a lien is bought out, part of the logic included in the _buyoutLien() function is to reduce the slope of the vault that is being bought out:

if (_isPublicVault(s, payee)) {
  IPublicVault(payee).handleBuyoutLien(
    IPublicVault.BuyoutLienParams({
      lienSlope: calculateSlope(params.encumber.stack[params.position]),
      lienEnd: params.encumber.stack[params.position].point.end,
      increaseYIntercept: buyout -
        params.encumber.stack[params.position].point.amount
    })
  );
}

However, there is no corresponding logic to increase the slope of the receiving lien. The result is that the receiving vault of any buyout will have its slope underrepresented.

This issue goes from Medium to High because all the slope adjustments are unchecked. In each of these cases, if we take an action that reduces the slope (making a payment, having the lien bought out from us, or getting liquidated), the slope underflows and becomes close to type(uint48).max.

The totalAssets() of the vault (which are used to determine the share value) are calculated using this slope:

return uint256(s.slope).mulDivDown(delta_t, 1) + uint256(s.yIntercept);

The result is that a user can maliciously invest in a vault with a small slope, buy out a lien, force the slope to underflow, and cash out their tokens at a massively inflated price, draining the vault.

Proof of Concept

• For simplicity, let's say a public vault exists with a large amount of assets but no slope (ie no active liens)
• A malicious user calls VaultImplementation#buyoutLien() on another vault to buy one of their liens on behalf of our target vault. As long as the buyout fits the target vault's approved strategies and improves the terms, this is accepted.
• This leaves our target vault with a slope of 0 but an open lien.
• The user then makes a payment towards the lien: lienToken#makePayment() => _makePayment() => _payment() => PublicVault#beforePayment(), which, in an unchecked block, sets the slope to s.slope - params.lienSlope.safeCastTo48().
• Since s.slope == 0, this underflows, and the slope is now very large, close to type(uint48).max.
• The malicious user then redeems their shares in the vault. This redemption is calculated (using the previewRedeem() function) as shares.mulDivDown(totalAssets(), supply), where totalAssets() = uint256(s.slope).mulDivDown(delta_t, 1) + uint256(s.yIntercept);.
• The artificially high slope allows the malicious user to drain the vault of all its funds.

Tools Used

Manual Review

Recommended Mitigation Steps

When handleBuyoutLien() is called in the _buyoutLien() function, add an equivalent function that increases the slope and y-intercept of the acquiring vault, to ensure that slope always accurately represents the active liens for all vaults.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L471-L489 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L153-L174

Vulnerability details

Impact

When a new lien is taken (or bought out), one of the validations is to ensure that the potentialDebt of each borrower on the stack is less than or equal to their liquidationInitialAsk.

if (potentialDebt > newStack[j].lien.details.liquidationInitialAsk) {
    revert InvalidState(InvalidStates.INITIAL_ASK_EXCEEDED);
  }

In _appendStack() and _buyoutLien(), this is performed by iterating through the stack backwards, totaling up the potentialDebt, and comparing it to each lien's liquidationInitialAsk:

for (uint256 i = stack.length; i > 0; ) {
      uint256 j = i - 1;
      newStack[j] = stack[j];
      if (block.timestamp >= newStack[j].point.end) {
        revert InvalidState(InvalidStates.EXPIRED_LIEN);
      }
      unchecked {
        potentialDebt += _getOwed(newStack[j], newStack[j].point.end);
      }
      if (potentialDebt > newStack[j].lien.details.liquidationInitialAsk) {
        revert InvalidState(InvalidStates.INITIAL_ASK_EXCEEDED);
      }

      unchecked {
        --i;
      }
    }

However, only the first item on the stack has a liquidationInitialAsk that matters. When a new auction is started on Seaport, Router#liquidate() uses stack[0].lien.details.liquidationInitialAsk as the starting price. The other values are meaningless, except in their ability to DOS future borrowers.

Proof of Concept

• I set my liquidationInitialAsk to be exactly the value of my loan
• A borrower has already borrowed on their collateral, and the first loan on the stack will determine the auction price
• When they borrow from me, my liquidationInitialAsk is recorded
• Any future borrows will check that futureBorrow + myBorrow <= myLiquidationInitialAsk, which is not possible for any futureBorrow > 0
• The result is that the borrower will be DOS'd from all future borrows

This is made worse by the fact that liquidationInitialAsk is not a variable that can justify a refinance, so they'll need to either pay back the loan or find a refinancier who will beat one of the other terms (rate or duration) in order to get rid of this burden.

Tools Used

Manual Review

Recommended Mitigation Steps

Get rid of all checks on liquidationInitialAsk except for comparing the total potential debt of the entire stack to the liquidationInitialAsk of the lien at position 0.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L114-L167 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L524-L545 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L497-L510 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L623-L656

Vulnerability details

Impact

The Clearing House is implemented as an ERC1155. This is used to settle up at the end of an auction. The Clearing House's token is listed as one of the Consideration Items, and when Seaport goes to transfer it, it triggers the settlement process.

This settlement process includes deleting the collateral state hash from LienToken.sol, burning all lien tokens, deleting the idToUnderlying mapping, and burning the collateral token. These changes effectively wipe out all record of the liens, as well as removing any claim the borrower has on their underlying collateral.

After an auction, this works as intended. The function verifies that sufficient payment has been made to meet the auction criteria, and therefore all these variables should be zeroed out.

However, the issue is that there is no check that this safeTransferFrom function is being called after an auction has completed. In the case that it is called when there is no auction, all the auction criteria will be set to 0, and therefore the above deletions can be performed with a payment of 0.

This allows any user to call the safeTransferFrom() function for any other user's collateral. This will wipe out all the liens on that collateral, and burn the borrower's collateral token, and with it their ability to ever reclaim their collateral.

Proof of Concept

The flow is as follows:

• safeTransferFrom(offerer, buyer, paymentToken, amount, data)
• _execute(offerer, buyer, paymentToken, amount)
• using the auctionStack in storage, it calculates the amount the auction would currently be listed at
• it confirms that the Clearing House has already received sufficient paymentTokens for this amount
• it then transfers the liquidator their payment (currently 13%)
• it calls LienToken#payDebtViaClearingHouse(), which pays back all liens, zeros out all lien storage and deletes the collateralStateHash
• if there is any remaining balance of paymentToken, it transfers it to the owner of the collateral
• it then calls Collateral#settleAuction(), which deletes idToUnderlying, collateralIdToAuction and burns the collateral token

In the case where the auction hasn't started, the auctionStack in storage is all set to zero. When it calculates the payment that should be made, it uses _locateCurrentAmount, which simply returns endAmount if startAmount == endAmount. In the case where they are all 0, this returns 0.

The second check that should catch this occurs in settleAuction():

    if (
      s.collateralIdToAuction[collateralId] == bytes32(0) &&
      ERC721(s.idToUnderlying[collateralId].tokenContract).ownerOf(
        s.idToUnderlying[collateralId].tokenId
      ) !=
      s.clearingHouse[collateralId]
    ) {
      revert InvalidCollateralState(InvalidCollateralStates.NO_AUCTION);
    }

However, this check accidentally uses an && operator instead of a ||. The result is that, even if the auction hasn't started, only the first criteria is false. The second is checking whether the Clearing House owns the underlying collateral, which happens as soon as the collateral is deposited in CollateralToken.sol#onERC721Received():

      ERC721(msg.sender).safeTransferFrom(
        address(this),
        s.clearingHouse[collateralId],
        tokenId_
      );

Tools Used

Manual Review

Recommended Mitigation Steps

Change the check in settleAuction() from an AND to an OR, which will block any collateralId that isn't currently at auction from being settled:

    if (
      s.collateralIdToAuction[collateralId] == bytes32(0) ||
      ERC721(s.idToUnderlying[collateralId].tokenContract).ownerOf(
        s.idToUnderlying[collateralId].tokenId
      ) !=
      s.clearingHouse[collateralId]
    ) {
      revert InvalidCollateralState(InvalidCollateralStates.NO_AUCTION);
    }

Lines of code

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

Vulnerability details

Impact

When claim() is called on the WithdrawProxy, the goal is to allow the vault to claim the share of the balance that is not a part of the withdrawRatio.

withdrawRatio is calculated as a share of 1e18, so the ideal math is as follows:

transferAmount = balance * withdrawRatio / 1e18;
sendToVault = balance * (1 - (withdrawRatio / 1e18));

Instead, the function uses 10**ERC20(asset()).decimals() in place of 1e18. This is equivalent when working with a token with 18 decimal places, but for tokens with different numbers of decimals, this can dramatically throw off the expected math.

Proof of Concept

Let's say withdrawRatio == 1e17 (ie 10%) and we are using a token with 8 decimal places.

transferAmount is calculated as: transferAmount = uint256(s.withdrawRatio).mulDivDown( balance, 10**ERC20(asset()).decimals() );

This simplifies to 1e17 * balance / 1e8 or 1e9 * balance.

This is a major problem, because transferAmount is supposed to be a share of balance, and should never be greater than balance. We can see this in the next line, when an unchecked block is used to subtract the two:

unchecked {
    balance -= transferAmount;
}

The result is that, if we use a token with fewer decimals than 18, balance will underflow.

Tools Used

Manual Review

Recommended Mitigation Steps

Replace 10**ERC20(asset()).decimals() with 1e18 in the transferAmount calculation.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L233-L244

Vulnerability details

Impact

When new commitments are validated, permissions are checked with the following:

    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);
    }

This check allows any commitment to pass as long the receiver is the owner of the collateral.

This allows a malicious vault owner to create any lien terms they'd like (200% interest rate, etc) and commit to the lien on behalf of a user with existing collateral. As long as they are honestly sending the loan to the collateral owner, they are able to choose any terms they'd like, allowing them to steal interest payments from unsuspecting users.

Proof of Concept

• I'm the owner of a vault that holds WETH
• I encode a strategy to lend against Bored Apes at a safe value at 200% interest
• I watch for any users putting Bored Apes up for collateral
• I then commit to liens on their behalf, sending them WETH collateralized by their apes
• These commitments pass because receiver == holder
• I can send them as many liens as possible, up to the maximum of 5
• They are forced to pay any interest that accrues between when I do this and when they notice

Tools Used

Manual Review

Recommended Mitigation Steps

Do not allow the receiver to play any role in whether the commitment is valid. It should be required that msg.sender is either the holder, the operator, or is approved for all.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L303-L305

Vulnerability details

Impact

As pointed out in the Spearbit audit:

If the processEpoch() endpoint does not get called regularly (especially close to the epoch boundaries), the updated currentEpoch would lag behind the actual expected value and this will introduce arithmetic errors in formulas regarding epochs and timestamps.

This can cause a problem because processEpoch() cannot be called when there are open liens, and liens may remain open in the event that a lien expires and isn't promptly liquidated.

Proof of Concept

processEpoch() contains the following check to ensure that all liens are closed before the epoch is processed:

if (s.epochData[s.currentEpoch].liensOpenForEpoch > 0) {
  revert InvalidState(InvalidStates.LIENS_OPEN_FOR_EPOCH_NOT_ZERO);
}

The accounting considers a lien open (via s.epochData[s.currentEpoch].liensOpenForEpoch) unless this value is decremented, which happens in three cases: when (a) the full payment is made, (b) the lien is bought out, or (c) the lien is liquidated.

In the event that a lien expires and nobody calls liquidate() (for example, if the NFT seems worthless and no other user wants to pay the gas to execute the function for the fee), this would cause processEpoch() to fail, and could create delays in the epoch processing and cause the accounting issues pointed out in the previous audit.

Tools Used

Manual Review

Recommended Mitigation Steps

Astaria should implement a monitoring solution to ensure that liquidate() is always called promptly for expired liens, and that processEpoch() is always called promptly when the epoch ends.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L96-L108

Vulnerability details

Impact

The minDepositAmount() function is intended to ensure that all depositors into Public Vaults are depositing more than dust to protect against attacks like the 4626 front running attack. It is calculated as follows:

  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);
    }
  }

For assets with 18 decimals, this calculation is totally reasonable, as the minimum deposit is just 100 gwei (1/10k of a USD). However, for other assets, the formula returns 0.1 tokens, regardless of value.

While this may be fine for low-priced tokens, it leads to a minimum deposit for tokens like WBTC to be over $2000 USD, certainly too high for many user and not aligned with the intended behavior (which can be seen in the 18 decimal base case).

Proof of Concept

• WBTC is 8 decimals and has a value of ~$20k USD
• the minimum deposit is calculated as return 10**(ERC20(asset()).decimals() - 1);
• for WBTC, this would return 10 ** 7
• 20,000 * (10 ** 7) / (10 ** 8) = 2000 USD

This is far out of line with the 1/10,000 USD expected for tokens with 18 decimals.

Tools Used

Manual Review

Recommended Mitigation Steps

Since our requirements for the size of deposit are so low, we can customize a formula that ensures we get a small final result in all cases. For example:

if (ERC20(asset()).decimals() < 4) {
    return 10**(ERC20(asset()).decimals() - 1);
else if (ERC20(asset()).decimals() < 8) {
    return 10**(ERC20(asset()).decimals() - 2);
} else {
    return 10**(ERC20(asset()).decimals() - 6);
}

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L321-L337

Vulnerability details

Impact

In PublicVault.sol#processEpoch(), we update the withdraw reserves based on how totalAssets() (the real amount of the underlying asset held by the vault) compares to the expected value in the withdraw proxy:

  unchecked {
    if (totalAssets() > expected) {
      s.withdrawReserve = (totalAssets() - expected)
        .mulWadDown(s.liquidationWithdrawRatio)
        .safeCastTo88();
    } else {
      s.withdrawReserve = 0;
    }
  }

In the event that the totalAssets() is greater than expected, we take the surplus in assets multiply it by the withdraw ratio, and assign this value to s.withdrawReserve.

However, because this logic is wrapped in an unchecked block, it must have confidence that this calculation does not overflow. Because the protocol allows arbitrary ERC20s to be used, it can't have confidence in the size of totalAssets(), which opens up the possibility for an overflow in this function.

Proof of Concept

mulWadDown is calculated by first multiplying the two values, and then dividing by 1e18. This is intended to prevent rounding errors with the division, but also means that an overflow is possible when the two values have been multiplied, before any division has taken place.

This unchecked block is safe from overflows if:

• (totalAssets() - expected) * s.liquidationWithdrawRatio < 1e88 (because s.withdrawRatio is 88 bytes)
• liquidationWithdrawRatio is represented as a ratio of WAD, so it will be in the 1e17 - 1e18 range, let's assume 1e17 to be safe
• therefore we require totalAssets() - expected < 1e61

Although this is unlikely with most tokens (and certainly would have been safe in the previous iteration of the protocol that only used WETH, when allowing arbitrary ERC20 tokens, this is a risk.

Tools Used

Manual Review

Recommended Mitigation Steps

Remove the unchecked block around this calculation, or add an explicitly clause to handle the situation where totalAssets() gets too large for the current logic.

Lines of code

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

Vulnerability details

Impact

The WithdrawProxy contract has the onlyWhenNoActiveAuction modifier on the withdraw() and redeem() functions. This modifier stops these functions from being called when an auction is active:

modifier onlyWhenNoActiveAuction() {
  WPStorage storage s = _loadSlot();
  if (s.finalAuctionEnd != 0) {
    revert InvalidState(InvalidStates.NOT_CLAIMED);
  }
  _;
}

Furthermore, both withdraw() and redeem() can only be called when totalAssets() > 0 based on logic within those functions.

The intention of these two checks is that the WithdrawProxy shares can only be cashed out after the PublicVault has called transferWithdrawReserve.

However, because s.finalAuctionEnd == 0 before an auction has started, and totalAssets() is calculated by taking the balance of the contract directly (ERC20(asset()).balanceOf(address(this));), a user may redeem their shares before the vault has been fully funded, and take less than their share of the balance, benefiting the other withdrawer.

Proof of Concept

• A depositor decides to withdraw from the PublicVault and receives WithdrawProxy shares in return
• A malicious actor deposits a small amount of the underlying asset into the WithdrawProxy, making totalAssets() > 0
• The depositor accidentally redeems, or is tricked into redeeming, from the WithdrawProxy, getting only a share of the small amount of the underlying asset rather than their share of the full withdrawal
• PublicVault properly processes epoch and full withdrawReserve is sent to WithdrawProxy
• All remaining holders of WithdrawProxy shares receive an outsized share of the withdrawReserve

Tools Used

Manual Review

Recommended Mitigation Steps

Add an additional storage variable that is explicitly switched to open when it is safe to withdraw funds.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L646

Vulnerability details

Impact

In _paymentAH(), we attempt to delete the stack position after making the payment:

delete stack[position];

However, stack is an argument provided to the function in memory, not in storage. As a result, the deletion will not do anything and the stack will remain in storage.

Proof of Concept

• There are 5 elements on the stack in storage
• The array is passed in memory to the function
• Each one is deleted using delete stack[position]
• The result is that all 5 elements are still in storage

Tools Used

Manual Review

Recommended Mitigation Steps

Pass the stack argument to _paymentAH() as a storage variable.

function _paymentAH(
    LienStorage storage s,
    address token,
    AuctionStack[] storage stack,
    uint256 position,
    uint256 payment,
    address payer
  ) internal returns (uint256) {
  ...

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L780-L785 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#L233-L244

Vulnerability details

Impact

When a user calls Router#commitToLiens(), the Router calls commitToLien(). The comments specify:

//router must be approved for the collateral to take a loan,

However, the Router being approved isn't enough. It must be approved for all, which is a level of approvals that many users are not comfortable with. This is because, when the commitment is validated, it is checked as follows:

    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);
    }

Proof of Concept

The check above allows the following situations pass:

• caller of the function == owner of the NFT
• receiver of the loan == owner of the NFT
• receiver of the loan == address approved for the individual NFT
• caller of the function == address approved for all

This is inconsistent and doesn't make much sense. The approved users should have the same permissions.

More importantly, the most common flow (that the address approved for the individual NFT — the Router — is the caller) does not work and will lead to the function reverting.

Tools Used

Manual Review

Recommended Mitigation Steps

Change the check to include msg.sender != operator rather than receiver != operator.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L611-L619

Vulnerability details

Impact

The canLiquidate() function allows liquidations to take place if either (a) the loan is over and still exists or (b) the caller owns the collateral.

In the second case, due to the liquidation fee (currently 13%), this can give a borrower an unfair position to be able to reclaim a percentage of the liquidation that should be going to their lenders.

Proof of Concept

• A borrower puts up a piece of collateral and takes a loan of 10 WETH
• The collateral depreciates in value and they decide to keep the 10 WETH
• Right before the loans expire, the borrower can call liquidate() themselves
• This sets them as the liquidator and gives them the first 13% return on the auction
• While the lenders are left at a loss, the borrower gets to keep the 10 WETH and get a 1.3 WETH bonus

Tools Used

Manual Review

Recommended Mitigation Steps

Don't allow users to liquidate their own loans until they are liquidatable by the public.

Lines of code

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

Vulnerability details

Impact

The entire Astaria system is based on calculating a y-intercept and slope for the vaults, and updating that based on the actions being taken. It does not often recalibrate with the actual underlying token balance.

Since the system allows arbitrary ERC20s to be used, it is likely that vaults will be created with fee on transfer tokens.

In this case, there will be a number of issues:

• y-int and slope will be inflated relative to reality, causing new depositors to pay a higher price than is fair
• borrows will be for less than expected
• paybacks will not leave lenders with the amount of the asset they expect

Proof of Concept

As an example of how this is done at the end of each epoch, the processEpoch() function is called, which updates the Y intercept as follows:

      _setYIntercept(
        s,
        s.yIntercept -
          totalAssets().mulDivDown(s.liquidationWithdrawRatio, 1e18)
      );

Total Assets is calculated as the current y-int plus the slope * time. So what this is doing is taking the y-int and adjusting it upwards based on the slope that should have been realized, net of withdrawals. This doesn't have a check with the real underlying balance.

The result is that this discrepancy can grow very substantial over time, until it is prohibitive for new users to join the vault.

Tools Used

Manual Review

Recommended Mitigation Steps

I'd recommend creating a whitelist for permitted ERC20s. Otherwise, explicitly disallow fee on transfer tokens from being used.