Astaria contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

exclusive access to vault,Other users will not be able to mint

Proof of Concept

In the previewMint method, if the first user mint, no matter how many shares the user passes in, only 10e18 assets are needed

  function previewMint(uint256 shares) public view virtual returns (uint256) {
    uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.
    //****@audit when supply ==0, ignore shares, Direct return 10e18 ***//
    return supply == 0 ? 10e18 : shares.mulDivUp(totalAssets(), supply);
  }

So that users can mint(shares=type(uint256).max), only need 10e18 asset

Other users will not be able to mint, because totalSupply will overflow

Tools Used

Recommended Mitigation Steps

asset 1:1 return

Lines of code

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

Vulnerability details

Impact

vault loss assets

Proof of Concept

When the auction is successful the NFT is transferred to the bidder and seaport calls ClearingHouse.safeTransferFrom() to trigger the repayment of the debt through the conduit mechanism

ClearingHouse.safeTransferFrom() -> ClearingHouse._execute() In _execute(), it does not check if msg.sender is from seaport, it only cares about the amount of payment received > currentOfferPrice This is fine. It doesn't matter who calls it, as long as the amount is right

But here is a problem, only limit the number of amounts, but did not check whether the token address is legal, the address is a parameter, can be passed arbitrarily

  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 {

    address paymentToken = bytes32(encodedMetaData).fromLast20Bytes();   <------From the parameter encodedMetaData, which can be forged arbitrarily

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

And this paymentToken address, in the subsequent logic also did not verify whether this token address is the payment token specified when creating Liens

  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 {
...
    ASTARIA_ROUTER.LIEN_TOKEN().payDebtViaClearingHouse(
      paymentToken,    <----Inside the payDebtViaClearingHouse() there is still no verification that the paymentToken address is legitimate
      collateralId,
      payment - liquidatorPayment,
      s.auctionStack.stack
    );  


    ASTARIA_ROUTER.COLLATERAL_TOKEN().settleAuction(collateralId);   <---- settleAuction() can pass ,it ok

To sum up:A malicious user can pass a worthless forged payment token to repay the vault, resulting in vault loss assets

Tools Used

Recommended Mitigation Steps

1._execute() add check payment token address

  //ClearingHouse.sol
  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 {
...
    address paymentToken = bytes32(encodedMetaData).fromLast20Bytes();
+   require(paymentToken!=address(0) && paymentToken == s.auctionData.paymentToken) ;

...

  }

  //LienToken.sol
  function _stopLiens(
...
+   auctionData.paymentToken = stack[0].lien.token;
    s.COLLATERAL_TOKEN.getClearingHouse(collateralId).setAuctionData(
      auctionData
    );
  }  

2.payDebtViaClearingHouse() also verifies the legitimacy of the payment token, currently createLien does not save the payment address, it is recommended to save it in s.lienMeta[id]

  //LienToken.sol
  function _createLien(
...
    newLienId = uint256(keccak256(abi.encode(params.lien)));  
+   s.lienMeta[newLienId].paymentToken = params.lien.token;



...
   }
  //LienToken.sol
  function _paymentAH(
    LienStorage storage s,
    address token,
    AuctionStack[] memory stack,
    uint256 position,
    uint256 payment,
    address payer
  ) internal returns (uint256) {
...
    uint256 lienId = stack[position].lienId;
+   require(s.lienMeta[lienId].paymentToken == token,"invald payment token");

Lines of code

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/AstariaRouter.sol#L485-L486

Vulnerability details

Impact

Stealing vault assets

Proof of Concept

There are currently two ways to create new Liens in the system

1. user call AstariaRouter.commitToLiens(), the internal implementation of this is through VaultImplementation.commitToLien() to create
2. user can call VaultImplementation.commitToLien() directly, because commitToLien is external(), as long as the owner of the NFT can call

Currently, the direct call to VaultImplementation.commitToLien() has a payment token legitimacy check vulnerability

Calling process: VaultImplementation.commitToLien() -> VaultImplementation._requestLienAndIssuePayout() -> AstariaRouter.requestLienPosition() -> AstariaRouter._validateCommitment() Set the payment token in _validateCommitment() The code is as follows:

  function _validateCommitment(
.....

      lien = ILienToken.Lien({
      collateralType: nlrType,
      details: details,
      strategyRoot: commitment.lienRequest.merkle.root,
      collateralId: commitment.tokenContract.computeId(commitment.tokenId),
      vault: commitment.lienRequest.strategy.vault,     <----Note that this vault parameter is passed in and can be specified arbitrarily, and can be specified as a malicious contract
      token: IAstariaVaultBase(commitment.lienRequest.strategy.vault).asset() <-----Specify vault as a malicious contract that can return a malicious payment token like:mock_token
    });

There is no other place in the system to verify that this token is legitimate

But the malicious user receives the real asset token of the vault

VaultImplementation.commitToLien() -> VaultImplementation._requestLienAndIssuePayout()

  function _requestLienAndIssuePayout(
...
    _validateCommitment(c, receiver);
    (newLienId, stack, slope) = ROUTER().requestLienPosition(c, recipient());
    payout = _handleProtocolFee(c.lienRequest.amount);
    ERC20(asset()).safeTransfer(receiver, payout);  <------use get real asset token
  }

LienToken.makePayment() is also repaid with the malicious payment token

In summary, a malicious user can borrow vault's asset token, but the malicious invalid payment token is repaid

Tools Used

Recommended Mitigation Steps

AstariaRouter.requestLienPosition() add check msg.sender == commitment.lienRequest.strategy.vault to avoid malicious vault parameters

  function requestLienPosition(
    IAstariaRouter.Commitment calldata params,
    address receiver
  )
    external
    whenNotPaused
    validVault(msg.sender)
    returns (
      uint256,
      ILienToken.Stack[] memory,
      uint256
    )
  {
+   require(msg.sender == params.lienRequest.strategy.vault,"invalid strategy.vault");

    RouterStorage storage s = _loadRouterSlot();
    return
      s.LIEN_TOKEN.createLien(
        ILienToken.LienActionEncumber({
          lien: _validateCommitment({
            s: s,
            commitment: params,
            timeToSecondEpochEnd: IPublicVault(msg.sender).supportsInterface(
              type(IPublicVault).interfaceId
            )
              ? IPublicVault(msg.sender).timeToSecondEpochEnd()
              : 0
          }),
          amount: params.lienRequest.amount,
          stack: params.lienRequest.stack,
          receiver: receiver
        })
      );
  }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

The VaultImplementation.commitToLien() method is external and can be executed by anyone The method will internally verify that the corresponding collateralId is yours or has the corresponding authorization The validation code is as follows:

  function _validateCommitment(
    if (
      msg.sender ! = holder &&
      receiver ! = holder && 
      receiver ! = operator &&
      !CT.isApprovedForAll(holder, msg.sender)
    ) {
      revert InvalidRequest(InvalidRequestReason.NO_AUTHORITY);
    }

Note that this receiver comes from the parameter, so anyone can pass receiver==holder to skip this authentication, i.e. anyone can createLien for the owner of the collateralId

There are many possible problems that can be caused: For example:

1. if the collateralId has not been borrowed (only transfer NFT to CollateralToken), then the malicious user can generate a private vault, vault.asset as a worthless token, then set a very short borrowing period policy, and then through the above commitToLien() to borrow money for others Since the period is very short, it will soon expire and enter the auction. Use worthless assets to bid to steal NFT

2. collateralId already has Lien, then the user can generate a very high interest rate strategy, and then generate Lien for someone else's borrowing through the above commitToLien(), thus earning interest

3. collateralId borrowed without knowing it

Suggestion:

Only verify msg.sender, do not verify receiver

Tools Used

Recommended Mitigation Steps

  function _validateCommitment(
    IAstariaRouter.Commitment calldata params,
    address receiver
  ) internal view {
...
    if (
      msg.sender != holder &&
-     receiver != holder && 
-     receiver != operator &&
+     msg.sender != operator && 
      !CT.isApprovedForAll(holder, msg.sender)
    ) {
      revert InvalidRequest(InvalidRequestReason.NO_AUTHORITY);
    }  

Lines of code

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

Vulnerability details

Impact

Illegal liquidationInitialAsk, resulting in insufficient bids to cover the debt

Proof of Concept

_buyoutLien() will validate against liquidationInitialAsk, but incorrectly uses the old stack for validation

  function _buyoutLien(
    LienStorage storage s,
    ILienToken.LienActionBuyout calldata params
  ) internal returns (Stack[] memory newStack, Stack memory newLien) {

....

    uint256 potentialDebt = 0;
    for (uint256 i = params.encumber.stack.length; i > 0; ) {
      uint256 j = i - 1;
      // should not be able to purchase lien if any lien in the stack is expired (and will be liquidated)
      if (block.timestamp >= params.encumber.stack[j].point.end) {
        revert InvalidState(InvalidStates.EXPIRED_LIEN);
      }

      potentialDebt += _getOwed(
        params.encumber.stack[j],
        params.encumber.stack[j].point.end
      );

      if (
        potentialDebt >
        params.encumber.stack[j].lien.details.liquidationInitialAsk   //1.****@audit use old stack
      ) {
        revert InvalidState(InvalidStates.INITIAL_ASK_EXCEEDED);
      }

      unchecked {
        --i;
      }
    }  
....
    newStack = _replaceStackAtPositionWithNewLien(
      s,
      params.encumber.stack,
      params.position,
      newLien,
      params.encumber.stack[params.position].point.lienId    //2.****@audit replace newStack
    );    

Tools Used

Recommended Mitigation Steps

Replace then verify, using the newStack[] for verification

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L526-L534

Vulnerability details

Impact

ClearingHouse.safeTransferFrom() to execute successfully even if there is no bid

Proof of Concept

settleAuction is called at the end of the auction and will check if the status is legal

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

This check seems to be miswritten，The normal logic would be

s.collateralIdToAuction[collateralId] == bytes32(0) || ERC721(s.idToUnderlying[collateralId].tokenContract).ownerOf(
        s.idToUnderlying[collateralId].tokenId
      ) == s.clearingHouse[collateralId]

This causes ClearingHouse.safeTransferFrom() to execute successfully even if there is no bid

Tools Used

Recommended Mitigation Steps

  function settleAuction(uint256 collateralId) public {
    if (
-     s.collateralIdToAuction[collateralId] == bytes32(0) &&
-    ERC721(s.idToUnderlying[collateralId].tokenContract).ownerOf(
-        s.idToUnderlying[collateralId].tokenId
-      ) !=
-      s.clearingHouse[collateralId]
+      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/LienToken.sol#L366-L368

Vulnerability details

Impact

Corrupt multiple key properties of public vault, causing vault not to function properly

Proof of Concept

When LienToken.makePayment()/buyoutLien()/payDebtViaClearingHouse() If it corresponds to PublicVault, it will make multiple changes to the vault, such as: yIntercept, slope, last, epochData, etc.

If LienToken corresponds to PublicVault, then ownerOf(lienId) = PublicVault address

When the LienToken is a private vault, it is possible to transfer the owner of the LienToken.

As the above seems, if the private vault is transferred to the PublicVault address will result in the wrong modification of the yIntercept, slope, last, epochData, etc.

So we restrict the to in transferFrom to not be a PublicVault address

  function transferFrom(
    address from,
    address to,
    uint256 id
  ) public override(ERC721, IERC721) {

    if (_isPublicVault(s, to)) {  //***@audit when to == PublicVault address , will revert
      revert InvalidState(InvalidStates.PUBLIC_VAULT_RECIPIENT);  
    }  

However, such a restriction does not prevent an attacker from transferring PrivateVault's LienToken to PublicVault Because the address is predictable when the vault contract is created, a malicious user can predict the vault address, front-run, and transfer PrivateVault's LienToken to the predicted PublicVault address before the public vault is created, thus bypassing this restriction

Assumptions:

1. alice creates PrivateVault, and creates multiple PrivateVault's LienToken
2. alice monitors bob's creation of the PublicVault transaction, i.e., AstariaRouter.newPublicVault(), and then predicts the address of the newly generated treasure chest Note: newPublicVAult() although the use of create(), but still can predict the address

see：https://ethereum.stackexchange.com/questions/760/how-is-the-address-of-an-ethereum-contract-computed

The address for an Ethereum contract is deterministically computed from the address of its creator (sender) and how many transactions the creator has sent (nonce). The sender and nonce are RLP encoded and then hashed with Keccak-256.

3.front-run , and transfer LienToken to public vault predict address 4.bob's public vault created success and do some loan 5.alice do makePayment() to Corrupt bob's public vault

Tools Used

Recommended Mitigation Steps

The corresponding vault address is stored in s.lienMeta[id].orginOwner when the LienToken is created, this is not modified. Get the vault address from this variable, not from ownerOf(id).

Judge has assessed an item in Issue #598 as M risk. The relevant finding follows:

Low: 1.deposit() check wrong variant

function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {

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

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

Judge has assessed an item in Issue #598 as M risk. The relevant finding follows:

2.minDepositAmount() When the asset is btc, the minDepositAmount is too large

when asset == btc , minDepositAmount = 0.1 btc , equal 2000 usd suggest:

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

• return 10**(ERC20(asset()).decimals() /2);
  

  } }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

makePayment() will be called when the user needs to repay the money, in two cases.

1. If the repayment amount >= (borrowed amount + interest), then the loan is paid off and the corresponding LienToken is deleted
2. If the repayment amount < (borrowed amount + interest), then the borrowed amount = the remaining amount (borrowed amount + interest - repayment amount)

Case 2, if the repayment amount is not enough when not taking into account a situation, the repayment amount is not even enough interest, according to case 2 of the principal becomes larger, resulting in the subsequent interest will become larger

Example: alice borrowing amount:1000 rate:10%/year borrowing 2 years After 1 year borrowing amount = 1000, interest = 100

At this point the user repays: 50, which will result in alice's borrowing amount becoming 1050

When the two years are up, alice will have to repay a total of: 1050 + 1050*10% = 1155 Together with the 50 already repaid, alice actually repay 1155 + 50 = 1205

Normal if 2 years at most should be to pay back 1000 + 1000 * 10% * 2 = 1200

As above, resulting in an extra repayment : 5 So case 2 is not reasonable

Tools Used

Recommended Mitigation Steps

If the repayment amount is less than the current interest rate, directly revert

Low: 1.deposit() check wrong variant

  function deposit(uint256 assets, address receiver)
    public
    virtual
    returns (uint256 shares)
  {

-    require(shares > minDepositAmount(), "VALUE_TOO_SMALL");
+    require(assets > minDepositAmount(), "VALUE_TOO_SMALL");

2.minDepositAmount() When the asset is btc, the minDepositAmount is too large

when asset == btc , minDepositAmount = 0.1 btc , equal 2000 usd suggest:

  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);
+     return 10**(ERC20(asset()).decimals() /2);
    }
  }