commitToLien() can create LienToken for any holder

[code423n4]

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

The VaultImplementation.commitToLien() method is external and can be executed by anyone
The method will internally verify that the corresponding collateralId is yours or has the corresponding authorization
The validation code is as follows:

  function _validateCommitment(
    if (
      msg.sender ! = holder &&
      receiver ! = holder && 
      receiver ! = operator &&
      !CT.isApprovedForAll(holder, msg.sender)
    ) {
      revert InvalidRequest(InvalidRequestReason.NO_AUTHORITY);
    }

Note that this receiver comes from the parameter, so anyone can pass receiver==holder to skip this authentication, i.e. anyone can createLien for the owner of the collateralId

There are many possible problems that can be caused:
For example:

1. if the collateralId has not been borrowed (only transfer NFT to CollateralToken), then the malicious user can generate a private vault, vault.asset as a worthless token, then set a very short borrowing period policy, and then through the above commitToLien() to borrow money for others
   Since the period is very short, it will soon expire and enter the auction. Use worthless assets to bid to steal NFT

2. collateralId already has Lien, then the user can generate a very high interest rate strategy, and then generate Lien for someone else's borrowing through the above commitToLien(), thus earning interest

3. collateralId borrowed without knowing it

Suggestion:

Only verify msg.sender, do not verify receiver

Tools Used

Recommended Mitigation Steps

  function _validateCommitment(
    IAstariaRouter.Commitment calldata params,
    address receiver
  ) internal view {
...
    if (
      msg.sender != holder &&
-     receiver != holder && 
-     receiver != operator &&
+     msg.sender != operator && 
      !CT.isApprovedForAll(holder, msg.sender)
    ) {
      revert InvalidRequest(InvalidRequestReason.NO_AUTHORITY);
    }  

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

SantiagoGregory marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

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

[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 not a duplicate

[c4-judge]

Picodes marked the issue as duplicate of #19