Astaria contest - chrisdior4's results

QA report

Low Risk

Total: 5 instances over 4 issues

Non-critical

Total: 12 instances over 7 issues

Low Risk

[L-01] Signature malleability for ecrecover

File: VaultImplementation.sol

The implementation of a cryptographic signature system in Ethereum contracts often assumes that the signature is unique, but signatures can be altered without the possession of the private key and still be valid. The EVM specification defines several so-called ‘precompiled’ contracts one of them being ecrecover which executes the elliptic curve public key recovery. A malicious user can slightly modify the three values v, r and s to create other valid signatures. A system that performs signature verification on contract level might be susceptible to attacks if the signature is part of the signed message hash. Valid signatures could be created by a malicious user to replay previously signed messages.

address recoveredAddress = ecrecover(

Line of code:

• https://github.com/code-423n4/2023-01-astaria/blob/57c2fe33c1d57bc2814bfd23592417fc4d5bf7de/src/VaultImplementation.sol#L246

Use OpenZeppelin ECDSA library.

[L-02] Prefer using _safeMint over _mint

Both LienToken::_createLien and CollateralToken::onERC721Received use ERC721's _mint method, which is missing the check if the account to mint the NFT to is a smart contract that can handle ERC721 tokens. The _safeMint method does exactly this, so prefer using it over _mint but always add a nonReentrant modifier, since calls to _safeMint can reenter.

 _mint(from_, collateralId);

 _mint(params.receiver, newLienId);

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L588

• https://github.com/code-423n4/2023-01-astaria/blob/57c2fe33c1d57bc2814bfd23592417fc4d5bf7de/src/LienToken.sol#L455

[L-03] 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

File: VaultImplementation.sol

function setDelegate(address delegate_) external

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L210

[L-04] Missing zero address check in function releaseToAddress in CollateralToken.sol

File: CollateralToken.sol

 function _releaseToAddress(
    CollateralStorage storage s,
    Asset memory underlyingAsset,
    uint256 collateralId,
    address `releaseTo`

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/57c2fe33c1d57bc2814bfd23592417fc4d5bf7de/src/CollateralToken.sol#L356

Consider adding zero address check.

Non-critical

[NC-01] Constants should be defined rather than using magic numbers

File: PublicVault.sol

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

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/57c2fe33c1d57bc2814bfd23592417fc4d5bf7de/src/PublicVault.sol#L604

File: AstariaRouter.sol

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

s.maxInterestRate = ((uint256(1e16) * 200) / (365 days)).safeCastTo88();

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/57c2fe33c1d57bc2814bfd23592417fc4d5bf7de/src/AstariaRouter.sol#L112

• https://github.com/code-423n4/2023-01-astaria/blob/57c2fe33c1d57bc2814bfd23592417fc4d5bf7de/src/AstariaRouter.sol#L115

[NC-02] Unclear comments in ClearningHouse.sol

The comments quality is poor, please revisit the commenting in this contract and consider improving it for better

[NC-03] Unnamed parameter in function _execute

File: ClearingHouse.sol

 function _execute(
    address tokenContract, // collateral token sending the fake nft
    address to, // buyer
    uint256 encodedMetaData, //retrieve token address from the encoded data
    uint256 // space to encode whatever is needed,

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/57c2fe33c1d57bc2814bfd23592417fc4d5bf7de/src/ClearingHouse.sol#L118

[NC-04] Functions setApprovalForAll and safeBatchTransferFrom are missing implementation and event emission

File: ClearingHouse.sol

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

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

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L104

• https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L180

[NC-05] Typos in comments

File: Vault.sol

Change vautls to vaults

File: PublicVault.sol

Change calcualtion to calculation

[NC-06] Unused imports

Consider removing them from the contracts

File: ClearingHouse.sol

 import {ConduitControllerInterface} from "seaport/interfaces/ConduitControllerInterface.sol"; 

File: LienToken.sol

import {Initializable} from "./utils/Initializable.sol";

[NC-07] Missing natspec

NatSpec documentation to all public methods and variables is essential for better understanding of the code by developers and auditors and is strongly recommended.

Most of the functions in the contract are missing natspec, some of them are missing only couple of params.

Consider adding natspec documentation for all the code.

Gas Optimization

Total: 10 issues

[G-01] THERE’S NO NEED TO SET DEFAULT VALUES FOR VARIABLES

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 … depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

File: LienToken.sol

uint256 potentialDebt = 0; 

Lines of code:

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

[G-02] Remove public visibility from constant variables

Constant variables are custom to the contract and won't need to be read on-chain - anyone can just see their values from the source code and, if needed, hardcode them into other contracts. Removing the public visibility will optimise deployment cost since no automatically generated getters will exist in the bytecode of the contract.

File: VaultImplementation.sol

bytes32 public constant STRATEGY_TYPEHASH

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L44

[G-03] Use uint256 instead of uint8 for storage variables and function parameters

Unless you are doing variable-packing in storage it is best to use always use uint256 because all other uint types get automatically padded to uint256 in memory anyway. Same applies for function parameters

File: VaultImplementation.sol

uint8 position

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L315

[G-04] Use custom errors instead of revert strings

Solidity 0.8.4 added the custom errors functionality, which can be use instead of revert strings, resulting in big gas savings on errors. Replace all revert statements in the contracts with custom error ones.

File: ClearingHouse.sol

Example:

require(payment >= currentOfferPrice, "not enough funds received");

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L134

[G-05] Use x != 0 instead of x > 0 for uint types

The != operator costs less gas than > and for uint types you can use it to check for non-zero values to save gas

File: ClearingHouse.sol

if (ERC20(paymentToken).balanceOf(address(this)) > 0) {

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L160

[G-06] x = x - y costs less gas than x -= y, same for addition

You can replace all -= and += occurrences to save gas

Files: AstariaRouter.sol

totalBorrowed += payout;

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L512

Files: LienToken.sol

 potentialDebt += _getOwed(

Lines of code:

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

[G-07] USE CALLDATA INSTEAD OF MEMORY

Some methods are declared as external but the arguments are defined as memory instead of as calldata. By marking the function as external it is possible to use calldata in the arguments shown below and save significant gas.

File: ClearingHouse.sol

function validateOrder(Order memory order) external 

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L197

[G-08] Splitting require() statements that use && saves gas

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper by 3 gas

File: Vault.sol

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

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/Vault.sol#L65

[G-09] USING STORAGE INSTEAD OF MEMORY FOR STRUCTS/ARRAYS SAVES GAS -3

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.

File: ClearingHouse.sol

Order[] memory listings = new Order[](1);

Lines of code:

• https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L200

File: LienToken.sol

Stack memory newStackSlot;

Point memory point = Point({

Lines of code:

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

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

[G-10] Caching the array length is more gas efficient.

This is because access to a local variable in solidity is more efficient than query storage / calldata / memory

We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... } to: uint len = array.length for (uint256 i=0; i<len; i++) { ... }

This issue persist all through the contracts.