Attackers can create as many loans as possible for the collateral's owner

[code423n4]

Lines of code

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

Vulnerability details

Vulnerable detail

When borrowers want to take a loan, they can call VaultImplementation.commitToLien(). This function will first validate the commitment to

• Check if msg.sender / receiver has correct permission to create liens
• Check if strategist has approved this lienRequest

  function _validateCommitment(
    IAstariaRouter.Commitment calldata params,
    address receiver
  ) internal view {
    /// [#explain] check if msg.sender / receiver has correct permission 
    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);
    }
      
    /// [#explain] check whether strategist has approved this lienRequest 
    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
      );
    }
  }

then transfer the loan from vault to receiver.

There are 2 things to note here is:

• Anyone can call this function if receiver was set to holder / operator
• function _encodeStrategyData returns the hash = keccak256(abi.encode(STRATEGY_TYPEHASH, s.strategistNonce, strategy.deadline, root));

This will lead to a opportunity for attackers to use the same parameters (s.strategistNonce, strategy.deadline, root, v, r, s) of the transaction that borrowers has used to take a loan before to create as many loans as possible for borrowers.

This will make borrowers take more loans than they expected.

Impact

Borrowers take more loans than expected. Careless users who don't know about the new loans can be liquidated and lose their NFTs.

Tools Used

Manual review

Recommended Mitigation Steps

Consider to require msg.sender must be holder/operator when validate a commitment.

uint256 collateralId = params.tokenContract.computeId(params.tokenId);
ERC721 CT = ERC721(address(COLLATERAL_TOKEN()));
address holder = CT.ownerOf(collateralId);
address operator = CT.getApproved(collateralId);

/// change here !!!
if (
  msg.sender != holder &&
  !CT.isApprovedForAll(holder, msg.sender)
) {
  revert InvalidRequest(InvalidRequestReason.NO_AUTHORITY);
}

[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