Jeiwan - Denial of service in `AstariaRouter.commitToLiens`

[sherlock-admin]

Jeiwan

medium

Denial of service in AstariaRouter.commitToLiens

Summary

Denial of service in AstariaRouter.commitToLiens

Vulnerability Detail

The commitToLiens function of AstariaRouter (AstariaRouter.sol#L249) always tries to send a CollateralToken to the caller while not being the owner of the token–this will always cause a revert.

First, the function deposits a caller's NFT collateral token (AstariaRouter.sol#L578):

function _transferAndDepositAsset(address tokenContract, uint256 tokenId)
  internal
{
  IERC721(tokenContract).safeTransferFrom(
    address(msg.sender),
    address(COLLATERAL_TOKEN),
    tokenId,
    ""
  );
}

The depositing is handled here (CollateralToken.sol#L266):

function onERC721Received(
  address operator_,
  address from_,
  uint256 tokenId_,
  bytes calldata data_
) external override returns (bytes4) {
  uint256 collateralId = msg.sender.computeId(tokenId_);

  (address underlyingAsset, ) = getUnderlying(collateralId);
  if (underlyingAsset == address(0)) {
    if (msg.sender == address(this) || msg.sender == address(LIEN_TOKEN)) {
      revert("system assets are not valid collateral");
    }

    address depositFor = operator_;

    if (operator_ != from_) {
      depositFor = from_;
    }

    _mint(depositFor, collateralId);

    idToUnderlying[collateralId] = Asset({
      tokenContract: msg.sender,
      tokenId: tokenId_
    });

    emit Deposit721(msg.sender, tokenId_, collateralId, depositFor);
  }
  return IERC721Receiver.onERC721Received.selector;
}

Even though AstariaRouter is an approved spender/operator, the CollateralToken will be minted to the actual owner of the collateral (the from_ argument). Thus, when commitToLiens tries to send a CollateralToken to the caller, it will always revert (AstariaRouter.sol#L589):

function _returnCollateral(uint256 collateralId, address receiver) internal {
  COLLATERAL_TOKEN.transferFrom(address(this), receiver, collateralId);
}

Impact

The commitToLiens function of AstariaRouter will always revert.

Code Snippet

AstariaRouter.sol#L249:

function commitToLiens(IAstariaRouter.Commitment[] calldata commitments)
  external
  whenNotPaused
  returns (uint256 totalBorrowed)
{
  totalBorrowed = 0;
  for (uint256 i = 0; i < commitments.length; ++i) {
    // @audit CollateralToken is minted to the collateral owner
    _transferAndDepositAsset(
      commitments[i].tokenContract,
      commitments[i].tokenId
    );
    totalBorrowed += _executeCommitment(commitments[i]);

    uint256 collateralId = commitments[i].tokenContract.computeId(
      commitments[i].tokenId
    );
    // @audit this contract is not the owner of the minted CollateralToken
    _returnCollateral(collateralId, address(msg.sender));
  }
  WETH.safeApprove(address(TRANSFER_PROXY), totalBorrowed);
  TRANSFER_PROXY.tokenTransferFrom(
    address(WETH),
    address(this),
    address(msg.sender),
    totalBorrowed
  );
}

Tool used

Manual Review

Recommendation

Consider removing the call to _returnCollateral from the commitToLiens function since the caller is already the owner of the CollateralToken. Also, consider improving the test coverage of the AstariaRouter contract.

Duplicate of #204