attacker can cause grief and fund loss for collateral owner because _validateCommitment() allows for anyone to create loan for collateral if receiver is owner or operator

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L229-L244
https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L287-L306
https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L379-L395

Vulnerability details

Impact

users can get loan for their collaterals by calling commitToLien() and code checks that caller is the holder of the collateral or receiver of the loan is holder or operator of the collateral (ERC721 operator). This would give attacker opportunity to issue unwanted loans(bad interest rate or other bad loan parameters) for collateral owners and cause collateral owner to lose funds because of the unfair loans and fee. in the extreme case where collateral owner sets another contract as operator (it's common to set AstariaRouter as collateral operator because users interact with router and router manage their funds in behalf of them) attacker can create a loan for user collateral for that operator and those loans would stuck in the operator address and user collateral would get liquidated after the loan duration. This is a critical issue because attacker can make all the collateral owners to lose their NFT and also break the protocol basic feature (attacker can create unwanted loans for everyone).

Proof of Concept

This is commitToLien() and _requestLienAndIssuePayout() code:

  function commitToLien(
    IAstariaRouter.Commitment calldata params,
    address receiver
  )
    external
    whenNotPaused
    returns (uint256 lienId, ILienToken.Stack[] memory stack, uint256 payout)
  {
    _beforeCommitToLien(params);
    uint256 slopeAddition;
    (lienId, stack, slopeAddition, payout) = _requestLienAndIssuePayout(
      params,
      receiver
    );
    _afterCommitToLien(
      stack[stack.length - 1].point.end,
      lienId,
      slopeAddition
    );
  }

  function _requestLienAndIssuePayout(
    IAstariaRouter.Commitment calldata c,
    address receiver
  )
    internal
    returns (
      uint256 newLienId,
      ILienToken.Stack[] memory stack,
      uint256 slope,
      uint256 payout
    )
  {
    _validateCommitment(c, receiver);
    (newLienId, stack, slope) = ROUTER().requestLienPosition(c, recipient());
    payout = _handleProtocolFee(c.lienRequest.amount);
    ERC20(asset()).safeTransfer(receiver, payout);
  }

As you can see this functions validate loan commitment and sends loan to receiver address which msg.sender specified. to validate commitment code calls _validateCommitment() which is:

  function _validateCommitment(
    IAstariaRouter.Commitment calldata params,
    address receiver
  ) internal view {
    uint256 collateralId = params.tokenContract.computeId(params.tokenId);
    ERC721 CT = ERC721(address(COLLATERAL_TOKEN()));
    address holder = CT.ownerOf(collateralId);
    address operator = CT.getApproved(collateralId);
    if (
      msg.sender != holder &&
      receiver != holder &&
      receiver != operator &&
      !CT.isApprovedForAll(holder, msg.sender)
    ) {
      revert InvalidRequest(InvalidRequestReason.NO_AUTHORITY);
    }
    VIData storage s = _loadVISlot();
    address recovered = ecrecover(
      keccak256(
        _encodeStrategyData(
          s,
          params.lienRequest.strategy,
          params.lienRequest.merkle.root
        )
      ),
      params.lienRequest.v,
      params.lienRequest.r,
      params.lienRequest.s
    );
    if (
      (recovered != owner() && recovered != s.delegate) ||
      recovered == address(0)
    ) {
      revert IVaultImplementation.InvalidRequest(
        InvalidRequestReason.INVALID_SIGNATURE
      );
    }
  }

As you can see code validates loan when receiver of the loan is holder of the collateral or operator of the collateral (to support AstariaRouter actions on behalf of the user). There is no other checks or limits to prevent attacker from calling commitToLien() and issue loan for other user's collateral. attacker can use this and create unwanted loans for user or operator and cause funds loss. these are the steps attacker would do:

1. user1 deposits his NFT as collateral and receives collateral token NFT.
2. user1 sets AstariaRouter as his collateral token operator (or any other contract) to call AstariaRouter functions and manage his funds.
3. attacker would call commitToLien(user1's collateral, AstariaRouter) for user1 collateral and set AstariaRouter as receiver of the loan and code would check and validate this call and would create a loan for user1's collateral and send tokens to AstariaRouter address.
4. AstariaRouter has no logic to handle loans and pay them back and after loan duration user1's collateral would get liquidated and user1 would lose his funds and receive no loan or token.
5. attacker can create unfair available loans for user1's collateral and set user1 as receiver and cause greifing, user1 has to pay fee and interest rate and would have unfair unwanted loan.

This is a critical issue because attacker can perform this attack on every collateral provider and cause basic functionality of the protocol to be broken. if attacker create loan for operator users lose their NFT for nothing.

Tools Used

VIM

Recommended Mitigation Steps

allow loan creation when msg.sender is operator or holder or approved for all.

[c4-judge]

Picodes marked the issue as duplicate of #292

[c4-judge]

Picodes marked the issue as duplicate of #565

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

Picodes marked the issue as satisfactory