Submitted by Lirios, also found by c7e7eff, Koolex, wallstreetvilkas, bin2chen, 0xcm, Jeiwan, 0xsomeone, synackrst, cergyk, evan, dragotanqueray, HE1M, unforgiven, and ladboy233
When a borrower does not return the borrowed funds on time, a liquidator can trigger a liquidation.
In that case the collateral NFT will be listed in an Seaport dutch auction.
The auction requests settlementToken and a fake ClearingHouse NFT.
When a buyer bids enough of the settlementToken, openSea auction will accept the offer, transfer the NFT from ClearingHouse to bidder, move settlementToken from bidder to ClearingHouse, and transfers the fake clearinghouse NFT to clearinghouse. This call to ClearingHouse.safeTransferFrom triggers the further processing of the liquidation. It will pay the debt with the funds received from Seaport, and delete data from LienToken and CollateralToken for this collateral NFT.
The problem is that the ClearingHouse.safeTransferFrom can be called by anyone and assumes valid call parameters. One of the parameters identifier is used to pass the paymentToken address. This can easily be modified to let the contract accept any ERC20 token as paymentToken to payoff the debt.
This allows a malicious actor to lock a users collateral NFT and cancel the auction.
This could be misused to completely block any liquidatons.
The steps to reproduce:
At this time a Seaport auction is initiated and CollateralToken state for this collateralID is updated to be in auction.
All fine so far.
After this call, the Collateral NFT will still be in de ClearingHouse contract, but references to the NFT are cleaned up from both CollateralToken and LienToken.
This results in the NFT being locked in the contract without any way to get it out.
When a liquidation is started, _generateValidOrderParameters is called to generate the Seaport order params. It sets settlementToken as the identifierOrCriteria.
ClearingHouse assumes that safeTransferFrom will only be called by Seaport after a succesful auction, and assumes the identifier is the settlementToken value that was set for the order.
The _execute function is called, which converts the identifier parameter to a paymentToken address and checks if the received amount of paymentToken is >= the expected auction currentOfferPrice it accepts the call and moves the token balance to the correct addresses.
It then calls LienToken.payDebtViaClearingHouse, passing the fake paymentToken as a parameter.
Lientoken contract also does not verify if the token is ineed the correct settlementToken and pays off the debt with the fake token balance.
It then deletes the collateralStateHash for the collateralId, removing the stack state.
After that,  CollateralToken.settleAuction is called, which burns the token for collateralId and deletes idToUnderlying and collateralIdToAuction for this collateralId.
We now have a state where the collateral NFT is in the ClearingHouse contract, but all actions are made impossible, because the state in the token contracts are removed.
CollateralToken.ownerOf(collateralID) reverts because the entry in s.idToUnderlying was removed.
This causes releaseToAddress() and flashAction() to fail.
liquidatorNFTClaim fails because s.collateralIdToAuction was cleared.
Trying to create a new Lien also fails as that calls CollateralToken.ownerOf(collateralID).
To test the scenario, I have modified the testLiquidationNftTransfer test in AstariaTest.t.sol
After this, the assert the bidder received the NFT test will fail, as the NFT is not moved.
But the state of the CollateralToken and LienToken contracts is updated.
Manual audit, forge
Minimal fix would be to check either check if the supplied paymentToken matches the expected paymentToken, or ignore the parameter alltogether and use the paymentToken from the contract. Other option is to restrict calls to the function to whitelisted addresses (OpenSea controler and conduit ).
In the current setup, there is no easy way for the ClearingHouse to access information about the settlementToken.
It could be added to the AuctionData struct:
and then add it in LienToken
In the ClearingHouse it can be used directly, ignoring the supplied parameter:
or used to check the supplied paramameter
With this step added, it would still be possible to lock the NFT in the contract, but this time that will only succeed when the requested auction amount is paid. So in that case it would be more logical to simply bid on OpenSea and also get the NFT instead of paying the tokens just to lock it.
SantiagoGregory (Astaria) confirmed via duplicate issue #564
