Astaria contest - shark's results

1. Use string.concat() to concatenate strings

As of Solidity v0.8.12 the string.concat(a,b) function is the preferred method, due to it being much more readable than abi.encodePacked().

• File: WithdrawProxy.sol Line 110
• File: WithdrawProxy.sol Line 124
• File: PublicVault.sol Line 83
• File: PublicVault.sol Line 93

2. Deprecated pragma

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.

Source: docs.soliditylang.org/en/v0.8.13/080-breaking-changes.html#silent-changes-of-the-semantics:

• File; CollateralToken.sol Line 16
• File: LienToken.sol Line 16

3. Solmate's safeTransferFrom() doesn't check for contract existence.

/// @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.

Source: SafeTransferLib.sol Line 9

As a result, safeTransferFrom() will return success even if the contract is non-existent. Due to this a contract may think that funds have transferred successfully when in reality no funds were transferred.

Here are some instances of this issue:

File: Vault.sol Line 66, Line 72

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

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

File: TransferProxy.sol Line 34

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

4. Use delete to clear variables instead of zero assignment

Using the delete keyword more clearly states your intention.

For example:

File: WithdrawProxy.sol Line 284

    s.finalAuctionEnd = 0;

The instance above could be changed to:

    delete s.finalAuctionEnd;

5. Typo mistakes

It is recommended to use the correct spelling of words. Not doing so can decrease readability.

File: Vault.sol Line 90

       /// @audit vautls --> vaults
90:    //invalid action private vautls can only be the owner or strategist

File CollateralToken.sol (Line 126, Line 507)

          /// @audit dont -> don't
126:      //revert auction params dont match

          /// @audit its -> (it's || it is)
507:      //get total Debt and ensure its being sold for more than that

File: PublicVault.sol (Line 307, Line 374)

        /// @audit calcualtion -> calculation
307:    // reset liquidationWithdrawRatio to prepare for re calcualtion

        @audit then -> than
374:    // prevent transfer of more assets then are available

File: IERC1155Receiver.sol Line 36

      /// @audit "a multiple" -> "multiple"
36:   * @dev Handles the receipt of a multiple ERC1155 token types. This function

6. safeCastTo32() not being utilized

SafeCastLib.sol is imported in AstariaRouter.sol. However, it is not fully utilized.

For instance:

File: AstariaRouter.sol Line 112

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

Instead of the above, consider utilizing safeCastTo32():

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

7. Incorrect uint256() casting

File: AstariaRouter.sol Line 690 - Line 691

52:  struct Details {
54:    uint256 rate; //rate per second
58:  }

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

As seen above, stack[position].lien.details.rate is cast into a uint256 when it is already of type uint256 rate;. However, s.minInterestBPS, which is type uint32 (defined here) is not cast into a uint256.

With that in mind, line 690-691 could be refactored to:

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

1. Enable/disable functions can be merged into a toggle function

The following functions have almost the same functionality:

File: VaultImplementation.sol Line 101-117

101:  /**
102:   * @notice disable the allowList for the vault
103:   */
104:  function disableAllowList() external virtual {
105:    require(msg.sender == owner()); //owner is "strategist"
106:    _loadVISlot().allowListEnabled = false;
107:    emit AllowListEnabled(false);
108:  }
109:
110:  /**
111:   * @notice enable the allowList for the vault
112:   */
113:  function enableAllowList() external virtual {
114:    require(msg.sender == owner()); //owner is "strategist"
115:    _loadVISlot().allowListEnabled = true;
116:    emit AllowListEnabled(true);
117:  }

To save gas, consider merging the functionalities into a new function, setAllowListEnabled():

  /**
   * @notice enable/disable the allowList for the vault
   */
  function setAllowListEnabled(bool isEnabled) external virtual {
    require(msg.sender == owner()); //owner is "strategist"
    _loadVISlot().allowListEnabled = isEnabled;
    emit AllowListEnabled(isEnabled);
  }

2. Splitting require() statements that use && saves gas

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 &&.

• File: PublicVault.sol Line 672-675
• File: PublicVault.sol Line 687-690
• File: Vault.sol Line 65

3. Unused local variable

File: PublicVault.sol Line 251-265

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

As seen above, uint256 assets is never used. As such, it may be removed to save gas.

4. Unchecked math saves gas

Use the unchecked keyword to avoid unnecessary arithmetic checks and save gas when an underflow/overflow will not happen.

The following code line could be moved into the unchecked block as it will not overflow:

File: AstariaRouter.sol Line 512

5. Unused parameters

In the following functions, the parameters are never used:

• https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L82-L88
• https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L90-L102

6. x += y costs more gas than x = x + y (same for -=)

Using the addition operator instead of plus equals saves around 22 gas

There are 22 instances of this issue:

File: AstariaRouter.sol (Line 512)

512:      totalBorrowed += payout;

File: LienToken.sol L160, L161, L162, L164, L210, L480, L524, L525, L679, L720, L735, L830

160:      potentialDebt += _getOwed(
161:        params.encumber.stack[j],
162:        params.encumber.stack[j].point.end
164:      );

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

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

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

679:      totalCapitalAvailable -= spent;

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

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

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

File: PublicVault.sol, (#L174, #L179, #L380, #L405, #L565, #L583, #L606, #L624)

174:    es.balanceOf[owner] -= shares;

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

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

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

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

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

606:      s.strategistUnclaimedShares += feeInShares;

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

Files: VaultImplementation.sol Line 404

404:        amount -= fee;

File: WithdrawProxy.sol (Line 237, Line 277, Line 313)

237    s.withdrawReserveReceived += amount;

277        balance -= transferAmount;

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