Astaria contest - caventa's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L592-L609

Vulnerability details

Impact

The vault fee could be larger than the payment amount.

Proof of Concept

Every payment could have a service fee. However, it is ridiculous if the fee is larger than the amount.

I change the existing _createPublicVault function of TestHelpers.t.sol

  function _createPublicVault(
    address strategist,
    address delegate,
    uint256 epochLength
  ) internal returns (address publicVault) {
    vm.startPrank(strategist);
    //bps
    publicVault = ASTARIA_ROUTER.newPublicVault(
      epochLength,
      delegate,
      address(WETH9),
      --- uint256(0),
      +++ uint256(10000000),
      false,
      new address[](0),
      uint256(0)
    );
    vm.stopPrank();
  }

Also, I amend the _handleStrategistInterestReward function of PublicVault.sol

  function _handleStrategistInterestReward(
    VaultData storage s,
    uint256 interestOwing,
    uint256 amount
  ) internal virtual {
    if (VAULT_FEE() != uint256(0)) {
      uint256 x = (amount > interestOwing) ? interestOwing : amount;
      uint256 fee = x.mulDivDown(VAULT_FEE(), 10000);
      +++ console.log(amount);
      +++ console.log(fee);
      uint88 feeInShares = convertToShares(fee).safeCastTo88();
      s.strategistUnclaimedShares += feeInShares;
      emit StrategistFee(feeInShares);
    }
  }

When I run

forge test --ffi -vv --match-test testBasicPublicVaultLoan

The amount is 10000000000000000000

The fee is 369863013693600000000

where 369863013693600000000 is almost 36 times larger than the amount

Tools Used

Manual and some testing code

Recommended Mitigation Steps

Fee should be smaller than amount

Change the _handleStrategistInterestReward function

  function _handleStrategistInterestReward(
    VaultData storage s,
    uint256 interestOwing,
    uint256 amount
  ) internal virtual {
    if (VAULT_FEE() != uint256(0)) {
      uint256 x = (amount > interestOwing) ? interestOwing : amount;
      uint256 fee = x.mulDivDown(VAULT_FEE(), 10000);
      +++ require(fee < amount, "Fee should less than amount");
      uint88 feeInShares = convertToShares(fee).safeCastTo88();
      s.strategistUnclaimedShares += feeInShares;
      emit StrategistFee(feeInShares);
    }
  }

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L27

Vulnerability details

Impact

As asset and share are different things, the user may deposit the ASSET amount which is <= minDepositAmount IF ITS SHARE is > minDepositAmount.

Proof of Concept

See ERC4626-Cloned.sol

function deposit(uint256 assets, address receiver)
    public
    virtual
    returns (uint256 shares)
  {
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

    require(shares > minDepositAmount(), "VALUE_TOO_SMALL");
    // Need to transfer before minting or ERC777s could reenter.
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    emit Deposit(msg.sender, receiver, assets, shares);

    afterDeposit(assets, shares);
  }

In the deposit function above, the system checks if shares <= minDepositAmount(), if yes, revert the deposit

require(shares > minDepositAmount(), "VALUE_TOO_SMALL");

The system tries to compare shares against assets (Deposit amount). Assets and shares are related but they are completely different things. For example,

In a new empty vault, user A and user B make deposits

User A has 10 assets which are 10 shares User B has 20 assets which are 20 shares

Here, the assets and shares are the same.

However, if the system withdraws 6 assets from the vault for some purpose

Total tokens = 24, Total shares = 30,

In this case, the assets and shares are not the same

User A has 10 shares (which is 10 / 30 * 24 = 8 assets) User B has 20 shares (which is 20 / 30 * 24 = 16 assets)

Let say,

If user C deposits 24 assets, he will get 30 shares [24 / (24 + 8 + 16) * (10 + 20 + 30)]

With the current code,

If minDepositAmount() = 25, his deposit will pass because 30 > 25. This is wrong because the deposit should fail as 24 < 25. We should verify by assets (NOT shares)

Tools Used

Manual

Recommended Mitigation Steps

Change the following code

function deposit(uint256 assets, address receiver)
    public
    virtual
    returns (uint256 shares)
  {
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

    --- require(shares > minDepositAmount(), "VALUE_TOO_SMALL");
    +++ require(assets > minDepositAmount(), "VALUE_TOO_SMALL");
    // Need to transfer before minting or ERC777s could reenter.
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    emit Deposit(msg.sender, receiver, assets, shares);

    afterDeposit(assets, shares);
  }

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L611-L619

Vulnerability details

Impact

The vault could get liquidated at any time if the msg.sender is the borrower (Who is the owner of the collateral id) which is WRONG.

Proof of Concept

Right now, either the owner of the collateral id can liquidate the stack ANYTIME or everyone can liquidate the vault after the stack has ended which is wrong. By right, everyone can be the liquidator. Once the stack ended, the stack can be liquidated by anyone.

Tools Used

Manual

Recommended Mitigation Steps

 function canLiquidate(ILienToken.Stack memory stack)
    public
    view
    returns (bool)
  {
    RouterStorage storage s = _loadRouterSlot();
    --- return (stack.point.end <= block.timestamp || msg.sender == s.COLLATERAL_TOKEN.ownerOf(stack.lien.collateralId));
    +++ return stack.point.end <= block.timestamp;
  }

There are some typos across the contracts.

memoizing - should be memorizing
offerer - should be offerrer
calcualtion - should be calculation

See VaultImplentation.sol

modifier whenNotPaused() {
 if (ROUTER().paused()) {
   revert InvalidRequest(InvalidRequestReason.PAUSED);
 }

 if (_loadVISlot().isShutdown) {
   revert InvalidRequest(InvalidRequestReason.SHUTDOWN);
 }
 _;
}

Obviously, this modifier caters for shutdown as well. Therefore the modifier name should change from

whenNotPaused

to

whenNoPausedOrShutdown

See AstariaRouter.sol

function pullToken(
 address token,
 uint256 amount,
 address recipient
) public payable override {
 RouterStorage storage s = _loadRouterSlot();
 s.TRANSFER_PROXY.tokenTransferFrom(
   address(token),
   msg.sender,
   recipient,
   amount
 );
}

We should remove the payable keyword as there is no msg.value in this function and tokenTransferFrom function. See the tokenTransferFrom function of TransferProxy.sol below

  function tokenTransferFrom(
    address token,
    address from,
    address to,
    uint256 amount
  ) external requiresAuth {
    ERC20(token).safeTransferFrom(from, to, amount);
  }

As this is an override function, we need to remove the payable keyword of the same function signature in its parent contract which is ERC4626Router.sol so there won't be a compilation error.

See vaultImpelentation.sol

function modifyDepositCap(uint256 newCap) external {
 require(msg.sender == owner()); //owner is "strategist"
 _loadVISlot().depositCap = newCap.safeCastTo88();
}

This function Is the parent contract of PublicVault that allows strategists to modify the maximum deposit cap value at any time. However, it should consider if the total deposit amount for the current vault is greater than the deposit cap.

I would suggest the following changes

a. Remove

function modifyDepositCap(uint256 newCap) external;

from IVaultImplementation.sol

b. Add the deleted function signature to IPublicVault.sol

c. Add

  function modifyDepositCap(uint256 newCap) external {
    require(msg.sender == owner()); //owner is "strategist"
    require(newCap == 0 || newCap <= totalAssets());
    _loadVISlot().depositCap = newCap.safeCastTo88();
  }

to PublicVault.sol

[Note: newCap can be 0 as this means there is no limit. Also, the reason why we move the modifyDepositCap function from parent contract to child contract is that the totalAssets() only can be called in PublicVault.sol. It is fine to remove the access of the function in Vault.sol because private vault does not call the same function]

See settleLiquidatorNFTClaim function of ClearingHouse.sol

  function settleLiquidatorNFTClaim() external {
    IAstariaRouter ASTARIA_ROUTER = IAstariaRouter(_getArgAddress(0));

    require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));
    ClearingHouseStorage storage s = _getStorage();
    ASTARIA_ROUTER.LIEN_TOKEN().payDebtViaClearingHouse(
      address(0),
      COLLATERAL_ID(),
      0,
      s.auctionStack.stack
    );
  }

It calls payDebtViaClearingHouse function of Astaria router. See the payDebtViaClearingHouse function of AstariaRoter.sol below

  function payDebtViaClearingHouse(
    address token,
    uint256 collateralId,
    uint256 payment,
    AuctionStack[] memory auctionStack
  ) external {
    LienStorage storage s = _loadLienStorageSlot();
    require(
      msg.sender == address(s.COLLATERAL_TOKEN.getClearingHouse(collateralId))
    );

    _payDebt(s, token, payment, msg.sender, auctionStack);
    delete s.collateralStateHash[collateralId];
  }

The first parameter token = address(0) and the third parameter payment = 0, so basically settleLiquidatorNFTClaim function call tells the code to ignore this line

    _payDebt(s, token, payment, msg.sender, auctionStack); 

The logic is not wrong but it is ugly and it costs gas too (I don't want to RESUBMIT this in Gas report.)

What we can do is create another function in LienToken.sol

   function deleteCollateralStateHashViaClearingHouse(
    uint256 collateralId
  ) external {
    LienStorage storage s = _loadLienStorageSlot();
    require(
      msg.sender == address(s.COLLATERAL_TOKEN.getClearingHouse(collateralId))
    );
    delete s.collateralStateHash[collateralId];
  }

And change the settleLiquidatorNFTClaim function

  function settleLiquidatorNFTClaim() external {
    IAstariaRouter ASTARIA_ROUTER = IAstariaRouter(_getArgAddress(0));

    require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));
    ClearingHouseStorage storage s = _getStorage();
    --- ASTARIA_ROUTER.LIEN_TOKEN().payDebtViaClearingHouse(
    ---  address(0),
    ---  COLLATERAL_ID(),
    ---  0,
    ---  s.auctionStack.stack
    --- );

    +++ ASTARIA_ROUTER.LIEN_TOKEN().deleteCollateralStateHashViaClearingHouse(
    +++  COLLATERAL_ID()
    +++);
  }

And also add a new function signature to ILienToken.sol

  function deleteCollateralStateHashViaClearingHouse(
    uint256 collateralId
  ) external;

See PublicVault.sol

  function minDepositAmount()
    public
    view
    virtual
    override(ERC4626Cloned)
    returns (uint256)
  {
    if (ERC20(asset()).decimals() == uint8(18)) {
      return 100 gwei;
    } else {
      return 10**(ERC20(asset()).decimals() - 1);
    }
  }

The code above handles assets with 18 decimal places differently compare to the one with lower decimal places. It assumes 18 decimal places is the smallest decimal place of an ERC20 token which is incorrect. We can have 19, 20, or any erc20 contract which is larger than 18. Therefore, we should change the code to handle those numbers greater than 18 too.

Change

  function minDepositAmount()
    public
    view
    virtual
    override(ERC4626Cloned)
    returns (uint256)
  {
   ---  if (ERC20(asset()).decimals() == uint8(18)) {
   +++ if (ERC20(asset()).decimals() >= uint8(18)) {
      return 100 gwei;
    } else {
      return 10**(ERC20(asset()).decimals() - 1);
    }
  }

See CollateralToken.sol

  function releaseToAddress(uint256 collateralId, address releaseTo)
    public
    releaseCheck(collateralId)
    onlyOwner(collateralId)
  {
    CollateralStorage storage s = _loadCollateralSlot();

    if (msg.sender != ownerOf(collateralId)) {
      revert InvalidSender();
    }
    Asset memory underlying = s.idToUnderlying[collateralId];
    address tokenContract = underlying.tokenContract;
    _burn(collateralId);
    delete s.idToUnderlying[collateralId];
    _releaseToAddress(s, underlying, collateralId, releaseTo);
  }

We should not check if the collateralId belongs to the msg.sender again as it is checked in the onlyOwner modifier (See below)

  modifier onlyOwner(uint256 collateralId) {
    require(ownerOf(collateralId) == msg.sender);
    _;
  }

Change the following function by remove certain lines

  function releaseToAddress(uint256 collateralId, address releaseTo)
    public
    releaseCheck(collateralId)
    onlyOwner(collateralId)
  {
    CollateralStorage storage s = _loadCollateralSlot();

    --- if (msg.sender != ownerOf(collateralId)) {
    ---  revert InvalidSender();
    ---}
    Asset memory underlying = s.idToUnderlying[collateralId];
    address tokenContract = underlying.tokenContract;
    _burn(collateralId);
    delete s.idToUnderlying[collateralId];
    _releaseToAddress(s, underlying, collateralId, releaseTo);
  }

There is additional casting in the following code

payable(address(s.clearingHouse[collateralId]))

of

_generateValidOrderParameters function of CollateralToken.sol

Change

--- payable(address(s.clearingHouse[collateralId]))
+++ payable(s.clearingHouse[collateralId])

See PublicVault.sol

return string(abi.encodePacked("AST-Vault-", ERC20(asset()).symbol())); return string(abi.encodePacked("AST-V-", ERC20(asset()).symbol()));

See Vault.sol

return string(abi.encodePacked("AST-Vault-", ERC20(asset()).symbol())); return string(abi.encodePacked("AST-V", owner(), "-", ERC20(asset()).symbol()));

It is not recommended to encodePacked two strings as strings because string is a dynamic type variable

See the following comment at https://docs.soliditylang.org/en/v0.8.11/abi-spec.html

If you use keccak256(abi.encodePacked(a, b)) and both a and b are dynamic types, it is easy to craft collisions in the hash value by moving parts of a into b and vice-versa. More specifically, abi.encodePacked("a", "bc") == abi.encodePacked("ab", "c"). If you use abi.encodePacked for signatures, authentication or data integrity, make sure to always use the same types and check that at most one of them is dynamic. Unless there is a compelling reason, abi.encode should be preferred.

Change

abi.encodePacked to abi.encode

I modify the existing test unit of AstariaTest.sol to apply for a loan with zero Ether and repay with zero Ether and the test passed.

function testBasicPublicVaultLoan() 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,
      +++ amount: 0 ether,
      isFirstLien: true
    });

    uint256 collateralId = tokenContract.computeId(tokenId);

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

    vm.warp(block.timestamp + 9 days);

    --- _repay(stack, 0, 10 ether, address(this));
    +++ _repay(stack, 0, 0 ether, address(this));
  }

It does not make sense to borrow or repay 0 ether. I would suggest adding the zero amount checking to

_createLien and _payment internal function of LienTokens.sol

function _createLien(
    LienStorage storage s,
    ILienToken.LienActionEncumber memory params
  ) internal returns (uint256 newLienId, ILienToken.Stack memory newSlot) {
    if (s.collateralStateHash[params.lien.collateralId] == ACTIVE_AUCTION) {
      revert InvalidState(InvalidStates.COLLATERAL_AUCTION);
    }
    if (
      params.lien.details.liquidationInitialAsk < params.amount ||
      params.lien.details.liquidationInitialAsk == 0
    ) {
      revert InvalidState(InvalidStates.INVALID_LIQUIDATION_INITIAL_ASK);
    }

    if (params.stack.length > 0) {
      if (params.lien.collateralId != params.stack[0].lien.collateralId) {
        revert InvalidState(InvalidStates.COLLATERAL_MISMATCH);
      }

      if (params.lien.token != params.stack[0].lien.token) {
        revert InvalidState(InvalidStates.ASSET_MISMATCH);
      }
    }

    +++ require(params.amount > 0, "ZERO amount");

    newLienId = uint256(keccak256(abi.encode(params.lien)));
    Point memory point = Point({
      lienId: newLienId,
      amount: params.amount.safeCastTo88(),
      last: block.timestamp.safeCastTo40(),
      end: (block.timestamp + params.lien.details.duration).safeCastTo40()
    });
    _mint(params.receiver, newLienId);
    return (newLienId, Stack({lien: params.lien, point: point}));
  }

function _payment(
    LienStorage storage s,
    Stack[] memory activeStack,
    uint8 position,
    uint256 amount,
    address payer
  ) internal returns (Stack[] memory, uint256) {
    Stack memory stack = activeStack[position];
    uint256 lienId = stack.point.lienId;

    +++ require(amount > 0, "zero Amount");

    if (s.lienMeta[lienId].atLiquidation) {
      revert InvalidState(InvalidStates.COLLATERAL_AUCTION);
    }
    uint64 end = stack.point.end;
    // Blocking off payments for a lien that has exceeded the lien.end to prevent repayment unless the msg.sender() is the AuctionHouse
    if (block.timestamp >= end) {
      revert InvalidLoanState();
    }
    uint256 owed = _getOwed(stack, block.timestamp);
    address lienOwner = ownerOf(lienId);
    bool isPublicVault = _isPublicVault(s, lienOwner);

    address payee = _getPayee(s, lienId);

    if (amount > owed) amount = owed;
    if (isPublicVault) {
      IPublicVault(lienOwner).beforePayment(
        IPublicVault.BeforePaymentParams({
          interestOwed: owed - stack.point.amount,
          amount: stack.point.amount,
          lienSlope: calculateSlope(stack)
        })
      );
    }

    //bring the point up to block.timestamp, compute the owed
    stack.point.amount = owed.safeCastTo88();
    stack.point.last = block.timestamp.safeCastTo40();

    if (stack.point.amount > amount) {
      stack.point.amount -= amount.safeCastTo88();
      //      // slope does not need to be updated if paying off the rest, since we neutralize slope in beforePayment()
      if (isPublicVault) {
        IPublicVault(lienOwner).afterPayment(calculateSlope(stack));
      }
    } else {
      amount = stack.point.amount;
      if (isPublicVault) {
        // since the openLiens count is only positive when there are liens that haven't been paid off
        // that should be liquidated, this lien should not be counted anymore
        IPublicVault(lienOwner).decreaseEpochLienCount(
          IPublicVault(lienOwner).getLienEpoch(end)
        );
      }
      delete s.lienMeta[lienId]; //full delete of point data for the lien
      _burn(lienId);
      activeStack = _removeStackPosition(activeStack, position);
    }

    s.TRANSFER_PROXY.tokenTransferFrom(stack.lien.token, payer, payee, amount);

    emit Payment(lienId, amount);
    return (activeStack, amount);
  }

Duplicate public vaults and private vaults can be created if they have the same

address(this), vaultType, msg.sender, underlying, block.timestamp, epochLength, vaultFee

See the _newVault function of AstariaRouter.sol

 function _newVault(
    RouterStorage storage s,
    address underlying,
    uint256 epochLength,
    address delegate,
    uint256 vaultFee,
    bool allowListEnabled,
    address[] memory allowList,
    uint256 depositCap
  ) internal returns (address vaultAddr) {
    uint8 vaultType;

    if (epochLength > uint256(0)) {
      vaultType = uint8(ImplementationType.PublicVault);
    } else {
      vaultType = uint8(ImplementationType.PrivateVault);
    }

    //immutable data
    vaultAddr = ClonesWithImmutableArgs.clone(
      s.BEACON_PROXY_IMPLEMENTATION,
      abi.encodePacked(
        address(this),
        vaultType,
        msg.sender,
        underlying,
        block.timestamp,
        epochLength,
        vaultFee
      )
    );

    //mutable data
    IVaultImplementation(vaultAddr).init(
      IVaultImplementation.InitParams({
        delegate: delegate,
        allowListEnabled: allowListEnabled,
        allowList: allowList,
        depositCap: depositCap
      })
    );

    s.vaults[vaultAddr] = true;

    emit NewVault(msg.sender, delegate, vaultAddr, vaultType);

    return vaultAddr;
  }

We don't want this because other loan activities can reuse the same object. And

vaultAddr = ClonesWithImmutableArgs.clone(
      s.BEACON_PROXY_IMPLEMENTATION,
      abi.encodePacked(
        address(this),
        vaultType,
        msg.sender,
        underlying,
        block.timestamp,
        epochLength,
        vaultFee
      )
    );

This code still creates different vault Address for the vault objects with the same parameter.

To fix this, add

 mapping(bytes => bool) vaultsSignature

to IAstariaRouter.sol

Then, change the _newVault function

function _newVault(
    RouterStorage storage s,
    address underlying,
    uint256 epochLength,
    address delegate,
    uint256 vaultFee,
    bool allowListEnabled,
    address[] memory allowList,
    uint256 depositCap
  ) internal returns (address vaultAddr) {
    uint8 vaultType;

    if (epochLength > uint256(0)) {
      vaultType = uint8(ImplementationType.PublicVault);
    } else {
      vaultType = uint8(ImplementationType.PrivateVault);
    }

    +++ require(
    +++ s.vaultsSignature[
    +++ abi.encodePacked(
    +++ address(this),
    +++ vaultType,
    +++ msg.sender,
    +++ underlying,
    +++ block.timestamp,
    +++ epochLength,
    +++ vaultFee
    +++ )
    +++ ], "Vault exists");

    //immutable data
    vaultAddr = ClonesWithImmutableArgs.clone(
      s.BEACON_PROXY_IMPLEMENTATION,
      abi.encodePacked(
        address(this),
        vaultType,
        msg.sender,
        underlying,
        block.timestamp,
        epochLength,
        vaultFee
      )
    );

    //mutable data
    IVaultImplementation(vaultAddr).init(
      IVaultImplementation.InitParams({
        delegate: delegate,
        allowListEnabled: allowListEnabled,
        allowList: allowList,
        depositCap: depositCap
      })
    );

    s.vaults[vaultAddr] = true;
    +++ s.vaultsSignature[
    +++ abi.encodePacked(
    +++ address(this),
    +++ vaultType,
    +++ msg.sender,
    +++ underlying,
    +++ block.timestamp,
    +++ epochLength,
    +++ vaultFee
    +++ )
    +++ ] = true;


    emit NewVault(msg.sender, delegate, vaultAddr, vaultType);

    return vaultAddr;
  }

See AstariaRouter.sol

Change

s.minInterestBPS = uint32((uint256(1e15) * 5) / (365 days)); s.maxInterestRate = ((uint256(1e16) * 200) / (365 days)).safeCastTo88();

to

s.minInterestBPS = uint32(158548959); // (uint256(1e15) * 5) / (365 days) s.maxInterestRate = (63419583967).safeCastTo88(); // ((uint256(1e16) * 200) / (365 days)

Save execution gas around 433 for every line

See collateralToken.sol

Change

uint256(uint160(settlementToken))

to

uint256(settlementToken)

Save execution gas around 6