Astaria contest - rvierdiiev's results

Lines of code

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

Vulnerability details

Impact

If PublicVault buyouts lien then yIntercept should be decreased by paid fees, because buyout amount is always bigger than new lien amount. Vault can be insolvent.

Proof of Concept

Any Vault can buyout lien if he provides better loan terms. To do that anyone can call VaultImplementation.buyoutLien function. https://github.com/code-423n4/2023-01-astaria/blob/main/src/VaultImplementation.sol#L313-L351

  function buyoutLien(
    ILienToken.Stack[] calldata stack,
    uint8 position,
    IAstariaRouter.Commitment calldata incomingTerms
  )
    external
    whenNotPaused
    returns (ILienToken.Stack[] memory, ILienToken.Stack memory)
  {
    LienToken lienToken = LienToken(address(ROUTER().LIEN_TOKEN()));


    (uint256 owed, uint256 buyout) = lienToken.getBuyout(stack[position]);


    if (buyout > ERC20(asset()).balanceOf(address(this))) {
      revert IVaultImplementation.InvalidRequest(
        InvalidRequestReason.INSUFFICIENT_FUNDS
      );
    }


    _validateCommitment(incomingTerms, recipient());


    ERC20(asset()).safeApprove(address(ROUTER().TRANSFER_PROXY()), buyout);


    return
      lienToken.buyoutLien(
        ILienToken.LienActionBuyout({
          position: position,
          encumber: ILienToken.LienActionEncumber({
            amount: owed,
            receiver: recipient(),
            lien: ROUTER().validateCommitment({
              commitment: incomingTerms,
              timeToSecondEpochEnd: _timeToSecondEndIfPublic()
            }),
            stack: stack
          })
        })
      );
  }

This function will get owed and buyout amount and will approve that buyout amount to transfer proxy and will set owed amount as the amount of new lien. Then LienToken.buyoutLien is called. Inside LienToken.__buyoutLien function new lien will be created for the Vault that bought lien and old lien will be removed from old Vault. In case if old Vault is PublicVault then slope and yIntercept will be updated for it. This all sets old Vault in correct state.

But in case if new Vault is PublicVault as well, then not everything is settled for it.

Let's check what is owed and buyout amount. https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L585-L594

  function _getBuyout(LienStorage storage s, Stack calldata stack)
    internal
    view
    returns (uint256 owed, uint256 buyout)
  {
    owed = _getOwed(stack, block.timestamp);
    buyout =
      owed +
      s.ASTARIA_ROUTER.getBuyoutFee(_getRemainingInterest(s, stack));
  }

As you can see owed is loan amount + already accrued interests by buyout moment, when buyout is owed + some percent of remaning interests. So if loan is 10 weth and by time of buyout accrued interests are 0.2 weth and remaining interests is 0.3 weth and buyout fee is 10% that means that new PublicVault paid 10.23 weth to old Vault. But new loan is created with amount 10.2 weth instead of 10.23.

Once new lien is created then PublicVault._afterCommitToLien is called for it which will update slope and increase liens for epoch. The slope will be updated to receive interests from user. And when this interests will be paid, the yIntercept will be increased as well. So at the end of loan user will pay 10.2 weth + interests. In case if user repaid this loan exactly after buyout, then he will pay just 10.2 weth, however PublicVault just paid 10.23 for it.

That means that PublicVault has paid 0.03 weth that he will not return and his yIntercept should be decreased by this difference.

Tools Used

VsCode

Recommended Mitigation Steps

I think that you need to set new lien amount to buyout amount instead of owed amount. Then you don't need to update yIntercept for new PublicVault.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L389-L422 https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L341

Vulnerability details

Impact

If lien is created with liquidationInitialAsk > uint88.max then it will be not possible to start liquidation, because _stopLiens function will revert while casting.

Proof of Concept

When user creates new lien then _createLien function is called which will check that params.lien.details.liquidationInitialAsk >= params.amount.

Later it will create lien and will set provided values. Amount will be casted to uint88, but params.lien.details.liquidationInitialAsk will be set as it is. Pls, note that params.lien.details.liquidationInitialAsk is uint256 value.

In case if user didn't pay his debt, then it's possible to liquidate it. This will call _stopLiens function. Later that function will cast lien.details.liquidationInitialAsk param to uint88 and if it exceeds max, then function will revert. As result it will be not possible to liquidate debt.

Scenario. 1.User open lien with amount < uint88.max and liquidationInitialAsk >uint88.max. This will succeed and user will be able to get loan. 2.User didn't pay his debt. 3.Someone wants to liquidate debt, but because of casting function will always revert. As result the nft will be locked and noone will be able to sell it.

Tools Used

VsCode

Recommended Mitigation Steps

Check that liquidationInitialAsk is less than uint88.max when createLien.

Lines of code

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

Vulnerability details

Impact

PublicVault.processEpoch calculates withdrawReserve incorrectly. As result user can receive less funds when totalAssets() <= expected from auction.

Proof of Concept

When users wants to withdraw from PublicVault then WithdrawProxy is deployed and PublicVault.processEpoch function is responsible to calculate s.withdrawReserve. This amount depends on how many shares should be redeemed and if there is auction for the epoch.

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

solidity
  function processEpoch() public {
    // check to make sure epoch is over
    if (timeToEpochEnd() > 0) {
      revert InvalidState(InvalidStates.EPOCH_NOT_OVER);
    }
    VaultData storage s = _loadStorageSlot();


    if (s.withdrawReserve > 0) {
      revert InvalidState(InvalidStates.WITHDRAW_RESERVE_NOT_ZERO);
    }


    WithdrawProxy currentWithdrawProxy = WithdrawProxy(
      s.epochData[s.currentEpoch].withdrawProxy
    );


    // split funds from previous WithdrawProxy with PublicVault if hasn't been already
    if (s.currentEpoch != 0) {
      WithdrawProxy previousWithdrawProxy = WithdrawProxy(
        s.epochData[s.currentEpoch - 1].withdrawProxy
      );
      if (
        address(previousWithdrawProxy) != address(0) &&
        previousWithdrawProxy.getFinalAuctionEnd() != 0
      ) {
        previousWithdrawProxy.claim();
      }
    }


    if (s.epochData[s.currentEpoch].liensOpenForEpoch > 0) {
      revert InvalidState(InvalidStates.LIENS_OPEN_FOR_EPOCH_NOT_ZERO);
    }


    // reset liquidationWithdrawRatio to prepare for re calcualtion
    s.liquidationWithdrawRatio = 0;


    // check if there are LPs withdrawing this epoch
    if ((address(currentWithdrawProxy) != address(0))) {
      uint256 proxySupply = currentWithdrawProxy.totalSupply();


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


      currentWithdrawProxy.setWithdrawRatio(s.liquidationWithdrawRatio);
      uint256 expected = currentWithdrawProxy.getExpected();


      unchecked {
        if (totalAssets() > expected) {
          s.withdrawReserve = (totalAssets() - expected)
            .mulWadDown(s.liquidationWithdrawRatio)
            .safeCastTo88();
        } else {
          s.withdrawReserve = 0;
        }
      }
      _setYIntercept(
        s,
        s.yIntercept -
          totalAssets().mulDivDown(s.liquidationWithdrawRatio, 1e18)
      );
      // burn the tokens of the LPs withdrawing
      _burn(address(this), proxySupply);
    }


    // increment epoch
    unchecked {
      s.currentEpoch++;
    }
  }

s.liquidationWithdrawRatio depends on how many shares exists inside WithdrawProxy. In case if amount of shares inside WithdrawProxy equal to amount of shares inside PublicVault that means that withdraw ratio is 100% and all funds from Vault should be sent to WithdrawProxy.

In case if auction is in progress then WithdrawProxy.getExpected is not 0 and some amount of funds is expected from auction.

unchecked {
        if (totalAssets() > expected) {
          s.withdrawReserve = (totalAssets() - expected)
            .mulWadDown(s.liquidationWithdrawRatio)
            .safeCastTo88();
        } else {
          s.withdrawReserve = 0;
        }
      }

This is s.withdrawReserve calculation. As you can see in case if totalAssets() <= expected then s.withdrawReserve is set to 0 and no funds will be sent to proxy. This is incorrect though.

For example in the case when withdraw ratio is 100% all funds should be sent to the withdraw proxy, but because of that check, some part of funds will be still inside the vault and depositors will lose their funds. If for example totalAssets is 5eth and expected is 5 eth, then depositors will lose all 5 eth.

This check is done in such way, because of calculations inside WithdrawProxy. But it's not correct.

Tools Used

VsCode

Recommended Mitigation Steps

You need to check this logic again. Maybe you need to always send s.withdrawReserve = totalAssets().mulWadDown(s.liquidationWithdrawRatio).safeCastTo88() amount to the withdraw proxy. But then you need rethink, how WithdrawProxy will handle yIntercept increase/decrease.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L314-L316 https://github.com/code-423n4/2023-01-astaria/blob/main/src/WithdrawProxy.sol#L271-L274

Vulnerability details

Impact

WithdrawProxy calculates withdraw amount for users incorrectly as it uses different base in calculations.

Proof of Concept

When WithdrawProxy is deployed then wihdraw ratio is calculated for it. https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L311-L329

    if ((address(currentWithdrawProxy) != address(0))) {
      uint256 proxySupply = currentWithdrawProxy.totalSupply();


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


      currentWithdrawProxy.setWithdrawRatio(s.liquidationWithdrawRatio);
      uint256 expected = currentWithdrawProxy.getExpected();


      unchecked {
        if (totalAssets() > expected) {
          s.withdrawReserve = (totalAssets() - expected)
            .mulWadDown(s.liquidationWithdrawRatio)
            .safeCastTo88();
        } else {
          s.withdrawReserve = 0;
        }
      }

Here e18 base is used to get ratio. And then this ratio is set to WithdrawProxy.

Later, if liquidation was set to the epoch, PublicVault can call claim function to receive some part of auction funds. https://github.com/code-423n4/2023-01-astaria/blob/main/src/WithdrawProxy.sol#L240-L287

  function claim() public {
    WPStorage storage s = _loadSlot();


    if (s.finalAuctionEnd == 0) {
      revert InvalidState(InvalidStates.CANT_CLAIM);
    }


    if (PublicVault(VAULT()).getCurrentEpoch() < CLAIMABLE_EPOCH()) {
      revert InvalidState(InvalidStates.PROCESS_EPOCH_NOT_COMPLETE);
    }
    if (block.timestamp < s.finalAuctionEnd) {
      revert InvalidState(InvalidStates.FINAL_AUCTION_NOT_OVER);
    }


    uint256 transferAmount = 0;
    uint256 balance = ERC20(asset()).balanceOf(address(this)) -
      s.withdrawReserveReceived; // will never underflow because withdrawReserveReceived is always increased by the transfer amount from the PublicVault


    if (balance < s.expected) {
      PublicVault(VAULT()).decreaseYIntercept(
        (s.expected - balance).mulWadDown(1e18 - s.withdrawRatio)
      );
    } else {
      PublicVault(VAULT()).increaseYIntercept(
        (balance - s.expected).mulWadDown(1e18 - s.withdrawRatio)
      );
    }


    if (s.withdrawRatio == uint256(0)) {
      ERC20(asset()).safeTransfer(VAULT(), balance);
    } else {
      transferAmount = uint256(s.withdrawRatio).mulDivDown(
        balance,
        10**ERC20(asset()).decimals()
      );


      unchecked {
        balance -= transferAmount;
      }


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


    emit Claimed(address(this), transferAmount, VAULT(), balance);
  }

As you can see, when function calculates how it shoud increase/decrease yIntercept is uses correct base(e18) to work with s.withdrawRatio.

    if (balance < s.expected) {
      PublicVault(VAULT()).decreaseYIntercept(
        (s.expected - balance).mulWadDown(1e18 - s.withdrawRatio)
      );
    } else {
      PublicVault(VAULT()).increaseYIntercept(
        (balance - s.expected).mulWadDown(1e18 - s.withdrawRatio)
      );
    }

But later, when it calculates amount that shoud be withdrawn by depositors it uses 10**ERC20(asset()).decimals() base, which is not correct in case when asset decimals is not 18.

    if (s.withdrawRatio == uint256(0)) {
      ERC20(asset()).safeTransfer(VAULT(), balance);
    } else {
      transferAmount = uint256(s.withdrawRatio).mulDivDown(
        balance,
        10**ERC20(asset()).decimals()
      );


      unchecked {
        balance -= transferAmount;
      }


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

As result, the amount of funds that users should receive is calculated incorrectly and depending on asset decimals it can be bigger or less than expected. This leads to lost of funds to the PublicVault or WithdrawProxy shares holders.

Tools Used

VsCode

Recommended Mitigation Steps

Use this code.

    if (s.withdrawRatio == uint256(0)) {
      ERC20(asset()).safeTransfer(VAULT(), balance);
    } else {
      transferAmount = uint256(s.withdrawRatio).mulDivDown(
        balance,
        1e18
      );


      unchecked {
        balance -= transferAmount;
      }


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

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L53 https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L15-L25 https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L41-L52

Vulnerability details

Impact

ERC4626RouterBase.mint and ERC4626RouterBase.withdraw approves incorrect amount of tokens to Vault. As result in case when share price != base asset price then function will be reverting.

Proof of Concept

AstariaRouter extends ERC4626RouterBase contract so it has ERC4626RouterBase.mint and ERC4626RouterBase.withdraw functions inherited from ERC4626RouterBase.

ERC4626RouterBase.mint function is designed to allow depositor to mint some shares amount of shares in Vault contract. https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L15-L25

  function mint(
    IERC4626 vault,
    address to,
    uint256 shares,
    uint256 maxAmountIn
  ) public payable virtual override returns (uint256 amountIn) {
    ERC20(vault.asset()).safeApprove(address(vault), shares);
    if ((amountIn = vault.mint(shares, to)) > maxAmountIn) {
      revert MaxAmountError();
    }
  }

As you can see user should send funds before the mint call to AstariaRouter and then AstariaRouter will approve those funds for the Vault and will call vault.mint, that will mint provided amount of shares.

The problem here is that when you want to mint shares, you need to provide some assets amount which can be not the same as shares amount. For example 1 share can cost 0.5 base tokens. So in this case AstariaRouter should approve not shares amount of base tokens, but it should calculate the needed amount of base tokens to provide to mint function.

Let's consider 2 cases. First one, when share price is bigger than base token price. share costs 2 base tokens. 1.Suppose that user called ERC4626RouterBase.mint function with shares amount == 100. He transferred 200 base tokens to receive 100 shares. 2.ERC4626RouterBase.mint approves vault with 100 base tokens. 3.Because it's not possible to mint 100 shares with 100 base tokens, vault.mint will revert.

And the second case when share price is less than base token price. share costs 0.5 base tokens. 1.Suppose that user called ERC4626RouterBase.mint function with shares amount == 100. He transferred 50 base tokens to receive 100 shares. 2.ERC4626RouterBase.mint approves vault with 100 base tokens. 3.ERC4626RouterBase.mint reverts because user sent only 50 base tokens, not 100.

Tools Used

VsCode

Recommended Mitigation Steps

Calculate the amount of base tokens that needs to be approved.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L359-L411 https://github.com/code-423n4/2023-01-astaria/blob/main/src/WithdrawProxy.sol#L192

Vulnerability details

Impact

User can lose funds if he withdraws from WithdrawProxy before all funds are sent.

Proof of Concept

When WithdrawProxy is deployed for the epoch then it mints some amount of shares for user.

Then user can redeem his shares from WithdrawProxy using redeem function. This function will not allow user to redeem only when auction is in progress.

Once WithdrawProxy is deployed it doesn't contain any funds. It will be funded when PublicVault.transferWithdrawReserve function will be called. https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L371-L389

    if (currentWithdrawProxy != address(0)) {
      uint256 withdrawBalance = ERC20(asset()).balanceOf(address(this));


      // prevent transfer of more assets then are available
      if (s.withdrawReserve <= withdrawBalance) {
        withdrawBalance = s.withdrawReserve;
        s.withdrawReserve = 0;
      } else {
        unchecked {
          s.withdrawReserve -= withdrawBalance.safeCastTo88();
        }
      }


      ERC20(asset()).safeTransfer(currentWithdrawProxy, withdrawBalance);
      WithdrawProxy(currentWithdrawProxy).increaseWithdrawReserveReceived(
        withdrawBalance
      );
      emit WithdrawReserveTransferred(withdrawBalance);
    }

This part of function sends funds to the proxy. And it's possible that not enough assets at the moment, so it will send less amount, then needed to withdraw proxy. Later, when the vault will have more assets it will be possible to call function again and it will send the rest to proxy.

However there is no way for user to know that all funds are already sent to withdraw proxy and he can redeem before all funds are sent. As result he will burn his shares and will receive less amount of assets.

Also it's possible for attacker to transfer some small amount of assets directly to newly deployed WithdrawProxy and expect that someone will call redeem before withdraw proxy will be funded by vault and burn his shares. If attacker is going to withdraw in this epoch as well this will increase his total assets amount.

Tools Used

VsCode

Recommended Mitigation Steps

Some mechanism should be considered to let users know that withdraw proxy funding has finished.

Lines of code

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

Vulnerability details

Impact

Approved operator of collateral owner can't liquidate lien

Proof of Concept

If someone wants to liquidate lien then canLiquidate function is called to check if it's possible. https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L611-L619

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

As you can see owner of collateral token can liquidate lien in any moment. However approved operators of owner can't do that, however they should.

As while validating commitment it's allowed for approved operator to request a loan. That means that owner of collateral token can approve some operators to allow them to work with their debts. So they should be able to liquidate loan as well.

Tools Used

VsCode

Recommended Mitigation Steps

Add ability for approved operators to liqiudate lien.

Lines of code

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

Vulnerability details

Impact

PublicVault decimals is 18, however it is actually same as asset.decimals. This will create problems for all integrations as they will be using PublicVault.decimals for calculations.

Proof of Concept

Currently, all PublicVaults not depending on asset have 18 decimals. However, there is no any convertions between base asset decimals and 18 decimals in the code. That means that actuall all PublicVaults use same decimals as their base asset.

This can create problems when another protocol will start integrating with Astaria. This incorrect decimals() function can break their calculations. For example if USDC PublicVault is used and some protocol sees that user has 110^6 assets inside Vault they may calculate this as 110^18 shares, however user has only 1*10^6 shares inside vault.

Tools Used

VsCode

Recommended Mitigation Steps

Change decimals to assets.decimals value.

Lines of code

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

Vulnerability details

Impact

PublicVault.processEpoch updates YIntercept incorrectly when totalAssets() <= expected.

Proof of Concept

When processEpoch is called it calculates amount of withdrawReserve that will be sent to the withdraw proxy. Later it updates yIntercept variable. https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L275-L337

  function processEpoch() public {
    // check to make sure epoch is over
    if (timeToEpochEnd() > 0) {
      revert InvalidState(InvalidStates.EPOCH_NOT_OVER);
    }
    VaultData storage s = _loadStorageSlot();


    if (s.withdrawReserve > 0) {
      revert InvalidState(InvalidStates.WITHDRAW_RESERVE_NOT_ZERO);
    }


    WithdrawProxy currentWithdrawProxy = WithdrawProxy(
      s.epochData[s.currentEpoch].withdrawProxy
    );


    // split funds from previous WithdrawProxy with PublicVault if hasn't been already
    if (s.currentEpoch != 0) {
      WithdrawProxy previousWithdrawProxy = WithdrawProxy(
        s.epochData[s.currentEpoch - 1].withdrawProxy
      );
      if (
        address(previousWithdrawProxy) != address(0) &&
        previousWithdrawProxy.getFinalAuctionEnd() != 0
      ) {
        previousWithdrawProxy.claim();
      }
    }


    if (s.epochData[s.currentEpoch].liensOpenForEpoch > 0) {
      revert InvalidState(InvalidStates.LIENS_OPEN_FOR_EPOCH_NOT_ZERO);
    }


    // reset liquidationWithdrawRatio to prepare for re calcualtion
    s.liquidationWithdrawRatio = 0;


    // check if there are LPs withdrawing this epoch
    if ((address(currentWithdrawProxy) != address(0))) {
      uint256 proxySupply = currentWithdrawProxy.totalSupply();


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


      currentWithdrawProxy.setWithdrawRatio(s.liquidationWithdrawRatio);
      uint256 expected = currentWithdrawProxy.getExpected();


      unchecked {
        if (totalAssets() > expected) {
          s.withdrawReserve = (totalAssets() - expected)
            .mulWadDown(s.liquidationWithdrawRatio)
            .safeCastTo88();
        } else {
          s.withdrawReserve = 0;
        }
      }
      _setYIntercept(
        s,
        s.yIntercept -
          totalAssets().mulDivDown(s.liquidationWithdrawRatio, 1e18)
      );
      // burn the tokens of the LPs withdrawing
      _burn(address(this), proxySupply);
    }

The part that we need to investigate is this.

unchecked {
        if (totalAssets() > expected) {
          s.withdrawReserve = (totalAssets() - expected)
            .mulWadDown(s.liquidationWithdrawRatio)
            .safeCastTo88();
        } else {
          s.withdrawReserve = 0;
        }
      }
      _setYIntercept(
        s,
        s.yIntercept -
          totalAssets().mulDivDown(s.liquidationWithdrawRatio, 1e18)
      );

In case if totalAssets() > expected then withdrawReserve is totalAssets() - expected multiplied by liquidationWithdrawRatio. That means that withdrawReserve amount will be sent of public vault to the withdraw proxy, so total assets should decrease by this amount. In this case call of _setYIntercept bellow is correct.

However in case when totalAssets() <= expected then withdrawReserve is set to 0, that means that nothing will be sent to the withdraw proxy. But _setYIntercept is still called in this case and total assets is decreased, but should not.

Tools Used

VsCode

Recommended Mitigation Steps

In case when totalAssets() <= expected do not call _setYIntercept.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L342 https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L169-L178 https://github.com/code-423n4/2023-01-astaria/blob/main/src/CollateralToken.sol#L109-L143

Vulnerability details

Impact

Liquidator can lose nft if auction has ended and noone bought it as someone can still buy nft using ClearingHouse.safeTransferFrom

Proof of Concept

When liquidator liquidates lien, then collateral nft is going to be selled on sea port auction during some auction window time. The minimum price for the auction is set to 1000 wei.

When someone will buy nft through the sea port, then ClearingHouse.safeTransferFrom function will be called. Liquidator will receive some funds to cover tx costs. Also, you should note that ClearingHouse.safeTransferFrom function doesn't have any restriction and can be called by anyone, not only trough sea port.

In case if noone bought nft, then this nft is considered to belong to liquidator and he can claim it using CollateralToken.liquidatorNFTClaim function. However ClearingHouse.safeTransferFrom can be called at any time, even when auction has already finished. So liquidator can be frontrunned and someone will buy nft, that is claimable by liquidator for 1000 wei. As result, liquidator lost nft and because the price for it was 1000 wei, he received super small amount of compensation that will not compensate tx costs.

Tools Used

VsCode

Recommended Mitigation Steps

Do not allow someone to buy nft when auction has finished.

Lines of code

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

Vulnerability details

Impact

LienToken._payment function increases users debt. Every time when user pays not whole lien amount then interests are added to the loan amount, so next time user pays interests based on interests.

Proof of Concept

LienToken._payment is used by LienToken.makePayment function that allows borrower to repay part or all his debt. https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L790-L853

  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;


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

The main problem is in line 826. stack.point.amount = owed.safeCastTo88(); https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L826

Here stack.point.amount becomes stack.point.amount + accrued interests, because owed is loan amount + accrued interests by this time.

stack.point.amount is the amount that user borrowed. So actually that line has just increased user's debt. And in case if he didn't pay all amount of lien, then next time he will pay more interests, because interests depend on loan amount.

In the docs Astaria protocol claims that:

All rates on the Astaria protocol are in simple interest and non-compounding.

https://docs.astaria.xyz/docs/protocol-mechanics/loanterms You can check that inside "Interest rate" section.

However _payment function is compounding interests. To avoid this, another field interestsAccrued should be introduced which will track already accrued interests.

Tools Used

VsCode

Recommended Mitigation Steps

You need store accrued interests by the moment of repayment to another variable interestsAccrued. And calculate _getInterest functon like this.

function _getInterest(Stack memory stack, uint256 timestamp)
    internal
    pure
    returns (uint256)
  {
    uint256 delta_t = timestamp - stack.point.last;

    return stack.point.interestsAccrued + (delta_t * stack.lien.details.rate).mulWadDown(stack.point.amount);
  }