Releasing an NFT by the liquidator does not update the mapping idToUnderlying and does not burn the collateralId

[code423n4]

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

[c4-sponsor]

SantiagoGregory marked the issue as sponsor confirmed

[androolloyd]

same underlying issue as this, #480

[c4-sponsor]

androolloyd requested judge review

[c4-judge]

Picodes marked the issue as duplicate of #480

[c4-judge]

Picodes marked the issue as selected for report

[c4-judge]

Picodes marked issue #480 as primary and marked this issue as a duplicate of 480

[c4-judge]

Picodes marked the issue as satisfactory