Astaria contest - chaduke's results

Judge has assessed an item in Issue #148 as 3 risk. The relevant finding follows:

Lines of code https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L122-L231

Vulnerability details Impact Detailed description of the impact of this finding. _buyoutLien() in LienToken.sol failes to update the new PublicVault's slope, yIntercept, and s.epochData[...].liensOpenForEpoch.

Proof of Concept Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. Suppose a PublicVault P calls _buyoutLien() to replace an old lien A by a new lien A`. The slope, yIntercept and s.epochData[...].liensOpenForEpoch for A's payee have been modified as follows, but the function fails to update these parameters for P, the payee of A'.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. _buyoutLien() in LienToken.sol failes to update the new PublicVault's slope, yIntercept, and s.epochData[...].liensOpenForEpoch.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. Suppose a PublicVault P calls _buyoutLien() to replace an old lien A by a new lien A`. The slope, yIntercept and s.epochData[...].liensOpenForEpoch for A's payee have been modified as follows, but the function fails to update these parameters for P, the payee of A'.

if (_isPublicVault(s, payee)) {
      IPublicVault(payee).handleBuyoutLien(
        IPublicVault.BuyoutLienParams({
          lienSlope: calculateSlope(params.encumber.stack[params.position]),
          lienEnd: params.encumber.stack[params.position].point.end,
          increaseYIntercept: buyout -
            params.encumber.stack[params.position].point.amount
        })
      );
    }

Tools Used

Remix

Recommended Mitigation Steps

We need to introduce code to update these parameters for P, the payee of A'.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. transferAmount is calculated wrongly in function claim() of WithdrawProxy.sol as follows:

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

The correct one should be:

transferAmount = uint256(s.withdrawRatio).mulDivDown(
        balance,
        1e18
      );

In other words, unless the decimals of the asset is 18, othewise transferAmount will not be correctly calculated.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/WithdrawProxy.sol#L271-L274 transferAmount is calculated wrongly in function claim() because:

1. withdrawRatio is set via the function

 function setWithdrawRatio(uint256 liquidationWithdrawRatio) public onlyVault {
    _loadSlot().withdrawRatio = liquidationWithdrawRatio.safeCastTo88();
  }

2. while liquidationWithdrawRatio is calculated via

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

which uses a decimal of 18, that is 100% is represented as 1e18.

Therefore, the following calculation of transferAmount is wrong when asset().decimals() != 18. Note that balance already uses the decimals of asset(),

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

The correct one should be:

transferAmount = uint256(s.withdrawRatio).mulDivDown(
        balance,
        1e18
      );

where 1e18 represents 100% and transferAmount will also use the decimals of asset().

Tools Used

Remix

Recommended Mitigation Steps

The correct one should be:

transferAmount = uint256(s.withdrawRatio).mulDivDown(
        balance,
        1e18
      );

where 1e18 represents 100%.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. The check of potentialDebt > params.encumber.stack[j].lien.details.liquidationInitialAsk is not done correctly in LienToken.sol. As a result, some liens might violate this constraint after refinancing, and when a lien is defaulted and put on auction, the initial price of the auction might not be able to cover all the potential debts of pending liens.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

The check of potentialDebt > params.encumber.stack[j].lien.details.liquidationInitialAsk is not done correctly below in LienToken.sol because: 1) in each iteration, potentialDebt is not the final value, 2) it has not considered the new lien, the constraint might be violated after refinancing, so we need to check the new stacks.

uint256 potentialDebt = 0;
    for (uint256 i = params.encumber.stack.length; i > 0; ) {
      uint256 j = i - 1;
      // should not be able to purchase lien if any lien in the stack is expired (and will be liquidated)
      if (block.timestamp >= params.encumber.stack[j].point.end) {
        revert InvalidState(InvalidStates.EXPIRED_LIEN);
      }

      potentialDebt += _getOwed(
        params.encumber.stack[j],
        params.encumber.stack[j].point.end
      );

      if (
        potentialDebt >
        params.encumber.stack[j].lien.details.liquidationInitialAsk
      ) {
        revert InvalidState(InvalidStates.INITIAL_ASK_EXCEEDED);
      }

      unchecked {
        --i;
      }
    }

Tools Used

Remix

Recommended Mitigation Steps

The fix is to calculate the final value of potentialDebt for the new stacks after refinancing and then iterate through each lien to check that the constraint it not violated.

for (i; i < n; ) {
      potentialDebt += _getOwed(newStack[i], newStack[i].point.end);
      unchecked {
        ++i;
      }
    }

for (i; i < n; ) {
      if (block.timestamp >=newStack[i].point.end) {
        revert InvalidState(InvalidStates.EXPIRED_LIEN);
      }
      if (
        potentialDebt >
        newStack[j].lien.details.liquidationInitialAsk
      ) {
        revert InvalidState(InvalidStates.INITIAL_ASK_EXCEEDED);
      }

      unchecked {
        ++i;
      }
    }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. Wrong amount of allowance approval for the mint() function, which leads to two consequences: the mint() function might fail each time due to insufficient allowance approval; or allowance approved is unnecessary large subject to future exploitation.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L21 The following allowance approval amount is shares, this is wrong since it needs to approve the amount of assets corresponds to those shares.

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

Tools Used

Remix

Recommended Mitigation Steps

We need to approve the amount of assets that corresponds to those shares, see below:

function mint(
    IERC4626 vault,
    address to,
    uint256 shares,
    uint256 maxAmountIn
  ) public payable virtual override returns (uint256 amountIn) {
    uint256 assets = previewMint(shares);
    ERC20(vault.asset()).safeApprove(address(vault), assets);      // @audit: we need approve ``assets`` rather than ``shares``
    if ((amountIn = vault.mint(shares, to)) > maxAmountIn) {
      revert MaxAmountError();
    }
  }

QA1. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaVaultBase.sol#L37 Wrong documentation in this line, it should ends at 41 instead of 44.

return _getArgAddress(21); //ends at 41

QA2. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaVaultBase.sol#L47 wrong documentation in this line, it should end with 61 instead of 64.

return _getArgAddress(41); //ends at 61

QA3. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaVaultBase.sol#L55 Wrong documentation, it should end with 125 instead of 116.

return _getArgUint256(93); //ends at 125

QA4. https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L129-L141 When supply == 0, both previewWithdraw() and previewRedeem() returns 10e18, it should return the input instead of a fixed valure regardless of in the input.

function previewMint(uint256 shares) public view virtual returns (uint256) {
    uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.

    return supply == 0 ? shares : shares.mulDivUp(totalAssets(), supply);
  }

  function previewWithdraw(
    uint256 assets
  ) public view virtual returns (uint256) {
    uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.

    return supply == 0 ? assets : assets.mulDivUp(supply, totalAssets());
  }

QA5. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AuthInitializable.sol#L41-L45 Zero address check for _owner and _authority is needed.

QA6. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AuthInitializable.sol#L95 For safety issue, transferring ownership should take two steps, first step is to propose a new pending owner, and the second step is let the new pending owner to accept the proposal and becomes the ownership, maybe using Zeppelin's claimable.sol: https://github.com/aragon/zeppelin-solidity/blob/master/contracts/ownership/Claimable.sol

QA7. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/WithdrawProxy.sol#L302-L304 It is necessary to check the range of liquidationWithdrawRatio to make sure it is not greater than 1e18 (100%)

 function setWithdrawRatio(uint256 liquidationWithdrawRatio) public onlyVault {
      if(liquidationWithdrawRatio > 1e18) revert OutOfRange(liquidationWithdrawRatio);

    _loadSlot().withdrawRatio = liquidationWithdrawRatio.safeCastTo88();
  }

QA8. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L462 WE need to check the point.end for newSlot as follows:

if (block.timestamp >= newSlot.point.end) {
        revert InvalidState(InvalidStates.EXPIRED_LIEN);
      }

QA9. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L70 Should include the address of the vault

emit NonceUpdated(address(this), s.strategistNonce);

QA10. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/WithdrawProxy.sol#L289-L300 Zero address check for withdrawProxy is necessary to avoid losing funding.

QA11. https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L27 Wrong check of minDepositAmount(), we should check assets instead of shares, the correction is:

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

QA12: https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L48 We do not need to approve allowance here since there is no need the vault to move funding from the AstariaRouter to the vault or any other account. Instead, the user should approve the AstariaRouter contract shares of Vaulttokens allowance.

QA13: https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L66 Zero address check for _TRANSFER_PROXY is necessary.

QA14. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L586-L588 Minor correct: exceedling cap should be ">":

if (v.depositCap != 0 && totalAssets() > v.depositCap) {
      revert InvalidState(InvalidStates.DEPOSIT_CAP_EXCEEDED);
}

QA15. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L916 Zero address check for newPayee is needed.

QA16. Inconsistent representation of s.withdrawRatio in terms of # of decimals: Below, s.withdrawRatio is represented as a WAD (18 decimals). https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/WithdrawProxy.sol#L260 But below, s.withdrawRatio is represented as 10**ERC20(asset()).decimals(), which depends on the particular asset() used. To make it consistent, we can always use the WAD representation for s.withdrawRatio, thus

transferAmount = uint256(s.withdrawRatio).mulDivDown(
        balance,
        1e18
      );

QA17. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L586-L588 The comparison with v.depositCap should use > instead of >=.

if (v.depositCap != 0 && totalAssets() = v.depositCap) {
      revert InvalidState(InvalidStates.DEPOSIT_CAP_EXCEEDED);
    }

QA18: https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L281 Zero address check for liquidator is needed to avoid losing funding to the zero address

QA19. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L293 Need to check that denominator is not equal to zero to avoid divide by zero revert.

QA20: https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L529-L532 Adding the address of the vault to the event: SlopeUpdated(newSlope) so that we can monitor which vault's slope has been updated.

QA21. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L204-L210 Both functions should return uint88 because uint88 are the datatypes of these two variables.

struct VaultData {
    uint88 yIntercept;
    uint48 slope;
    uint40 last;
    uint64 currentEpoch;
    uint88 withdrawReserve;
    uint88 liquidationWithdrawRatio;
    uint88 strategistUnclaimedShares;
    mapping(uint64 => EpochData) epochData;
  }

function getWithdrawReserve() public view returns (uint88) {
    return _loadStorageSlot().withdrawReserve;
  }

  function getLiquidationWithdrawRatio() public view returns (uint88) {
    return _loadStorageSlot().liquidationWithdrawRatio;
  }

QA22. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L69 Refactor all of the following code using function ROUTER():

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L120 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L215 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L198 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L221

QA23. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L337-L342 Maybe we should use the maximum liquidationInitialAsk here in all the stacks, otherwise, if we chose the liquidationInitialAsk from stack[0] by default, some liquidationInitialAsk agreements for other stacks might have already been violated.

QA24. G24. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L715-L725 We need to check that we will not exceed stack.length here, otherwise, it is an out-of-array-bound error:

uint stacklen = stack.length;
if(n > stacklen ) n = stacklen;

QA25. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L255-L263 The interests can only be calculated up to stack.point.end, so we need some adjustment here:

function _getInterest(Stack memory stack, uint256 timestamp)
    internal
    pure
    returns (uint256)
  {
    if(timestamp > stack.point.end) timestamp = stack.point.end;

    uint256 delta_t = timestamp - stack.point.last;

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

QA26. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L727-L740 We need adjust end according to each stack[i].point.end since we will not charge interest beyond stack[i].point.end.

 function getMaxPotentialDebtForCollateral(Stack[] memory stack, uint256 end)
    public
    view
    validateStack(stack[0].lien.collateralId, stack)
    returns (uint256 maxPotentialDebt)
  {
    uint256 i; 
    uint256 realEnd; 
    for (; i < stack.length; ) {
      if(end > stack[i].point.end) realEnd = stack[i].point.end;
      else realEnd = end;
 
      maxPotentialDebt += _getOwed(stack[i], realEnd); // @audit: need to calculate what the real end should be
      unchecked {
        ++i;
      }
    }
  }

QA29. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L761-L767 We need to adjust timestamp here since we will not charge a lien interest beyond stack.point.end.

function _getOwed(Stack memory stack, uint256 timestamp)
    internal
    pure
    returns (uint88)
  {
    if(timestamp > stack.point.end) timestamp = stack.point.end; // @audit: make adjustment to timestamp

    return stack.point.amount + _getInterest(stack, timestamp).safeCastTo88();
  }

G1. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L153-L175 Caching params.encumber.stack[j] can save gas:

Stack stack = params.encumber.stack[j]; 

G2. https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L148 Introducing a storage variable to store s.epochData[epoch].withdrawProxy can save gas:

WithdrawProxy storage wproxy = s.epochData[epoch].withdrawProxy;

G3. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L262 Deleting this line can save gas since this line is not needed.

G4. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L432-L433 Dropping the second condition since this first condition already implies the second one.

G5. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L684 Caching newLien.details.rate and newLien.details.duration can save gas

G6. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L138-L142 This check is not necessary and can be eliminated to save gas sine this check has already been conducted inside the previous call _createLien(s, params.encumber).

G7. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L397-L399 Deleting this line can save gas since we already make such assignment at L366.

G8. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L400-L403 We can use variable currentWithdrawProxy to save gas here:

uint256 drainBalance = WithdrawProxy(withdrawProxy).drain(
        s.withdrawReserve,
        currentWithdrawProxy
      );

G9. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L855-L881 Much gas can be saved with the following implementation since we have less memory copies.

function _removeStackPosition(Stack[] memory stack, uint8 position)
    internal
    returns (Stack[] memory)
  {
    require(position < length);

    for (int i=position; i < length - 1; ) {
      unchecked {
        stack[i] = stack[i + 1];
        ++i;
      }

      stack.pop();
    }
     
     emit LienStackUpdated(
      stack[position].lien.collateralId,
      position,
      StackAction.REMOVE,
      uint8(newStack.length)
    );

    return stack;

  }

G10. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L439-L457 _afterCommitToLien() can be deleted since it is not used anywhere.

G11. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/WithdrawProxy.sol#L258-L266 Enclosing this block inside unchecked can save gas since it is impossible to have underflow/overflow here.

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

G12. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/interfaces/IAstariaRouter.sol#L56-L85 Introducing a constant (such 10,000) as the denomiator for all denominator paramters can save gas.

G13. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L490-L493 Using * instead of muldivdown can save gas here.

function _totalAssets(VaultData storage s) internal view returns (uint256) {
    uint256 delta_t = block.timestamp - s.last;
    return uint256(s.slope*delta_t) + uint256(s.yIntercept);
  }

G14. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L338-L340 This if-block check can be deleted since the modifier onlyOwner(collateralId) has done the job.

G15. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L342 This line can be deleted since tokenContract is not used at all.

G16. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L138-L139 These two lines can be deleted since they are not used afterwards.

G17. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L154 Enclosing it inside unchecked can save gas since underflow is impossible due to the check in the for loop. In addition, j can be defined outside.

uint256 j = i - 1;

G18. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L122 Caching params.encumber.stack[params.position] can save gas for function _buyoutLien():

Stack calldata currentStack = params.encumber.stack[params.position];

G19. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L107 Caching params.encumber.lien.collateralId can save gas.

uint256 collateralId = params.encumber.lien.collateralId; 

G20. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L309 Introducing owed outside of the for loop can save gas to avoid allocating a new variable for each iteration.

G21. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L389 Caching params.lien.collateralId can save gas.

uint256 collateralId = params.lien.collateralId;

G22. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L464 Caching stack.length can save gas.

uint oldStackLength = stack.length;

G23: https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L649-L653 This is only necessary when remaining != 0, so we can add this condition to short-circuit to save gas:

if (remaining != 0 && _isPublicVault(s, payee)) {  // @audit: use short-circuit here by adding condition 1
      IPublicVault(payee).updateAfterLiquidationPayment(
        IPublicVault.LiquidationPaymentParams({remaining: remaining})
      );
}

G24: https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L829 Changing this line slightly:

if (owed > amount)

G25. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L836 This line is a duplicate function of L814. Only one of these two lines need to be used, no need for both.

G26. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L184-L189 _deployWithdrawProxyIfNotDeployed() should have returned the deployed WithdrawProxy to save the gas from reading from the state variable.

    WithdrawProxy wp = _deployWithdrawProxyIfNotDeployed(s, epoch);

    emit Withdraw(msg.sender, receiver, owner, assets, shares);

    // WithdrawProxy shares are minted 1:1 with PublicVault shares
    wp.mint(shares, receiver);                // @now we can save gas here

G27. https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L669 All public/external functions that can be called by privileged users can add payable to the function to save gas.