Astaria contest - RaymondFam's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L123-L153 https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L233-L265 https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L19-L52

Vulnerability details

Impact

When depositing capital to selected PublicVault.sol via deposit() or mint(), the underlying ERC20 token amount transferred to the contract proportionately determines the minted amount of ERC-4626 VaultTokens that represent the liquidity provider's share in the vault. However, if the ERC20 token entailed is deflationary, it could lead to accounting errors resulting in the last batch of liquidity providers unable to withdraw their funds due to insufficient contract balance.

Proof of Concept

File: ERC4626-Cloned.sol#L19-L52

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

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

    _mint(receiver, shares);

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

    afterDeposit(assets, shares);
  }

  function mint(
    uint256 shares,
    address receiver
  ) public virtual returns (uint256 assets) {
    assets = previewMint(shares); // No need to check for rounding error, previewMint rounds up.
    require(assets > minDepositAmount(), "VALUE_TOO_SMALL");
    // Need to transfer before minting or ERC777s could reenter.
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

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

    afterDeposit(assets, shares);
  }

super.deposit() and super.mint() are unanimously used by deposit() and mint() in AstariaRouter.sol and PublicVault.sol to correspondingly invoke the above parental functions.

As can be seen in the code block, no measure has been made to either stem or cater for fee-on-transfer tokens.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider:

1. whitelisting the underlying deposit asset ensuring no fee-on-transfer token is allowed when deploying a new PublicVault from AstariaRouter.sol, or
2. calculating the balance before and after the transfer, and use the difference between those two balances as the amount rather than using the input amount if deflationary token is going to be allowed in the protocol.

Comments and code mismatch

As denoted by the comments on commitToLiens() in IAstariaRouter.sol:

  /**
   * @notice Deposits collateral and requests loans for multiple NFTs at once.
   * @param commitments The commitment proofs and requested loan data for each loan.
   * @return lienIds the lienIds for each loan.
   */

However, the logic entailed in AstariaRouter.sol only deals with one NFT and multiple, NOT multiple NFTs. Consider updating the NatSpec associated to better reflect the intended design of this particular function.

Solmate's contract existence check

As denoted by File: SafeTransferLib.sol#L9:

/// @dev Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.

As such, the following token transfers associated with Solmate's SafeTransferLib.sol, are some of the instances performing low-level calls without confirming contract’s existence that could return success even though no function call was executed:

File: TransferProxy.sol#L34

    ERC20(token).safeTransferFrom(from, to, amount);

File: Vault.sol

66:    ERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);

72:    ERC20(asset()).safeTransferFrom(address(this), msg.sender, amount);

Camel casing input parameter names

Input parameter names should be conventionally camel cased where possible.

Here are some of the instances entailed:

File: TransferProxy.sol#L24

-  constructor(Authority _AUTHORITY) Auth(msg.sender, _AUTHORITY) {
+  constructor(Authority _authority) Auth(msg.sender, _authority) {

Camel/snake casing function names

Function names should also be conventionally camel cased where possible. If snake case is preferred/adopted, consider lower casing them.

Here are some of the instances entailed:

File: AstariaVaultBase.sol

50:  function START() public pure returns (uint256) {

54:  function EPOCH_LENGTH() public pure returns (uint256) {

58:  function VAULT_FEE() public pure returns (uint256) {

62:  function COLLATERAL_TOKEN() public view returns (ICollateralToken) {

Wrong method called

name() in Vault.sol, WithdrawProxy.sol, and PublicVault.sol have their ERC20 instance externally and erroneously calling symbol() instead of name().

Consider having their respective errors fixed as follows:

File: Vault.sol#L28-L36

  function name()
    public
    view
    virtual
    override(VaultImplementation)
    returns (string memory)
  {
-    return string(abi.encodePacked("AST-Vault-", ERC20(asset()).symbol()));
+    return string(abi.encodePacked("AST-Vault-", ERC20(asset()).name()));
  }

File: WithdrawProxy.sol#L103-L111

  function name()
    public
    view
    override(IERC20Metadata, WithdrawVaultBase)
    returns (string memory)
  {
    return
-      string(abi.encodePacked("AST-WithdrawVault-", ERC20(asset()).symbol()));
+      string(abi.encodePacked("AST-WithdrawVault-", ERC20(asset()).name()));
  }

File: PublicVault.sol#L76-L84

  function name()
    public
    view
    virtual
    override(IERC20Metadata, VaultImplementation)
    returns (string memory)
  {
-    return string(abi.encodePacked("AST-Vault-", ERC20(asset()).symbol()));
+    return string(abi.encodePacked("AST-Vault-", ERC20(asset()).name()));
  }

string.concat over abi.encodePacked

Solidity ^0.8.12 introduced string.concat() featuring better code readability than abi.encodePacked().

Here are some of the instances entailed:

File: Vault.sol

- 35:    return string(abi.encodePacked("AST-Vault-", ERC20(asset()).symbol()));
+ 35:    return string.concat("AST-Vault-", ERC20(asset()).symbol());

- 46:      string(abi.encodePacked("AST-V", owner(), "-", ERC20(asset()).symbol()));
+ 46:      string.concat("AST-V", owner(), "-", ERC20(asset()).symbol());

Non-compliant contract layout with Solidity's Style Guide

According to Solidity's Style Guide below:

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

In order to help readers identify which functions they can call, and find the constructor and fallback definitions more easily, functions should be grouped according to their visibility and ordered in the following manner:

constructor, receive function (if exists), fallback function (if exists), external, public, internal, private

And, within a grouping, place the view and pure functions last.

Additionally, inside each contract, library or interface, use the following order:

type declarations, state variables, events, modifiers, functions

Consider adhering to the above guidelines for all contract instances entailed in the code bases.

Typo mistakes

File: Vault.sol#L90

-    //invalid action private vautls can only be the owner or strategist
+    //invalid action private vaults can only be the owner or strategist

Empty blocks

Function with empty block should have a comment explaining why it is empty, or an event emitted.

Here are some of the instances entailed:

File: ClearingHouse.sol#L104

  function setApprovalForAll(address operator, bool approved) external {}

File: ClearingHouse.sol#L180-L186

  function safeBatchTransferFrom(
    address from,
    address to,
    uint256[] calldata ids,
    uint256[] calldata amounts,
    bytes calldata data
  ) public {}

Use delete to clear variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address or a boolean to false. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic.

For instance, the a = false instance below may be refactored as follows:

File: VaultImplementation.sol#L106

-    _loadVISlot().allowListEnabled = false;
+    delete _loadVISlot().allowListEnabled;

Deprecated code

As denoted in Solidity Documentation:

"... The pragma pragma experimental ABIEncoderV2; is still valid, but it is deprecated and has no effect. If you want to be explicit, please use pragma abicoder v2; instead."

Consider the removing the deprecated code line in the contract instance below:

File: CollateralToken.sol#L16

- pragma experimental ABIEncoderV2;

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

The code bases are in general lacking partial/full NatSpec that will be of added values to the users and developers if adequately provided.

Empty Event

The following event has no parameter in it to emit anything. Although the standard global variables like block.number and block.timestamp will implicitly be tagged along, consider adding some relevant parameters to the make the best out of this emitted event for better support of off-chain logging API.

File: VaultImplementation.sol#L149

    emit VaultShutdown();

Imported library not fully utilized

SafeCastLib.sol imported in AstariaRouter should be fully utilized, serving also as a measure to synchronize all related casting styles.

File: AstariaRouter.sol#L112

-    s.minInterestBPS = uint32((uint256(1e15) * 5) / (365 days));
+    s.minInterestBPS = ((uint256(1e15) * 5) / (365 days)).safeCastTo32;

Erroneous uint256() cast

stack[position].lien.details.rate, a uint256 variable, is cast into uint256() in AstoriaRouter.sol. This cast should have been done on s.minInterestBPS, a uint32 variable:

File: AstariaRouter.sol#L690-L691

-    uint256 maxNewRate = uint256(stack[position].lien.details.rate) -
-      s.minInterestBPS;
+    uint256 maxNewRate = stack[position].lien.details.rate -
+      uint256(s.minInterestBPS);

return statement on named returns

_executeCommitment() in AstariaRouter.sol adopts named returns but still resort to using return in its function logic. Consider removing the names to return values or the opposite manner but not the hybrid of both to avoid any unnecessary confusion.

File: AstariaRouter.sol#L761-L786

  function _executeCommitment(
    RouterStorage storage s,
    IAstariaRouter.Commitment memory c
  )
    internal
    returns (
      uint256,
-      ILienToken.Stack[] memory stack,
+      ILienToken.Stack[] memory,
-      uint256 payout
+      uint256
    )
  {
    uint256 collateralId = c.tokenContract.computeId(c.tokenId);

    if (msg.sender != s.COLLATERAL_TOKEN.ownerOf(collateralId)) {
      revert InvalidSenderForCollateral(msg.sender, collateralId);
    }
    if (!s.vaults[c.lienRequest.strategy.vault]) {
      revert InvalidVault(c.lienRequest.strategy.vault);
    }
    //router must be approved for the collateral to take a loan,
    return
      IVaultImplementation(c.lienRequest.strategy.vault).commitToLien(
        c,
        address(this)
      );
  }

Unneeded functions

In ClearingHouse.sol, the function parameters of balanceOf() and balanceOfBatch() aren't utilized in the respective function logic to retrieve any state variables. The code blocks entailed are essentially associated with a pure function logic. These external functions are neither called internally nor inherited, and additionally, there are no override and/or virtual visibility associated.

Consider removing them to save gas on contract deployment. If need be, type(uint256).max can always be coded inline by the calling contracts/functions instead of being externally interacted with.

File: ClearingHouse.sol#L82-L102

  function balanceOfBatch(address[] calldata accounts, uint256[] calldata ids)
    external
    view
    returns (uint256[] memory output)
  {
    output = new uint256[](accounts.length);
    for (uint256 i; i < accounts.length; ) {
      output[i] = type(uint256).max;
      unchecked {
        ++i;
      }
    }
  }

  function setApprovalForAll(address operator, bool approved) external {}

  function isApprovedForAll(address account, address operator)
    external
    view
    returns (bool)
  {
    return true;
  }

Split require statements using &&

Instead of using the && operator in a single require statement to check multiple conditions, using multiple require statements with 1 condition per require statement will save 3 GAS per &&.

Here are some of the instances entailed:

File: Vault.sol#L65

    require(s.allowList[msg.sender] && receiver == owner());

Unneeded function parameter

In deposit() of Vault.sol, the input parameter of receiver is only used for comparing with owner() and no state change is associated with it. Additionally, with the owner the only depositor allowed when having a new vault deployed via newVault() in AstariaRouter.sol, the second condition in the require statement may be omitted to save even more gas on contract deployment and function calls.

Consider having the code block refactored as follows :

File: Vault.sol#L59-L68

-  function deposit(uint256 amount, address receiver)
+  function deposit(uint256 amount)
    public
    virtual
    returns (uint256)
  {
    VIData storage s = _loadVISlot();
-    require(s.allowList[msg.sender] && receiver == owner());
+    require(s.allowList[msg.sender];
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);
    return amount;
  }

`newVault() and its associated logic could be trimmed

In newVault() of AstariaRouter.sol, since owner() is the only depositor allowed in allowList, it does not make much sense caching a 1 element array to update the mapping allowlist.

Consider having the unnecessary code lines removed to save gas both on contract deployment and function calls:

AstariaRouter.sol#L522-L542

  function newVault(address delegate, address underlying)
    external
    whenNotPaused
    returns (address)
  {
-    address[] memory allowList = new address[](1);
-    allowList[0] = msg.sender;
    RouterStorage storage s = _loadRouterSlot();

    return
      _newVault(
        s,
        underlying,
        uint256(0),
        delegate,
        uint256(0),
-        true,
+        false,
-        allowList,
+        /* emptyList */,
        uint256(0)
      );
  }

With the above trim measure in place, the second if block along with its nested for loop in the init() instance of VaultImplementation.sol will be skipped when externally invoked via IVaultImplementation(vaultAddr).init(IVaultImplementation.InitParams(), saving even more gas at deployment:

File: VaultImplementation.sol#L190-L208

  function init(InitParams calldata params) external virtual {
    require(msg.sender == address(ROUTER()));
    VIData storage s = _loadVISlot();

    if (params.delegate != address(0)) {
      s.delegate = params.delegate;
    }
    s.depositCap = params.depositCap.safeCastTo88();
    if (params.allowListEnabled) {
      s.allowListEnabled = true;
      uint256 i;
      for (; i < params.allowList.length; ) {
        s.allowList[params.allowList[i]] = true;
        unchecked {
          ++i;
        }
      }
    }
  }

Lastly, deposit() in Vault.sol will need to be refactored as follows to accommodate the trimming changes above:

File: Vault.sol#L59-L68

-  function deposit(uint256 amount, address receiver)
+  function deposit(uint256 amount)
    public
    virtual
    returns (uint256)
  {
    VIData storage s = _loadVISlot();
-    require(s.allowList[msg.sender] && receiver == owner());
+    require(msg.sender == owner());
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);
    return amount;
  }

|| costs less gas than its equivalent &&

Rule of thumb: (x && y) is (!(!x || !y))

Even with the 10k Optimizer enabled: ||, OR conditions cost less than their equivalent &&, AND conditions.

As an example, the code line instance below may be refactored as follows:

File: VaultImplementation.sol#L66

-    if (msg.sender != owner() && msg.sender != s.delegate) {
+    if (!(msg.sender == owner() || msg.sender == s.delegate)) {

Private function with embedded modifier reduces contract size

Consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that saves more gas on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.

For instance, the modifier instance below may be refactored as follows:

File: VaultImplementation.sol#L131-L140

+    function _whenNotPaused() private view {
+        if (ROUTER().paused()) {
+          revert InvalidRequest(InvalidRequestReason.PAUSED);
+        }
+
+        if (_loadVISlot().isShutdown) {
+          revert InvalidRequest(InvalidRequestReason.SHUTDOWN);
+        }
+    }

    modifier whenNotPaused() {
-        if (ROUTER().paused()) {
-          revert InvalidRequest(InvalidRequestReason.PAUSED);
-        }
-
-        if (_loadVISlot().isShutdown) {
-          revert InvalidRequest(InvalidRequestReason.SHUTDOWN);
-        }
+        _whenNotPaused();
        _;
    }

Unneeded event emission

setDelegate() has the event AllowListUpdated() emitted without having delegate_ enabled in the mapping allowList.

Consider removing this irrelevant emission to save gas both on contract deployment and function calls:

File: VaultImplementation.sol#L210-L216

  function setDelegate(address delegate_) external {
    require(msg.sender == owner()); //owner is "strategist"
    VIData storage s = _loadVISlot();
    s.delegate = delegate_;
    emit DelegateUpdated(delegate_);
-    emit AllowListUpdated(delegate_, true);
  }

Functions of similar logic can be merged

disableAllowList() and enableAllowList() bear similar (if not identical) logic.

Consider having them merged as follows to save gas on contract deployment:

File: VaultImplementation.sol#L104-L117

  function toggleAllowList(bool enabled) external virtual {
    require(msg.sender == owner()); //owner is "strategist"
    _loadVISlot().allowListEnabled = enabled;
    emit AllowListEnabled(enabled);
  }

+= and -= cost more gas

+= and -= generally cost 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop.

For instance, the -= instance below may be refactored as follows:

File: VaultImplementation.sol#L404

-        amount -= fee;
+        amount = amount - fee;

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, the following inequality instances may be refactored as follows:

File: AstariaRouter.sol#L697-L702

-      (newLien.details.rate <= maxNewRate &&
+      (newLien.details.rate < maxNewRate + 1 &&
-        newLien.details.duration + block.timestamp >=
+        newLien.details.duration + block.timestamp >
-        stack[position].point.end) ||
+        stack[position].point.end - 1) ||
-      (block.timestamp + newLien.details.duration - stack[position].point.end >=
+      (block.timestamp + newLien.details.duration - stack[position].point.end >
-        s.minDurationIncrease &&
+        s.minDurationIncrease - 1 &&
-        newLien.details.rate <= stack[position].lien.details.rate);
+        newLien.details.rate < stack[position].lien.details.rate + 1);

Unneeded check

In deposit() of ERC4626-Cloned.sol, shares > minDepositAmount() signifies that shares != 0. As such, the first require statement is just a waste of gas.

Consider having the function refactored as follows:

File: ERC4626-Cloned.sol#L19-L36

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

-    require(shares > minDepositAmount(), "VALUE_TOO_SMALL");
+    require((shares = previewDeposit(assets)) > minDepositAmount(), "VALUE_TOO_SMALL");

    ...

Unneeded code line

In deposit() of PublicVault.sol, totalAssets() is cached but never used in the function call.

Consider removing this unused line of code to save gas both on contract deployment and function calls:

  function deposit(uint256 amount, address receiver)
    public
    override(ERC4626Cloned)
    whenNotPaused
    returns (uint256)
  {
    VIData storage s = _loadVISlot();
    if (s.allowListEnabled) {
      require(s.allowList[receiver]);
    }

-    uint256 assets = totalAssets();

    return super.deposit(amount, receiver);
  }

Unneeded unit256() cast

Casting the numeral 0 to uint256() in the following instances is unnecessary and a waste of gas since 0 is already an unsigned integer belonging to any size, i.e. uint8, uint16, ..., uint256.

File: AstariaRouter.sol

458:    if (details.rate == uint256(0) || details.rate > s.maxInterestRate) {

535:        uint256(0),

537:        uint256(0),

540:        uint256(0),

724:    if (epochLength > uint256(0)) {

Unchecked SafeMath saves gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops. When no arithmetic overflow/underflow is going to happen, unchecked { ... } to use the previous wrapping behavior further saves gas.

For instance, the SafeMath instance in the for loop below could be extended to include += too:

File: AstariaRouter.sol#L505-L515

    uint256 i;
    for (; i < commitments.length; ) {
      if (i != 0) {
        commitments[i].lienRequest.stack = stack;
      }
      uint256 payout;
      (lienIds[i], stack, payout) = _executeCommitment(s, commitments[i]);
+      unchecked {
      totalBorrowed += payout;
-      unchecked {
        ++i;
      }

Function order affects gas consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.17",
settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Use of named returns for local variables saves gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable.

For instance, the code block below may be refactored as follows:

File: AstariaRouter.sol#L402-L405

-  function getAuctionWindow(bool includeBuffer) public view returns (uint256) {
+  function getAuctionWindow(bool includeBuffer) public view returns (uint256 auctionWindow) {
    RouterStorage storage s = _loadRouterSlot();
-    return s.auctionWindow + (includeBuffer ? s.auctionWindowBuffer : 0);
+    auctionWindow = s.auctionWindow + (includeBuffer ? s.auctionWindowBuffer : 0);
  }

Division by 1

Using mulDivDown from FixedPointMathLib.sol to divide x * y by 1, the denominator, is a waste of gas.

Consider having the following instance refactored as follows:

File: PublicVault.sol#L490-L493

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

Inline over internal

Internal function entailing only one line of code may be embedded in the calling function(s) to save gas.

Here is an instance entailed:

File: PublicVault.sol#L556-L560

  function _increaseOpenLiens(VaultData storage s, uint64 epoch) internal {
    unchecked {
      s.epochData[epoch].liensOpenForEpoch++;
    }
  }

Ternary over if ... else

Using ternary operator instead of the if else statement saves gas.

For instance, the code block below may be refactored as follows:

File: AstariaRouter.sol#L724-L728

 epochLength > uint256(0)
     ? vaultType = uint8(ImplementationType.PublicVault)
     : vaultType = uint8(ImplementationType.PrivateVault);

Payable access control functions costs less gas

Consider marking functions with access control as payable. This will save 20 gas on each call by their respective permissible callers for not needing to have the compiler check for msg.value.

For instance, the function below may be refactored as follows:

File: PublicVault.sol#L562

-  function afterPayment(uint256 computedSlope) public onlyLienToken {
+  function afterPayment(uint256 computedSlope) public payable onlyLienToken {