Astaria contest - nogo's results

QA

[L-01] Documentation and code mismatch

The documentation is stating that a borrower can take up to 5 loans against the same NFT, but the code is not limiting the borrower. Considering adding a check to limit the number of loans or update the documentation.

• src/contract/AstariaRouter.sol

[L-02] init method is callable multiple times

Upgradable contracts should be initialized only once. Consider adding a check to prevent multiple calls to the init method. Even if the method is technically not callable by anyone and it is setting the delegate address and the allow list. It is still a good practice to add a check to prevent multiple calls.

• src/contract/VaultImplementation.sol

[L-03] Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment.

• src/contract/PublicVault.sol

// check for rounding error since we round down in previewRedeem.

[L-04] Useless casting

• src/contract/CollateralToken.sol
• src/contract/CollateralToken.sol
• src/contract/CollateralToken.sol
• src/contract/WithdrawProxy.sol

ERC20 tokenContract = ERC20(tokenAddress);

[L-05] Prefer external over public for function not called internally by the contract

• src/contract/AstariaRouter.sol
• src/contract/AstariaRouter.sol
• src/contract/CollateralToken.sol
• src/contract/LienToken.sol
• src/contract/LienToken.sol
• src/contract/LienToken.sol
• src/contract/LienToken.sol

[L-06] Follow the order layout in contract

As mentioned in solidity documention, it is recommended to follow the order layout in contract.

1. Type declarations
2. State variables
3. Events
4. Modifiers
5. Functions

• src/contract/AstariaRouter.sol
• src/contract/PublicVault.sol

[N-01] NATSPEC IS MISSING

NatSpec is missing for some functions.

• src/contract/PublicVault.sol

[N-02] Using switch statement instead of if-else

Considering the number of conditions, it is recommended to use switch statement instead of if-else.

• src/contract/AstariaRouter.sol
• src/contract/AstariaRouter.sol

[N-03] Use pure instead of view for functions that do not read from storage

• src/contract/ClearingHouse.sol
• src/contract/ClearingHouse.sol

[N-04] Remove functions that are not used

• src/contract/ClearingHouse.sol
• src/contract/ClearingHouse.sol

[N-05] Pragma experimental abiencoderV2 is deprecated

Use pragma abicoder v2 instead

• src/contract/CollateralToken.sol

[N-06] require()/revert() statements should have descriptive reason strings

• src/contract/CollateralToken.sol
• src/contract/PublicVault.sol

Gas optimizations

[G-01] Using > 0 costs more gas than != 0 when used on a uint in a require() statement

• src/contract/AstariaRouter.sol

[G-02] Functions guaranteed to revert when called by normal users can be marked payable

No additional checks are required since the function can accept both zero or non-zero values of ether.

• src/contract/AstariaRouter.sol
• src/contract/AstariaRouter.sol
• src/contract/AstariaRouter.sol
• src/contract/AstariaRouter.sol
• src/contract/AstariaRouter.sol
• src/contract/AstariaRouter.sol
• src/contract/AstariaRouter.sol

[G-03] Reject earlier when possible

_validateCommitment can be called at the begining of the function to save gas. This will also improve readability.

• src/contract/AstariaRouter.sol

[G-04] Unused imports can be removed

• src/contract/AstariaVaultBase.sol
• src/contract/AstariaVaultBase.sol
• src/contract/AstariaVaultBase.sol
• src/contract/CollateralToken.sol
• src/contract/CollateralToken.sol
• src/contract/LienToken.sol
• src/contract/LienToken.sol

[G-05] Use custom errors rather than rever()/require() strings to save gas

Custom errors are available from solidity version 0.8.0. Custom errors save ~50 gas each time they’re hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas.

• src/contract/ClearingHouse.sol
• src/contract/ClearingHouse.sol
• src/contract/ClearingHouse.sol
• src/contract/ClearingHouse.sol
• src/contract/CollateralToken.sol
• src/contract/CollateralToken.sol
• src/contract/PublicVault.sol

[G-05] Using bool for storage brings overhead

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled. Use uint256(1) and uint256(2) for true/false instead

• src/contract/AstariaRouter.sol

[G-06] Use assembly for address 0 check

Example

assembly {
 if iszero(_addr) {
  mstore(0x00, "zero address")
  revert(0x00, 0x20)
 }
}

• src/contract/AstariaRouter.sol

[G-07] Internal function never used can be removed

Try to keep the number of internal and private functions to a minimum to balance function complexity and quantity. In turn, this will help you reduce gas fees upon execution by reducing the number of function calls. However, keep in mind that having too large functions makes testing more difficult and even jeopardizes security.

• src/contract/CollateralToken.sol

function _settleAuction(CollateralStorage storage s, uint256 collateralId)
  internal
{
  delete s.collateralIdToAuction[collateralId];
}

[G-08] Use named returns for local variables

• src/contract/CollateralToken.sol
• src/contract/PublicVault.sol
• src/contract/VaultImplementation.sol

[G-09] Remove unused variables

• src/contract/PublicVault.sol