Astaria contest - HE1M's results

Lines of code

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

Vulnerability details

Impact

The function safeTransferFrom is used when an NFT is auctioned, and it is supposed to handle the payments done for the NFT in seaport. The payment is done through a token which is not validated. So, an attacker can use a fake token for the payment.

Proof of Concept

The function safeTransferFrom can be called by anyone, so:

• Bob (an attacker) calls the function safeTransferFrom to buy the auctioned NFT. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L169

• He provides the parameter identifier, so that its last 20 bytes is the address of a malicious contract under his control. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L172 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L123

• So, the paymentToken is a fake token under control of Bob.

• It returns a value larger than currentOfferPrice, when it is called in the line: https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L132

• It does not do anything when called in the following lines: https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L143 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L148 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L161

• During the call to the function payDebtViaClearingHouse, this fake token does not do anything either. payDebtViaClearingHouse >> _payDebt >> _paymentAH https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L153 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L497 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L523 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L643

• As a result, all the debt payments are done for the auctioned NFT with a fake token. In other words, it is not validated that the token which is going to be used for payments of the NFT is a valid and accepted one or not.

In summary, the attacker could use a fake token for buying the NFT and paying the debts.

Tools Used

Recommended Mitigation Steps

The identifier parameter should be validated.

Lines of code

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

Vulnerability details

Impact

The ClearingHouse is supposed to be the owner of the collateralized NFT always. The only time that it is not the owner, is during flashAction in which the NFT will be transferred back to the ClearingHouse at the end of the transaction. The ownership is tracked by ownerOf(collateralId), and the NFT data is tracked by s.idToUnderlying[collateralId]. In a normal case, if an NFT is going to be released by calling the function releaseToAddress, these data will be burned and removed, ownerOf(collateralId) = address(0) and delete s.idToUnderlying[collateralId]. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L331

But, this is not the case if the NFT is going to be released by a liquidator during an auction.

Proof of Concept

• Suppose Alice owns an NFT and transfers it to CollateralToken.sol to be used as collateral. This transfer triggers the onERC721Received:

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

• This contract will assign a collateralId to this NFT by appending the token contract address and token id.

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

• This contract will deploy a ClearingHouse contract and transfers this NFT to that contract.

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

• This contract will mint a collateral id for the owner of this NFT.

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

• So, we will have:

  • ERC721(tokenContract).ownerOf(tokenId) = the address of the deployed contract ClearingHouse
  • s.clearingHouse[collateralId] = the address of the deployed contract ClearingHouse
  • ownerOf(collateralId) = Alice
  • s.idToUnderlying[collateralId].tokenContract = address of the token contract

• Suppose later, Alice is going to be liquidated, and her collateral (her NFT which is locked in the ClearingHouse) is auctioned.

• After the liquidation, the liquidator should claim this NFT by calling liquidatorNFTClaim.

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

• This function calls the internal function _releaseToAddress.

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

• In this function, the NFT will be transferred to the liquidator. So, we will have:

  • ERC721(tokenContract).ownerOf(tokenId) = the address of liquidator
  • s.clearingHouse[collateralId] = the address of the deployed contract ClearingHouse
  • ownerOf(collateralId) = Alice
  • s.idToUnderlying[collateralId].tokenContract = address of the token contract

• This is the vulnerability, because that NFT is transferred to the liquidator, but the mapping idToUnderlying is not deleted and the minted collaterId is not burned. In other words, it is correct to have the following states:

  • ERC721(tokenContract).ownerOf(tokenId) = the address of liquidator
  • s.clearingHouse[collateralId] = the address of the deployed contract ClearingHouse
  • ownerOf(collateralId) = address(0)
  • s.idToUnderlying[collateralId].tokenContract = address(0)

• This can lead to some issues. For example, if the liquidator sells this NFT to another innocent user (Bob):

  • First: If Bob intends to use this NFT again as a collateral in the protocol by transferring it to contract CollateralToken, it will be failed. Because s.idToUnderlying[collateralId].tokenContract is nonzero, and it means that this NFT has already used. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L598
  • Second: If Bob transfers this NFT to the ClearingHouse already deployed for this NFT, he will loss this NFT, and can not take it back. Because, we have still ownerOf(collateralId) = Alice, so only Alice will be able to release this NFT from the ClearingHouse. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L338

• The situation can be even worse:

  • Alice can request for a loan (although her NFT is sold to the liquidator during the auction) by calling commitToLien. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L287
  • During validation of the loan request, it verifies the merkle proof for a loan commitment (since this one was verified before, it will again verify). The important part is that CT.ownerOf(collateralId) is still Alice not address(0). Although, Alice's NFT was sold to the liquidator in the auction, but collateralId is not burned, so Alice is still owner of this collateralId. commitToLien >> _requestLienAndIssuePayout >> _validateCommitment https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L235
  • So, it seems that Alice can get a loan although she does not have any NFT as collateral.

In summary, the issue is that the mapping and burning are done only in the function releaseToAddress, while the internal function _releaseToAddress is called during claiming the NFT by the liquidator in the function liquidatorNFTClaim.

Tools Used

Recommended Mitigation Steps

The following modifications should be applied:

  // Before
  
  function releaseToAddress(uint256 collateralId, address releaseTo)
    public
    releaseCheck(collateralId)
    onlyOwner(collateralId)
  {
    CollateralStorage storage s = _loadCollateralSlot();

    if (msg.sender != ownerOf(collateralId)) {
      revert InvalidSender();
    }
    Asset memory underlying = s.idToUnderlying[collateralId];
    address tokenContract = underlying.tokenContract;
    _burn(collateralId);
    delete s.idToUnderlying[collateralId];
    _releaseToAddress(s, underlying, collateralId, releaseTo);
  }

  function _releaseToAddress(
    CollateralStorage storage s,
    Asset memory underlyingAsset,
    uint256 collateralId,
    address releaseTo
  ) internal {
    ClearingHouse(s.clearingHouse[collateralId]).transferUnderlying(
      underlyingAsset.tokenContract,
      underlyingAsset.tokenId,
      releaseTo
    );
    emit ReleaseTo(
      underlyingAsset.tokenContract,
      underlyingAsset.tokenId,
      releaseTo
    );
  }

//After  
  function releaseToAddress(uint256 collateralId, address releaseTo)
    public
    releaseCheck(collateralId)
    onlyOwner(collateralId)
  {
    CollateralStorage storage s = _loadCollateralSlot();

    if (msg.sender != ownerOf(collateralId)) {
      revert InvalidSender();
    }
    Asset memory underlying = s.idToUnderlying[collateralId];
    _releaseToAddress(s, underlying, collateralId, releaseTo);
  }

  function _releaseToAddress(
    CollateralStorage storage s,
    Asset memory underlyingAsset,
    uint256 collateralId,
    address releaseTo
  ) internal {
    _burn(collateralId);
    delete s.idToUnderlying[collateralId];  
    ClearingHouse(s.clearingHouse[collateralId]).transferUnderlying(
      underlyingAsset.tokenContract,
      underlyingAsset.tokenId,
      releaseTo
    );
    emit ReleaseTo(
      underlyingAsset.tokenContract,
      underlyingAsset.tokenId,
      releaseTo
    );
  }

No. 1 Redundant variable and calculation

The following line does nothing: https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L262

No. 2 Redundant check

The following line is redundant as the modifier onlyOwner does the same job: https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L338-L340 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L334

No. 3 Wrong comment

The terms are hashed and singed by the strategist not the borrower, so the following comment should be modified. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L223