Astaria contest - KIntern_NA's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L160-L165

Vulnerability details

Proof of concept

Function ClearingHouse.safeTransferFrom is used to pay debt with auction funds. This action can be executed when the balance of clearing house is larger than currentOfferPrice.

// url = https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L125-L134
uint256 currentOfferPrice = _locateCurrentAmount({
  startAmount: s.auctionStack.startAmount,
  endAmount: s.auctionStack.endAmount,
  startTime: s.auctionStack.startTime,
  endTime: s.auctionStack.endTime,
  roundUp: true //we are a consideration we round up
});
uint256 payment = ERC20(paymentToken).balanceOf(address(this));
require(payment >= currentOfferPrice, "not enough funds received");

When this condition has reached, the clearing house will

1. reward the liquidator a portion of auction funds
2. pay debt for the lien token
3. if there remains any funds after 2 steps above, clearing house will send it to the borrower (owner of collateral)

The last transferring action can induce some potential risk for the vault if the payment token is ERC777. ERC777 is one of the ERC20 variation which notifies receiver after transferring tokens. With technique of this ERC20 variation, borrower can totally deny all the transferring token from clearing house to him on his callback function. This will make the function ClearingHouse.safeTransferFrom revert and no debt will be repayed.

Furthermore if the paymentToken is USDC (not ERC777), the borrower can send the collateral token to one of blacklisted users and make the last transfer failed --> revert the transaction.

Beside this issue can apply for the liquidator, when he can deny the transferring and make the repayDebt failed.

Impact

Debt in LienToken can't be repayed.

Tools Used

Manual review

Recommended Mitigation Steps

Have one more function for the user to claim their fund by themself and update the claimable balance for each user in storage.

mapping(address => uint) claimableBalance;

function claimReward(address token, uint amount, address receiver) external {
    claimableBalance[msg.sender] -= amount;
    if (amount > 0) {
        ERC20(token).safeTransfer(receiver, amount);
    }
}

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L143-L146

Vulnerability details

Proof of concept

When a user transfer an NFT to CollateralToken contract, it will toggle the function CollateralToken.onERC721Received(). In this function if there didn't exist any clearingHouse for the collateralId, it will create a new one for that collateral.

if (s.clearingHouse[collateralId] == address(0)) {
    address clearingHouse = ClonesWithImmutableArgs.clone(
      s.ASTARIA_ROUTER.BEACON_PROXY_IMPLEMENTATION(),
      abi.encodePacked(
        address(s.ASTARIA_ROUTER),
        uint8(IAstariaRouter.ImplementationType.ClearingHouse),
        collateralId
      )
    );

    s.clearingHouse[collateralId] = clearingHouse;
  }

The interesting thing of this technique is: there will be just one clearingHouse be used for each collateral no matter how many times the collateral is transferred to the contract. Even when the lien is liquidated / fully repayed, the s.clearingHouse[collateralId] remain unchanged.

The question here is any stale datas in clearingHouse from the previous time that the nft was used as collateral can affect the behavior of protocol when the nft was transfered to CollateralToken again ? Let take a look at the function ClearingHouse._execute(). In this function, the implementation uses safeApprove() to approve payment - liquidatorPayment amount for the TRANSFER_PROXY.

ERC20(paymentToken).safeApprove(
  address(ASTARIA_ROUTER.TRANSFER_PROXY()),
  payment - liquidatorPayment
);

the safeApprove function will revert if the allowance was set from non-zero value to non-zero value. This will incur some potential risk for the function like example below:

1. NFT x is transferred to CollateralToken to take loans and then it is liquidated.
2. At time 10, function ClearingHouse._execute() was called and the payment - liquidatorPayment > totalDebt. This will the paymentToken.allowance[clearingHouse][TRANSFER_PROXY] > 0 after the function ended.
3. NFT x is transferred to CollateralToken for the second time to take a loans and then it is liquidated again.
4. At time 15 (> 10), function ClearingHouse._execute() was called, but at this time, the safeApprove will revert since the previous allowance is different from 0

Impact

The debt can be repayed by auction funds when liquidation.

Tools Used

Manual review

Recommended Mitigation Steps

Consider to use approve instead of safeApprove.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L169-L178 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L524-L539

Vulnerability details

Vulnerable details

Function ClearingHouse.safeTransferFrom() is used to repay debt for the vault after the nft was sold. Since this function forget to check the collateralId is liquidated or not, anyone can call this function before the liquidation.

Because the liquidation hasn't happened, the currentOfferPrice will equal to 0 (because startAmount = endAmount = startTime = endTime = 0) and the payment = balanceOf(address(this)) = 0. It will make the function pass all the checks and transfer function without incurring any cost to execute. Eventually, this function will call CollateralToken.settleAuction().

  function settleAuction(uint256 collateralId) public {
    CollateralStorage storage s = _loadCollateralSlot();
    if (
      s.collateralIdToAuction[collateralId] == bytes32(0) &&
      ERC721(s.idToUnderlying[collateralId].tokenContract).ownerOf(
        s.idToUnderlying[collateralId].tokenId
      ) !=
      s.clearingHouse[collateralId]
    ) {
      revert InvalidCollateralState(InvalidCollateralStates.NO_AUCTION);
    }
    require(msg.sender == s.clearingHouse[collateralId]);
    _settleAuction(s, collateralId);
    delete s.idToUnderlying[collateralId];
    _burn(collateralId);
  }

Function above will _burn the collateralId of the borrower. It will make the borrower unable to claim his nft back even he repays all the loan.

Impact

Borrowers lose their nfts.

Proof of concept

// SPDX-License-Identifier: BUSL-1.1
pragma solidity =0.8.17;

import "forge-std/Test.sol";
import "forge-std/console.sol";

import "./TestHelpers.t.sol";
import {OrderParameters} from "seaport/lib/ConsiderationStructs.sol";
import {ClearingHouse} from "core/ClearingHouse.sol";

contract Test1 is TestHelpers {
  using FixedPointMathLib for uint256;
  using CollateralLookup for address;
  using SafeCastLib for uint256;

  function test1() public {
    TestNFT nft = new TestNFT(1);
    address tokenContract = address(nft);
    uint256 tokenId = uint256(0);

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

    // create a PublicVault with a 14-day epoch
    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 14 days
    });

    // lend 50 ether to the PublicVault as address(1)
    _lendToVault(
      Lender({addr: address(1), amountToLend: 50 ether}),
      publicVault
    );

    // borrow 10 eth against the dummy NFT
    (, ILienToken.Stack[] memory stack) = _commitToLien({
      vault: publicVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: tokenId,
      lienDetails: standardLienDetails,
      amount: 10 ether,
      isFirstLien: true
    });

    uint256 collateralId = tokenContract.computeId(tokenId);

    // make sure the borrow was successful
    assertEq(WETH9.balanceOf(address(this)), initialBalance + 10 ether);

    ClearingHouse clearingHouse = COLLATERAL_TOKEN.getClearingHouse(collateralId);

    address owner = COLLATERAL_TOKEN.ownerOf(collateralId);
    assertEq(owner, address(this));

    address alice = vm.addr(0xa11ce);
    vm.prank(alice);
    clearingHouse.safeTransferFrom(address(COLLATERAL_TOKEN), address(0), uint256(uint160(address(WETH9))), 0, '');
    
    vm.expectRevert(bytes("NOT_MINTED"));
    owner = COLLATERAL_TOKEN.ownerOf(collateralId);
  }
}

Tools Used

Foundry, Manual review

Recommended Mitigation Steps

Should check whether collateral was liquidated when anyone calling ClearingHouse.safeTransferFrom()

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);
}

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L143-L146

Vulnerability details

Vulnerable details

Function ClearingHouse.safeTransferFrom is used to pay debt with auction funds. It calls the internal function _execute that will transfer the payment reward to liquiditor, pay debt for lien token, and transfer all the remains to the owner of this collateral. If the payment token is ERC777, there is a risk of reentrancy because of the callback function (IERC777recipient.tokensReceived) in validator (contract).

Proof of concept

Let's see function _execute in contract ClearingHouse.sol:

ERC20(paymentToken).safeTransfer(
  s.auctionStack.liquidator,
  liquidatorPayment
);

ERC20(paymentToken).safeApprove(
  address(ASTARIA_ROUTER.TRANSFER_PROXY()),
  payment - liquidatorPayment
);

ASTARIA_ROUTER.LIEN_TOKEN().payDebtViaClearingHouse(
  paymentToken,
  collateralId,
  payment - liquidatorPayment,
  s.auctionStack.stack
);

if (ERC20(paymentToken).balanceOf(address(this)) > 0) {
  ERC20(paymentToken).safeTransfer(
    ASTARIA_ROUTER.COLLATERAL_TOKEN().ownerOf(collateralId),
    ERC20(paymentToken).balanceOf(address(this))
  );
}

Malicious validator can reentrant function _execute if the validator is a contract that implemented function tokensReceived to call ClearingHouse.safeTransferFrom again when the payment token is ERC777. Then contract ClearingHouse will call payDebtViaClearingHouse many times with different payments and the same s.auctionstack.stack. It can lead to users losing funds

Example: In the case of ERC777, if payment = 10 and liquidatorPayment = 1 and the total debt of this collateral = 4, then owner of this collateral should take 10 - 1 - 4 = 5 revenue. But if the validator is the malicious contract that will reentrant more than 1 time, the debt will be paid twice and the owner will take only 10 - 1 - 4*2 = 1 token.

Impact

If the payment token is ERC777, the owner of collateral can lose the revenue after liquidating.

Tools Used

Manual review

Recommended Mitigation Steps

Consider to use ReentrancyGuard

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L406 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L394 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/TransferProxy.sol#L34

Vulnerability details

Vulnerability Detail

There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One of these customization is the one which revert the transfer transaction when amount = 0. You can find more informations about this type of token in weird ERC20.

Function _handleProtocolFee() is implemented as follows:

  function _handleProtocolFee(uint256 amount) internal returns (uint256) {
    address feeTo = ROUTER().feeTo();
    bool feeOn = feeTo != address(0);
    if (feeOn) {
      uint256 fee = ROUTER().getProtocolFee(amount);


      unchecked {
        amount -= fee;
      }
      ERC20(asset()).safeTransfer(feeTo, fee);
    }
    return amount;
  }
}

It hasn't checked fee != 0 before transferring, then it can revert with customized ERC20 mentioned above.

Impact

The function doesn't work as expected, it will revert if realAmount = 0 and the baseAsset is ERC20 which reverts the transfer transaction when amount = 0.

Tool used

Manual Review

Recommendation

Check whether amount > 0 before executing the transfer.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L781-L785 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L518-L520 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L511

Vulnerability details

Vulnerable detail

There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().

Assume vault.asset() is a deflationary token. When the borrower calls function AstariaRouter.commitToLiens() to request loans. Router will call to function VaultImplementation.commitToLien() to transfer money from vault to router.

  function _executeCommitment(
    RouterStorage storage s,
    IAstariaRouter.Commitment memory c
  )
    internal
    returns (
      uint256,
      ILienToken.Stack[] memory stack,
      uint256 payout
    )
  {
    uint256 collateralId = c.tokenContract.computeId(c.tokenId);


    if (msg.sender != s.COLLATERAL_TOKEN.ownerOf(collateralId)) {
      revert InvalidSenderForCollateral(msg.sender, collateralId);
    }
    if (!s.vaults[c.lienRequest.strategy.vault]) {
      revert InvalidVault(c.lienRequest.strategy.vault);
    }
    //router must be approved for the collateral to take a loan,
    return
      IVaultImplementation(c.lienRequest.strategy.vault).commitToLien(
        c,
        address(this)
      );
  }

It then transfer totalBorrowed amount to borrower. Since vault.asset is a FoT, router may receive less than totalBorrow and make the execution of transferring from router to sender revert.

Impact

Borrower can't take a loan

Tools Used

Manual review

Recommended Mitigation Steps

Consider not to support Fee on Transfer token, or calculate the balance before and after a transfer to determine exactly amount of tokens receive.