Astaria contest - Rolezn's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L251-L265 https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L148-L190 https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L148

Vulnerability details

Description

Some tokens take a transfer fee (e.g. STA, PAXG), some do not currently charge a fee but may do so in the future (e.g. USDT, USDC).

Should a fee-on-transfer token be added to the PublicVault, the tokens will be locked in the PublicVault.sol contract. Depositors will be unable to withdraw their rewards. In the current implementation, it is assumed that the received amount is the same as the transfer amount. However, due to how fee-on-transfer tokens work, much less will be received than what was transferred. As a result, later users may not be able to successfully withdraw their shares, as it may revert at https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L148 when WithdrawProxy is called due to insufficient balance.

Proof of Concept

i.e. Fee-on-transfer scenario: Contract calls transfer from contractA 100 tokens to current contract Current contract thinks it received 100 tokens It updates balances to increase +100 tokens While actually contract received only 90 tokens That breaks whole math for given token

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

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

  function _redeemFutureEpoch(
    VaultData storage s,
    uint256 shares,
    address receiver,
    address owner,
    uint64 epoch
  ) internal virtual returns (uint256 assets) {
    // check to ensure that the requested epoch is not in the past

    ERC20Data storage es = _loadERC20Slot();

    if (msg.sender != owner) {
      uint256 allowed = es.allowance[owner][msg.sender]; // Saves gas for limited approvals.

      if (allowed != type(uint256).max) {
        es.allowance[owner][msg.sender] = allowed - shares;
      }
    }

    if (epoch < s.currentEpoch) {
      revert InvalidState(InvalidStates.EPOCH_TOO_LOW);
    }
    require((assets = previewRedeem(shares)) != 0, "ZERO_ASSETS");
    // check for rounding error since we round down in previewRedeem.

    //this will underflow if not enough balance
    es.balanceOf[owner] -= shares;

    // Cannot overflow because the sum of all user
    // balances can't exceed the max uint256 value.
    unchecked {
      es.balanceOf[address(this)] += shares;
    }

    emit Transfer(owner, address(this), shares);
    // Deploy WithdrawProxy if no WithdrawProxy exists for the specified epoch
    _deployWithdrawProxyIfNotDeployed(s, epoch);

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

    // WithdrawProxy shares are minted 1:1 with PublicVault shares
    WithdrawProxy(s.epochData[epoch].withdrawProxy).mint(shares, receiver);
  }

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

These functions inherits functions from the ERC4626-Cloned.sol https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol

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

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L19-L36

Recommended Mitigation Steps

1. Consider comparing before and after balance to get the actual transferred amount.
2. Alternatively, disallow tokens with fee-on-transfer mechanics to be added as tokens.

Summary<a name="Summary">

Low Risk Issues

Total: 24 contexts over 12 issues

Non-critical Issues

Total: 205 contexts over 21 issues

Low Risk Issues

<a href="#Summary">[LOW‑1]</a><a name="LOW&#x2011;1"> Add to blacklist function

NFT thefts have increased recently, so with the addition of hacked NFTs to the platform, NFTs can be converted into liquidity. To prevent this, I recommend adding the blacklist function.

Marketplaces such as Opensea have a blacklist feature that will not list NFTs that have been reported theft, NFT projects such as Manifold have blacklist functions in their smart contracts.

Here is the project example; Manifold

Manifold Contract https://etherscan.io/address/0xe4e4003afe3765aca8149a82fc064c0b125b9e5a#code

     modifier nonBlacklistRequired(address extension) {
         require(!_blacklistedExtensions.contains(extension), "Extension blacklisted");
         _;
     }

<ins>Recommended Mitigation Steps</ins>

Add to Blacklist function and modifier.

<a href="#Summary">[LOW‑2]</a><a name="LOW&#x2011;2"> Use of ecrecover is susceptible to signature malleability

The built-in EVM precompile ecrecover is susceptible to signature malleability, which could lead to replay attacks. References: https://swcregistry.io/docs/SWC-117, https://swcregistry.io/docs/SWC-121, and https://medium.com/cryptronics/signature-replay-vulnerabilities-in-smart-contracts-3b6f7596df57. While this is not immediately exploitable, this may become a vulnerability if used elsewhere.

<ins>Proof Of Concept</ins>

246: function _validateCommitment(
    IAstariaRouter.Commitment calldata params,
    address receiver
  ) internal view {
    uint256 collateralId = params.tokenContract.computeId(params.tokenId);
    ERC721 CT = ERC721(address(COLLATERAL_TOKEN()));
    address holder = CT.ownerOf(collateralId);
    address operator = CT.getApproved(collateralId);
    if (
      msg.sender != holder &&
      receiver != holder &&
      receiver != operator &&
      !CT.isApprovedForAll(holder, msg.sender)
    ) {
      revert InvalidRequest(InvalidRequestReason.NO_AUTHORITY);
    }
    VIData storage s = _loadVISlot();
    address recovered = ecrecover(
      keccak256(
        _encodeStrategyData(
          s,
          params.lienRequest.strategy,
          params.lienRequest.merkle.root
        )
      ),
      params.lienRequest.v,
      params.lienRequest.r,
      params.lienRequest.s
    );
    if (
      (recovered != owner() && recovered != s.delegate) ||
      recovered == address(0)
    ) {
      revert IVaultImplementation.InvalidRequest(
        InvalidRequestReason.INVALID_SIGNATURE
      );
    }
  }

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L246

Recommended Mitigation Steps

Consider using OpenZeppelin’s ECDSA library (which prevents this malleability) instead of the built-in function. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L138-L149

<a href="#Summary">[LOW‑3]</a><a name="LOW&#x2011;3"> Init functions are susceptible to front-running

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: code-423n4/2021-09-sushimiso-findings#64)

<ins>Proof Of Concept</ins>

function init(InitParams calldata params) external virtual {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L190

<ins>Recommended Mitigation Steps</ins>

Ensure atomic calls to init functions along with deployment via robust deployment scripts or factory contracts. Emit explicit events for initializations of contracts. Enforce prevention of re-initializations via explicit setting/checking of boolean initialized variables in the main init function instead of downstream function checks.

<a href="#Summary">[LOW‑4]</a><a name="LOW&#x2011;4"> Use _safeMint instead of _mint

According to openzepplin's ERC721, the use of _mint is discouraged, use _safeMint whenever possible. https://docs.openzeppelin.com/contracts/3.x/api/token/erc721#ERC721-_mint-address-uint256-

<ins>Proof Of Concept</ins>

588: _mint(from_, collateralId);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L588

455: _mint(params.receiver, newLienId);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L455

512: _mint(msg.sender, unclaimed);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L512

139: _mint(receiver, shares);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L139

<ins>Recommended Mitigation Steps</ins>

Use _safeMint whenever possible instead of _mint

<a href="#Summary">[LOW‑5]</a><a name="LOW&#x2011;5"> The Contract Should approve(0) First

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

<ins>Proof Of Concept</ins>

203: ERC721(order.parameters.offer[0].token).approve(

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L203

<ins>Recommended Mitigation Steps</ins>

Approve with a zero amount first before setting the actual amount.

<a href="#Summary">[LOW‑6]</a><a name="LOW&#x2011;6"> Missing Checks for Address(0x0)

Lack of zero-address validation on address parameters may lead to transaction reverts, waste gas, require resubmission of transactions and may even force contract redeployments in certain cases within the protocol.

<ins>Proof Of Concept</ins>

88: function initialize: address _VAULT_IMPL

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L88

89: function initialize: address _SOLO_IMPL

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L89

90: function initialize: address _WITHDRAW_IMPL

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L90

91: function initialize: address _BEACON_PROXY_IMPL

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L91

92: function initialize: address _CLEARING_HOUSE_IMPL

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L92

339: function setNewGuardian: address _guardian

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L339

210: function setDelegate: address delegate_

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L210

<ins>Recommended Mitigation Steps</ins>

Consider adding explicit zero-address validation on input parameters of address type.

<a href="#Summary">[LOW‑7]</a><a name="LOW&#x2011;7"> Pragma Experimental ABIEncoderV2 is Deprecated

<ins>Proof Of Concept</ins>

pragma experimental ABIEncoderV2

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L16

pragma experimental ABIEncoderV2

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L16

<ins>Recommended Mitigation Steps</ins>

Use pragma abicoder v2 instead.

<a href="#Summary">[LOW‑8]</a><a name="LOW&#x2011;8"> Protect your NFT from copying in POW forks

Ethereum has performed the long-awaited "merge" that will dramatically reduce the environmental impact of the network

There may be forked versions of Ethereum, which could cause confusion and lead to scams as duplicated NFT assets enter the market.

If the Ethereum Merge, which took place in September 2022, results in the Blockchain splitting into two Blockchains due to the 'THE DAO' attack in 2016, this could result in duplication of immutable tokens (NFTs).

In any case, duplicate NFTs will exist due to the ETH proof-of-work chain and other potential forks, and there’s likely to be some level of confusion around which assets are 'official' or 'authentic.'

Even so, there could be a frenzy for these copies, as NFT owners attempt to flip the proof-of-work versions of their valuable tokens.

As ETHPOW and any other forks spin off of the Ethereum mainnet, they will yield duplicate versions of Ethereum’s NFTs. An NFT is simply a blockchain token, and it can work as a deed of ownership to digital items like artwork and collectibles. A forked Ethereum chain will thus have duplicated deeds that point to the same tokenURI

About Merge Replay Attack: https://twitter.com/elerium115/status/1558471934924431363?s=20&t=RRheaYJwo-GmSnePwofgag

<ins>Proof Of Concept</ins>

401: function tokenURI(uint256 collateralId)
    public
    view
    virtual
    override(ERC721, IERC721)
    returns (string memory)
  {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L401

348: function tokenURI(uint256 tokenId)
    public
    view
    override(ERC721, IERC721)
    returns (string memory)
  {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L348

<ins>Recommended Mitigation Steps</ins>

Add the following check:

if(block.chainid != 1) { 
    revert(); 
}

<a href="#Summary">[LOW‑9]</a><a name="LOW&#x2011;9"> Remove unused code

This code is not used in the main project contract files, remove it or add event-emit Code that is not in use, suggests that they should not be present and could potentially contain insecure functionalities.

<ins>Proof Of Concept</ins>

function computeDomainSeparator

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L271

function afterDeposit

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L575

<a href="#Summary">[LOW‑10]</a><a name="LOW&#x2011;10"> Use safetransfer Instead Of transfer

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

For example, Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert.

<ins>Proof Of Concept</ins>

374: super.transferFrom(from, to, id);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L374

<ins>Recommended Mitigation Steps</ins>

Consider using safeTransfer/safeTransferFrom or require() consistently.

<a href="#Summary">[LOW‑11]</a><a name="LOW&#x2011;11"> Admin privilege - A single point of failure can allow a hacked or malicious owner use critical functions in the project

The owner role has a single point of failure and onlyOwner can use critical a few functions.

owner role in the project: Owner is not behind a multisig and changes are not behind a timelock.

Even if protocol admins/developers are not malicious there is still a chance for Owner keys to be stolen. In such a case, the attacker can cause serious damage to the project due to important functions. In such a case, users who have invested in project will suffer high financial losses.

Impact

Hacked owner or malicious owner can immediately use critical functions in the project.

<ins>Proof Of Concept</ins>

270: function flashAction(
    IFlashAction receiver,
    uint256 collateralId,
    bytes calldata data
  ) external onlyOwner(collateralId) {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L270

331: function releaseToAddress(uint256 collateralId, address releaseTo)
    public
    releaseCheck(collateralId)
    onlyOwner(collateralId)
  {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L331

<ins>Recommended Mitigation Steps</ins>

Add a time lock to critical functions. Admin-only functions that change critical parameters should emit events and have timelocks. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services.

Allow only multi-signature wallets to call the function to reduce the likelihood of an attack.

https://twitter.com/danielvf/status/1572963475101556738?s=20&t=V1kvzfJlsx-D2hfnG0OmuQ

Reference

The status quo regarding significant centralization vectors has always been to award M severity, in order to warn users of the protocol of this category of risks. See <a href="https://gist.github.com/GalloDaSballo/881e7a45ac14481519fb88f34fdb8837">here</a> for list of centralization issues previously judged.

<a href="#Summary">[LOW‑12]</a><a name="LOW&#x2011;12"> Stack too deep

When attempting to run forge coverage, the compilers runs into a Stack too deep compiler error.

CompilerError: Stack too deep. Try compiling with `--via-ir` (cli) or the equivalent `viaIR: true` (standard JSON) while enabling the optimizer. Otherwise, try removing local variables.
   --> lib/seaport/contracts/lib/FulfillmentApplier.sol:223:9:
    |
223 |         assembly {
    |         ^ (Relevant source part starts here and spans across multiple lines).

Non Critical Issues

<a href="#Summary">[NC‑1]</a><a name="NC&#x2011;1"> Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user). Consider adding a timelock to the following functions:

<ins>Proof Of Concept</ins>

339: function setNewGuardian(address _guardian) external {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L339

66: function setAuctionData(ILienToken.AuctionData calldata auctionData)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L66

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

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L104

220: function settleLiquidatorNFTClaim() external {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L220

524: function settleAuction(uint256 collateralId) public {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L524

210: function setDelegate(address delegate_) external {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L210

302: function setWithdrawRatio(uint256 liquidationWithdrawRatio) public onlyVault {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L302

266: function setNewGuardian(address _guardian) external;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L266

139: function settleAuction(uint256 collateralId) external;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ICollateralToken.sol#L139

<a href="#Summary">[NC‑2]</a><a name="NC&#x2011;2"> Variable Names That Consist Of All Capital Letters Should Be Reserved For Const/immutable Variables

If the variable needs to be different based on which class it comes from, a view/pure function should be used instead.

<ins>Proof Of Concept</ins>

140: ClearingHouse CH = ClearingHouse(payable(s.clearingHouse[collateralId]));

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L140

234: ERC721 CT = ERC721(address(COLLATERAL_TOKEN()));

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L234

<a href="#Summary">[NC‑3]</a><a name="NC&#x2011;3"> Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

While it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.

<ins>Proof Of Concept</ins>

44: bytes32 public constant STRATEGY_TYPEHASH =
    keccak256("StrategyDetails(uint256 nonce,uint256 deadline,bytes32 root)");

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L44

47: bytes32 constant EIP_DOMAIN =
    keccak256(

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L47

51: bytes32 constant VERSION = keccak256("0");

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L51

<a href="#Summary">[NC‑4]</a><a name="NC&#x2011;4"> Critical Changes Should Use Two-step Procedure

The critical procedures should be two step process.

See similar findings in previous Code4rena contests for reference: https://code4rena.com/reports/2022-06-illuminate/#2-critical-changes-should-use-two-step-procedure

<ins>Proof Of Concept</ins>

339: function setNewGuardian(address _guardian) external {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L339

66: function setAuctionData(ILienToken.AuctionData calldata auctionData)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L66

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

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L104

220: function settleLiquidatorNFTClaim() external {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L220

524: function settleAuction(uint256 collateralId) public {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L524

210: function setDelegate(address delegate_) external {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L210

302: function setWithdrawRatio(uint256 liquidationWithdrawRatio) public onlyVault {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L302

266: function setNewGuardian(address _guardian) external;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L266

139: function settleAuction(uint256 collateralId) external;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ICollateralToken.sol#L139

<ins>Recommended Mitigation Steps</ins>

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

<a href="#Summary">[NC‑5]</a><a name="NC&#x2011;5"> Event emit should emit a parameter

Some emitted events do not have any emitted parameters. It is recommended to add some parameter such as state changes or value changes when events are emitted

<ins>Proof Of Concept</ins>

149: emit VaultShutdown()

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L149

<a href="#Summary">[NC‑6]</a><a name="NC&#x2011;6"> Event Is Missing Indexed Fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields).

Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

<ins>Proof Of Concept</ins>

event Claimed(
    address withdrawProxy,
    uint256 withdrawProxyAmount,
    address publicVault,
    uint256 publicVaultAmount
  );

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L42

event FileUpdated(FileType what, bytes data);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L54

event Liquidation(uint256 collateralId, uint256 position);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L294

event NewVault(
    address strategist,
    address delegate,
    address vault,
    uint8 vaultType
  );

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L295

event ListedOnSeaport(uint256 collateralId, Order listingOrder);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ICollateralToken.sol#L32

event FileUpdated(FileType what, bytes data);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ICollateralToken.sol#L33

event FileUpdated(FileType what, bytes data);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ILienToken.sol#L34

event LiensOpenForEpochRemaining(uint64 epoch, uint256 liensOpenForEpoch);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IPublicVault.sol#L169

event LienOpen(uint256 lienId, uint256 epoch);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IPublicVault.sol#L172

event AllowListUpdated(address, bool);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IVaultImplementation.sol#L52

<a href="#Summary">[NC‑7]</a><a name="NC&#x2011;7"> Function writing that does not comply with the Solidity Style Guide

Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

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

Functions should be grouped according to their visibility and ordered:

• constructor
• receive function (if exists)
• fallback function (if exists)
• external
• public
• internal
• private
• within a grouping, place the view and pure functions last

<ins>Proof Of Concept</ins>

All in-scope contracts

<a href="#Summary">[NC‑8]</a><a name="NC&#x2011;8"> 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. 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. Consider replacing assignments of zero with delete statements.

<ins>Proof Of Concept</ins>

308: s.liquidationWithdrawRatio = 0;
327: s.withdrawReserve = 0;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L308

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L327

377: s.withdrawReserve = 0;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L377

511: s.strategistUnclaimedShares = 0;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L511

284: s.finalAuctionEnd = 0;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L284

<a href="#Summary">[NC‑9]</a><a name="NC&#x2011;9"> NatSpec return parameters should be included in contracts

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

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

<ins>Proof Of Concept</ins>

All in-scope contracts

<ins>Recommended Mitigation Steps</ins>

Include return parameters in NatSpec comments

Recommendation Code Style: (from Uniswap3)

    /// @notice Adds liquidity for the given recipient/tickLower/tickUpper position
    /// @dev The caller of this method receives a callback in the form of IUniswapV3MintCallback#uniswapV3MintCallback
    /// in which they must pay any token0 or token1 owed for the liquidity. The amount of token0/token1 due depends
    /// on tickLower, tickUpper, the amount of liquidity, and the current price.
    /// @param recipient The address for which the liquidity will be created
    /// @param tickLower The lower tick of the position in which to add liquidity
    /// @param tickUpper The upper tick of the position in which to add liquidity
    /// @param amount The amount of liquidity to mint
    /// @param data Any data that should be passed through to the callback
    /// @return amount0 The amount of token0 that was paid to mint the given amount of liquidity. Matches the value in the callback
    /// @return amount1 The amount of token1 that was paid to mint the given amount of liquidity. Matches the value in the callback
    function mint(
        address recipient,
        int24 tickLower,
        int24 tickUpper,
        uint128 amount,
        bytes calldata data
    ) external returns (uint256 amount0, uint256 amount1);

<a href="#Summary">[NC‑10]</a><a name="NC&#x2011;10"> No need to initialize uints to zero

There is no need to initialize uint variables to zero as their default value is 0

<ins>Proof Of Concept</ins>

152: uint256 potentialDebt = 0;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L152

636: uint256 remaining = 0;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L636

254: uint256 transferAmount = 0;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L254

<a href="#Summary">[NC‑11]</a><a name="NC&#x2011;11"> Lines are too long

Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length

<ins>Proof Of Concept</ins>

54: // The sum of the remaining debt (amountOwed) accrued against the NFT at the timestamp when it is liquidated. yIntercept (virtual assets) of a PublicVault are not modified on liquidation, only once an auction is completed.

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L54

<a href="#Summary">[NC‑12]</a><a name="NC&#x2011;12"> Missing event for critical parameter change

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

<ins>Proof Of Concept</ins>

339: function setNewGuardian(address _guardian) external {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L339

66: function setAuctionData(ILienToken.AuctionData calldata auctionData)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L66

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

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L104

220: function settleLiquidatorNFTClaim() external {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L220

524: function settleAuction(uint256 collateralId) public {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L524

302: function setWithdrawRatio(uint256 liquidationWithdrawRatio) public onlyVault {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L302

266: function setNewGuardian(address _guardian) external;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L266

139: function settleAuction(uint256 collateralId) external;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ICollateralToken.sol#L139

<a href="#Summary">[NC‑13]</a><a name="NC&#x2011;13"> Implementation contract may not be initialized

OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5

<ins>Proof Of Concept</ins>

70: constructor()

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L70

76: constructor()

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L76

55: constructor()

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L55

24: constructor(Authority _AUTHORITY) Auth(msg.sender, _AUTHORITY)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/TransferProxy.sol#L24

<a href="#Summary">[NC‑14]</a><a name="NC&#x2011;14"> NatSpec comments should be increased in contracts

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability. https://docs.soliditylang.org/en/v0.8.15/natspec-format.html

<ins>Proof Of Concept</ins>

All in-scope contracts

<ins>Recommended Mitigation Steps</ins>

NatSpec comments should be increased in contracts

<a href="#Summary">[NC‑15]</a><a name="NC&#x2011;15"> Omissions in Events

Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, some events are missing important parameters

<ins>Proof Of Concept</ins>

459: event SlopeUpdated(uint48 newSlope);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L459

56: event DelegateUpdated(address);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IVaultImplementation.sol#L56

<ins>Recommended Mitigation Steps</ins>

The events should include the new value and old value where possible.

<a href="#Summary">[NC‑16]</a><a name="NC&#x2011;16"> Public Functions Not Called By The Contract Should Be Declared External Instead

Contracts are allowed to override their parents’ functions and change the visibility from external to public.

<ins>Proof Of Concept</ins>

function pullToken(
    address token,
    uint256 amount,
    address recipient
  ) public payable override {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L203

<a href="#Summary">[NC‑17]</a><a name="NC&#x2011;17"> Redundant Cast

The type of the variable is the same as the type to which the variable is being cast

<ins>Proof Of Concept</ins>

210: address(token)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L210

<a href="#Summary">[NC‑18]</a><a name="NC&#x2011;18"> Large multiples of ten should use scientific notation

Use (e.g. 1e6) rather than decimal literals (e.g. 100000), for better code readability.

<ins>Proof Of Concept</ins>

604: uint256 fee = x.mulDivDown(VAULT_FEE(), 10000);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L604

<a href="#Summary">[NC‑19]</a><a name="NC&#x2011;19"> Use bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))

<ins>Proof Of Concept</ins>

733: abi.encodePacked(
        address(this)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L733

569: abi.encodePacked(
            address(s.ASTARIA_ROUTER)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L569

83: return string(abi.encodePacked("AST-Vault-", ERC20(asset()
93: return string(abi.encodePacked("AST-V-", ERC20(asset()
222: abi.encodePacked(
          address(ROUTER()

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L83

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L93

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L222

35: return string(abi.encodePacked("AST-Vault-", ERC20(asset()
46: string(abi.encodePacked("AST-V", owner()

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

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

187: abi.encodePacked(bytes1(0x19)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L187

110: string(abi.encodePacked("AST-WithdrawVault-", ERC20(asset()
124: string(abi.encodePacked("AST-W", VAULT()

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L110

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L124

<ins>Recommended Mitigation Steps</ins>

Use bytes.concat() and upgrade to at least Solidity version 0.8.4 if required.

<a href="#Summary">[NC‑20]</a><a name="NC&#x2011;20"> Use of Block.Timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts. References: SWC ID: 116

<ins>Proof Of Concept</ins>

439: if (block.timestamp > commitment.lienRequest.strategy.deadline) {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L439

132: if (block.timestamp < params.endTime) {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L132

112: if (block.timestamp >= params.encumber.stack[params.position].point.end) {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L112

156: if (block.timestamp >= params.encumber.stack[j].point.end) {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L156

475: if (block.timestamp >= newStack[j].point.end) {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L475

805: if (block.timestamp >= end) {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L805

706: if (block.timestamp >= epochEnd) {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L706

250: if (block.timestamp < s.finalAuctionEnd) {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L250

<ins>Recommended Mitigation Steps</ins>

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

<a href="#Summary">[NC‑21]</a><a name="NC&#x2011;21"> Use Underscores for Number Literals

<ins>Proof Of Concept</ins>

604: uint256 fee = x.mulDivDown(VAULT_FEE(), 10000);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L604

Summary<a name="Summary">

Gas Optimizations

Total: 261 contexts over 16 issues

Gas Optimizations

<a href="#Summary">[GAS‑1]</a><a name="GAS&#x2011;1"> abi.encode() is less efficient than abi.encodepacked()

See for more information: https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison

<ins>Proof Of Concept</ins>

124: s.collateralIdToAuction[collateralId] != keccak256(abi.encode(params))

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L124

520: abi.encode(listingOrder.parameters)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L520

229: abi.encode(newStack)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L229

271: if (stateHash != bytes32(0) && keccak256(abi.encode(stack)) != stateHash) {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L271

406: abi.encode(newStack)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L406

448: newLienId = uint256(keccak256(abi.encode(params.lien)));

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L448

563: lienId = uint256(keccak256(abi.encode(lien)));

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L563

698: s.collateralStateHash[collateralId] = keccak256(abi.encode(stack));

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L698

155: abi.encode(

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L155

184: abi.encode(STRATEGY_TYPEHASH, s.strategistNonce, strategy.deadline, root)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L184

<a href="#Summary">[GAS‑2]</a><a name="GAS&#x2011;2"> Setting the constructor to payable

Saves ~13 gas per instance

<ins>Proof Of Concept</ins>

70: constructor()

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L70

76: constructor()

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L76

55: constructor()

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L55

24: constructor(Authority _AUTHORITY) Auth(msg.sender, _AUTHORITY)

https://github.com/code-423n4/2023-01-astaria/tree/main/src/TransferProxy.sol#L24

<a href="#Summary">[GAS‑3]</a><a name="GAS&#x2011;3"> Duplicated require()/revert() Checks Should Be Refactored To A Modifier Or Function

Saves deployment costs

<ins>Proof Of Concept</ins>

341: require(msg.sender == s.guardian);
347: require(msg.sender == s.guardian);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L341

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L347

199: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));
216: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));
223: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L199

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L216

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L223

241: require(s.allowList[receiver]);
259: require(s.allowList[receiver]);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L241

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L259

78: require(msg.sender == owner());
96: require(msg.sender == owner());
105: require(msg.sender == owner());
114: require(msg.sender == owner());
147: require(msg.sender == owner());
211: require(msg.sender == owner());

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L78

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L96

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L105

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L114

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L147

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L211

<a href="#Summary">[GAS‑4]</a><a name="GAS&#x2011;4"> Empty Blocks Should Be Removed Or Emit Something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})

<ins>Proof Of Concept</ins>

87: function _beforeFallback() internal virtual {}

https://github.com/code-423n4/2023-01-astaria/tree/main/src/BeaconProxy.sol#L87

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

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L104

186: ) public {}

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L186

272: ) internal virtual {}

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L272

272: {}

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L272

277: {}

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L277

<a href="#Summary">[GAS‑5]</a><a name="GAS&#x2011;5"> internal functions only called once can be inlined to save gas

<ins>Proof Of Concept</ins>

761: function _executeCommitment

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L761

788: function _transferAndDepositAssetIfAble

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L788

20: function _getBeacon

https://github.com/code-423n4/2023-01-astaria/tree/main/src/BeaconProxy.sol#L20

29: function _delegate

https://github.com/code-423n4/2023-01-astaria/tree/main/src/BeaconProxy.sol#L29

114: function _execute

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

425: function _generateValidOrderParameters

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L425

502: function _listUnderlyingOnSeaport

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L502

541: function _settleAuction

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L541

122: function _buyoutLien

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L122

233: function _replaceStackAtPositionWithNewLien

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L233

292: function _stopLiens

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L292

459: function _appendStack

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L459

512: function _payDebt

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L512

623: function _paymentAH

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L623

663: function _makePayment

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L663

715: function _getMaxPotentialDebtForCollateralUpToNPositions

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L715

855: function _removeStackPosition

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L855

911: function _setPayee

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L911

271: function computeDomainSeparator

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L271

439: function _afterCommitToLien

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L439

556: function _increaseOpenLiens

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L556

597: function _handleStrategistInterestReward

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L597

268: function _afterCommitToLien

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L268

379: function _requestLienAndIssuePayout

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L379

397: function _handleProtocolFee

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L397

<a href="#Summary">[GAS‑6]</a><a name="GAS&#x2011;6"> Multiple Address Mappings Can Be Combined Into A Single Mapping Of An Address To A Struct, Where Appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot.

<ins>Proof Of Concept</ins>

i.e. The following can be changed to the following struct:

60: mapping(address => bool) flashEnabled;
64: mapping(address => address) securityHooks;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ICollateralToken.sol#L60 https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ICollateralToken.sol#L64

    struct sampleStructName {
        bool flashEnabled;
        address securityHooks;
    }

<a href="#Summary">[GAS‑8]</a><a name="GAS&#x2011;8"> Optimize names to save gas

Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions.

See more <a href="https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92">here</a>

<ins>Proof Of Concept</ins>

All in-scope contracts

<ins>Recommended Mitigation Steps</ins>

Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas For example, the function IDs in the Gauge.sol contract will be the most used; A lower method ID may be given.

<a href="#Summary">[GAS‑9]</a><a name="GAS&#x2011;9"> <x> += <y> Costs More Gas Than <x> = <x> + <y> For State Variables

<ins>Proof Of Concept</ins>

512: totalBorrowed += payout;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L512

160: potentialDebt += _getOwed(
210: maxPotentialDebt += _getOwed(newStack[i], newStack[i].point.end);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L160

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L210

480: potentialDebt += _getOwed(newStack[j], newStack[j].point.end);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L480

524: totalSpent += spent;
525: payment -= spent;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L524-L525

679: totalCapitalAvailable -= spent;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L679

720: maxPotentialDebt += _getOwed(stack[i], stack[i].point.end);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L720

735: maxPotentialDebt += _getOwed(stack[i], end);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L735

830: stack.point.amount -= amount.safeCastTo88();

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

174: es.balanceOf[owner] -= shares;
179: es.balanceOf[address(this)] += shares;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L174

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L179

380: s.withdrawReserve -= withdrawBalance.safeCastTo88();
405: s.withdrawReserve -= drainBalance.safeCastTo88();

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L380

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L405

565: s.slope += computedSlope.safeCastTo48();

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L565

583: s.yIntercept += assets.safeCastTo88();

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L583

606: s.strategistUnclaimedShares += feeInShares;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L606

624: s.yIntercept += params.increaseYIntercept.safeCastTo88();

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L624

404: amount -= fee;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/VaultImplementation.sol#L404

237: s.withdrawReserveReceived += amount;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L237

277: balance -= transferAmount;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L277

313: s.expected += newLienExpectedValue.safeCastTo88();

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L313

<a href="#Summary">[GAS‑10]</a><a name="GAS&#x2011;10"> Public Functions To External

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

<ins>Proof Of Concept</ins>

Small sample of examples:

function redeemFutureEpoch(
    IPublicVault vault,
    uint256 shares,
    address receiver,
    uint64 epoch
  ) public virtual validVault(address(vault)) returns (uint256 assets) {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L187

function pullToken(
    address token,
    uint256 amount,
    address recipient
  ) public payable override {

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L203

This applies to all in-scope contracts that use the public modifier rather than the external modifier that are only called externally.

<a href="#Summary">[GAS‑11]</a><a name="GAS&#x2011;11"> Splitting require() statements that use && saves gas

Instead of using operator && on a single require. Using a two require can save more gas.

i.e. for require(s.allowList[msg.sender] && receiver == owner()); use:

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

<ins>Proof Of Concept</ins>

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

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

<a href="#Summary">[GAS‑12]</a><a name="GAS&#x2011;12"> Help The Optimizer By Saving A Storage Variable's Reference Instead Of Repeatedly Fetching It

To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array. The effect can be quite significant. As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.

<ins>Proof Of Concept</ins>

302: s.auctionData[collateralId].liquidator = liquidator;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L302

876: stack[position].lien.collateralId,

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L876

<a href="#Summary">[GAS‑13]</a><a name="GAS&#x2011;13"> Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract's gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed

<ins>Proof Of Concept</ins>

442: uint8 nlrType = uint8(_sliceUint(commitment.lienRequest.nlrDetails, 0));

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L442

722: uint8 vaultType;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/AstariaRouter.sol#L722

309: uint88 owed = _getOwed(stack[i], block.timestamp);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L309

803: uint64 end = stack.point.end;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L803

356: interfaceId == type(IERC165).interfaceId;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L356

449: uint48 newSlope = s.slope + lienSlope.safeCastTo48();

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L449

453: uint64 epoch = getLienEpoch(lienEnd);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L453

622: uint48 newSlope = s.slope - params.lienSlope.safeCastTo48();

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L622

605: uint88 feeInShares = convertToShares(fee).safeCastTo88();

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L605

654: uint64 lienEpoch = getLienEpoch(params.lienEnd);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L654

686: uint64 currentEpoch = s.currentEpoch;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L686

53: uint88 withdrawRatio;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L53

54: uint88 expected;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L54

55: uint40 finalAuctionEnd;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L55

314: uint40 auctionEnd = (block.timestamp + finalAuctionDelta).safeCastTo40();

https://github.com/code-423n4/2023-01-astaria/tree/main/src/WithdrawProxy.sol#L314

58: uint32 auctionWindow;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L58

59: uint32 auctionWindowBuffer;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L59

60: uint32 liquidationFeeNumerator;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L60

61: uint32 liquidationFeeDenominator;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L61

62: uint32 maxEpochLength;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L62

63: uint32 minEpochLength;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L63

64: uint32 protocolFeeNumerator;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L64

65: uint32 protocolFeeDenominator;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L65

73: uint88 maxInterestRate;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L73

74: uint32 minInterestBPS;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L74

78: uint32 buyoutFeeNumerator;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L78

79: uint32 buyoutFeeDenominator;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L79

80: uint32 minDurationIncrease;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L80

102: uint8 version;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L102

118: uint8 v;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IAstariaRouter.sol#L118

71: uint56 maxDuration;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ICollateralToken.sol#L71

37: uint8 maxLiens;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ILienToken.sol#L37

61: uint8 collateralType;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ILienToken.sol#L61

70: uint88 amount;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ILienToken.sol#L70

71: uint40 last;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ILienToken.sol#L71

232: uint40 end;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ILienToken.sol#L232

89: uint8 position;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ILienToken.sol#L89

231: uint88 amountOwed;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ILienToken.sol#L231

236: uint88 startAmount;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ILienToken.sol#L236

237: uint88 endAmount;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ILienToken.sol#L237

238: uint48 startTime;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ILienToken.sol#L238

239: uint48 endTime;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\ILienToken.sol#L239

21: uint64 liensOpenForEpoch;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IPublicVault.sol#L21

26: uint88 yIntercept;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IPublicVault.sol#L26

27: uint48 slope;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IPublicVault.sol#L27

28: uint40 last;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IPublicVault.sol#L28

29: uint64 currentEpoch;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IPublicVault.sol#L29

30: uint88 withdrawReserve;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IPublicVault.sol#L30

31: uint88 liquidationWithdrawRatio;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IPublicVault.sol#L31

32: uint88 strategistUnclaimedShares;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IPublicVault.sol#L32

51: uint40 lienEnd;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IPublicVault.sol#L51

76: uint24 fee;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IV3PositionManager.sol#L76

77: int24 tickLower;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IV3PositionManager.sol#L77

78: int24 tickUpper;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IV3PositionManager.sol#L78

135: uint128 liquidity;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IV3PositionManager.sol#L135

157: uint128 amount0Max;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IV3PositionManager.sol#L157

158: uint128 amount1Max;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IV3PositionManager.sol#L158

44: uint88 depositCap;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/interfaces\IVaultImplementation.sol#L44

<a href="#Summary">[GAS‑14]</a><a name="GAS&#x2011;14"> Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block

<ins>Proof Of Concept</ins>

150: payment - liquidatorPayment
156: payment - liquidatorPayment

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L150

https://github.com/code-423n4/2023-01-astaria/tree/main/src/ClearingHouse.sol#L156

861: newStack = new ILienToken.Stack[](length - 1);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L861

163: es.allowance[owner][msg.sender] = allowed - shares;

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L163

674: msg.sender == s.epochData[currentEpoch - 1].withdrawProxy
689: msg.sender == s.epochData[currentEpoch - 1].withdrawProxy

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L674

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L689

674: msg.sender == s.epochData[currentEpoch - 1].withdrawProxy
689: msg.sender == s.epochData[currentEpoch - 1].withdrawProxy
691: _setYIntercept(s, s.yIntercept - amount);

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L674

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L689

https://github.com/code-423n4/2023-01-astaria/tree/main/src/PublicVault.sol#L691

<a href="#Summary">[GAS‑15]</a><a name="GAS&#x2011;15"> Using storage instead of memory saves gas

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct

<ins>Proof Of Concept</ins>

137: Asset memory underlying = s.idToUnderlying[collateralId];
341: Asset memory underlying = s.idToUnderlying[collateralId];
434: Asset memory underlying = s.idToUnderlying[collateralId];

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L137

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L341

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L434

137: Asset memory underlying = s.idToUnderlying[collateralId];
341: Asset memory underlying = s.idToUnderlying[collateralId];
434: Asset memory underlying = s.idToUnderlying[collateralId];

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L137

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L341

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L434

137: Asset memory underlying = s.idToUnderlying[collateralId];
341: Asset memory underlying = s.idToUnderlying[collateralId];
434: Asset memory underlying = s.idToUnderlying[collateralId];

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L137

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L341

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L434

562: Asset memory incomingAsset = s.idToUnderlying[collateralId];

https://github.com/code-423n4/2023-01-astaria/tree/main/src/CollateralToken.sol#L562

797: Stack memory stack = activeStack[position];

https://github.com/code-423n4/2023-01-astaria/tree/main/src/LienToken.sol#L797

<a href="#Summary">[GAS‑16]</a><a name="GAS&#x2011;16"> Struct packing

In IFlashAction.sol, AuctionVaultParams's maxDuration is unlikely to ever be used with uint256.max value. This can be changed to at least uint96 to save 1 storage slot.

  struct AuctionVaultParams {
    address settlementToken;
    uint256 collateralId;
    uint256 maxDuration;
    uint256 startingPrice;
    uint256 endingPrice;
  }

Can be changed to:

  struct AuctionVaultParams {
    address settlementToken;
    uint96 maxDuration;
    uint256 collateralId;
    uint256 startingPrice;
    uint256 endingPrice;
  }