Astaria contest - 0xAgro's results

QA Report

Finding Summary

[NC-01] Inconsistent Astaria LOGO Format

Some contracts start a newline with a space in the Astaria Logo at the top of the contract, some do not. Consider sticking to a single style.

Style A: L3-L12 Style B: L3-L12

Style A Contracts: BeaconProxy.sol, ClearingHouse.sol, CollateralToken.sol, PublicVault.sol, AstariaRouter.sol, AstariaVaultBase.sol, IBeacon.sol, ISecurityHook.sol, IRouterBase.sol, ITransferProxy.sol, IFlashAction.sol, IStrategyValidator.sol, IAstariaVaultBase.sol, IWithdrawProxy.sol, IVaultImplementation.sol, IPublicVault.sol, ICollateralToken.sol, IAstariaRouter.sol, ILienToken.sol. Style B Contracts: TransferProxy.sol, Vault.sol, WithdrawProxy.sol, LienToken.sol, WithdrawVaultBase.sol, VaultImplementation.sol.

[NC-02] NatSpec Comment Used For Logo

The Astaria Logo in all related contracts uses the NatSpec comment /** instead of the standard /*. /** is losely reserved for NatSpec comments. Consider using the standard /* comment for the Astaria Logo.

[NC-03] Long Lines (> 120 Characters)

Lines with greater length than 120 characters are used. The Solidity Style Guide suggests that all lines should be 120 characters or less in width.

The following lines are longer than 120 characters, it is suggested to shorten these lines:

src/WithdrawProxy.sol

• 54, 56, 256.

src/CollateralToken.sol

• 349.

src/PublicVault.sol

• 476.

src/AstariaRouter.sol

• 75.

src/LienToken.sol

• 42, 804.

src/VaultImplementation.sol

• 226, 281, 282, 363, 376.

src/interfaces/IERC1155Receiver.sol

• 49.

src/interfaces/IWithdrawProxy.sol

• 27, 33, 35, 52, 64, 74.

lib/gpl/src/interfaces/IERC4626RouterBase.sol

• 10, 15.

lib/gpl/src/interfaces/IUniswapV3PoolState.sol

• 34, 38, 58, 60, 101, 104, 105.

src/interfaces/IPublicVault.sol

• 79, 145, 146.

src/interfaces/ICollateralToken.sol

• 100, 161.

src/interfaces/IAstariaRouter.sol

• 224, 232.

src/interfaces/ILienToken.sol

• 181, 193, 263, 272.

[NC-04] Contracts Missing @title NatSpec Tag

30 out of 45 of the contracts in scope are missing a @title tag. Given that 15 contracts all have a @title tag, consider adding one per the 30 remaining contracts.

TransferProxy.sol, BeaconProxy.sol, ClearingHouse.sol, CollateralToken.sol, WithdrawVaultBase.sol, AstariaVaultBase.sol, ERC4626-Cloned.sol, ERC20-Cloned.sol, ERC721.sol, IBeacon.sol, IERC165.sol, ISecurityHook.sol, IRouterBase.sol, IERC20Metadata.sol, ITransferProxy.sol, IFlashAction.sol, IStrategyValidator.sol, IAstariaVaultBase.sol, IERC1155Receiver.sol, IERC20.sol, IWithdrawProxy.sol, IERC721.sol, IERC1155.sol, IERC4626.sol, IVaultImplementation.sol, IPublicVault.sol, IV3PositionManager.sol, ICollateralToken.sol, IAstariaRouter.sol, and ILienToken.sol are missing a @title tag.

[NC-05] Inconsistent Comment Initial Spacing

Some comments have an initial space after // or /// while others do not. It is best for code clearity to keep a consistent style.

1. The following contracts only have initial space comments (EX. // foo): BeaconProxy.sol, WithdrawProxy.sol, WithdrawVaultBase.sol, ERC4626Router.sol, ERC4626RouterBase.sol, ERC4626-Cloned.sol, ERC20-Cloned.sol, ERC721.sol, IBeacon.sol, IERC165.sol, ISecurityHook.sol, IMulticall.sol, IRouterBase.sol, IERC721Receiver.sol, ITransferProxy.sol, IFlashAction.sol, IStrategyValidator.sol, IAstariaVaultBase.sol, IERC1155Receiver.sol, IERC20.sol, IERC4626Router.sol, IWithdrawProxy.sol, IUniswapV3Factory.sol, IERC4626RouterBase.sol, IERC1155.sol, IUniswapV3PoolState.sol, IERC4626.sol, IVaultImplementation.sol, and IPublicVault.sol.
2. No contracts have only no initial space comments (EX. //foo).
3. The following contracts have both: TransferProxy.sol, Vault.sol, ClearingHouse.sol, CollateralToken.sol, PublicVault.sol, AstariaRouter.sol, LienToken.sol, AstariaVaultBase.sol, VaultImplementation.sol, ICollateralToken.sol, IAstariaRouter.sol, and ILienToken.sol.

[NC-06] Inconsistent Named Returns

Some functions use named returns and others do not. It is best for code clearity to keep a consistent style.

1. The following contracts only have named returns (EX. returns(uint256 foo)): ERC4626Router.sol, and ERC4626RouterBase.sol.
2. The following contracts only have non-named returns (EX. returns(uint256)): BeaconProxy.sol, Vault.sol, WithdrawVaultBase.sol, and AstariaVaultBase.sol.
3. The following contracts have both: ClearingHouse.sol, WithdrawProxy.sol, CollateralToken.sol, PublicVault.sol, AstariaRouter.sol, LienToken.sol, ERC4626-Cloned.sol, ERC20-Cloned.sol, ERC721.sol, and VaultImplementation.sol.

[NC-07] Contract Layout Voids Solidity Docs

The Solidity Style Guide suggests the following contract layout order: Type declarations, State variables, Events, Modifiers, Functions.

The following contracts are not compliant (examples are only to prove the layout are out of order NOT a full description):

• WithdrawProxy.sol: State is positioned after Events.
• CollateralToken.sol: Modifiers are positioned after Functions.
• PublicVault.sol: Events are positioned after Functions.
• AstariaRouter.sol: Modifiers are positioned after Functions.
• LienToken.sol: Modifiers are positioned after Functions.
• VaultImplementation.sol: State is positioned after Functions.
• IPublicVault.sol: State is positioned after Functions.
• IV3PositionManager.sol: State is positioned after Functions.
• ICollateralToken.sol: State is positioned after Events.
• IAstariaRouter.sol: State is positioned after Events.
• ILienToken.sol: State is positioned after Events.

[NC-08] Order of Functions Not Compliant With Solidity Docs

The Solidity Style Guide suggests the following function order: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private.

The following contracts are not compliant (examples are only to prove the functions are out of order NOT a full description):

• BeaconProxy.sol: fallback function is positioned after internal functions.
• Vault.sol: external functions are positioned after public functions.
• ClearingHouse.sol: external functions are positioned after internal functions.
• WithdrawProxy.sol: external functions are positioned after public functions.
• CollateralToken.sol: external functions are positioned after public functions.
• PublicVault.sol: public functions are positioned after internal functions.
• AstariaRouter.sol: public functions are positioned after internal functions.
• LienToken.sol: external functions are positioned after internal functions.
• ERC20-Cloned.sol: external functions are positioned after internal functions.
• ERC721.sol: external functions are positioned after public functions.
• VaultImplementation.sol: external functions are positioned after internal functions.

[NC-09] Modifiers/Functions Should Replace Duplicate require Statements

There are times in the codebase where the same or similar require statements are used and can be made into a single modifier for better code cleanliness.

/src/ClearingHouse.sol Links: 199, 216.

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

lib/gpl/src/ERC4626-Cloned.sol Links: 27, 43.

27:	require(shares > minDepositAmount(), "VALUE_TOO_SMALL");
43:	require(assets > minDepositAmount(), "VALUE_TOO_SMALL");

lib/gpl/src/ERC721.sol Links: 155-160, 171-176.

155:	require(
156:	  to.code.length == 0 ||
157: ...
171:	require(
172:	  to.code.length == 0 ||
173: ...

Links: 240-250, 260-270.

240:	require(
241:	  to.code.length == 0 ||
242: ...
260:	require(
261:	  to.code.length == 0 ||
262: ...

/src/VaultImplementation.sol Links: 78, 96, 105, 144, 147, 211.

78:	require(msg.sender == owner()); //owner is "strategist"
96:	require(msg.sender == owner()); //owner is "strategist"
105:	require(msg.sender == owner()); //owner is "strategist"
144:	require(msg.sender == owner()); //owner is "strategist"
147:	require(msg.sender == owner()); //owner is "strategist"
211:	require(msg.sender == owner()); //owner is "strategist"

[NC-10] No License Indication

Some contracts are missing a license indication. If no license is used SPDX-License-Identifier: UNLICENSED should be at the top of a contract.

The following contracts are missing a license:

• IWETH9.sol
• IERC20Metadata.sol
• IERC721.sol
• IV3PositionManager.sol

[NC-11] acheive Spelling Mistake

The word achieve is misspelled as acheive.

/src/interfaces/IV3PositionManager.sol Links: 122, 123.

122:	/// @return amount0 The amount of token0 to acheive resulting liquidity
123:	/// @return amount1 The amount of token1 to acheive resulting liquidity

[NC-12] nft vs NFT

NFT is spelled in all capitals everywhere in the codebase except 2 places (EX. L27). Consider changing all occurrences of nft to NFT.

/src/ClearingHouse.sol Links: 115.

115:	address tokenContract, // collateral token sending the fake nft

/src/interfaces/ICollateralToken.sol Links: 63.

63:	//mapping of a security token hook for an nft's token contract address

[NC-13] Use of pragma experimental ABIEncoderV2;

As of Solidity v0.8.0 "pragma experimental ABIEncoderV2; is still valid, but is deprecated and has no effect". Consider removing all occurrences of pragma experimental ABIEncoderV2; when contracts are >=v0.8.0.

/src/CollateralToken.sol Links: 16.

16:	pragma experimental ABIEncoderV2;

/src/LienToken.sol Links: 16.

16:	pragma experimental ABIEncoderV2;

[NC-14] Inconsistent Code Style For Similar Code

There are two sections of code that are very similar yet written in 2 different formats A and B seen below:

lib/gpl/src/ERC721.sol A: 155-160, 171-176.

require(
  to.code.length == 0 ||
    ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") ==
    ERC721TokenReceiver.onERC721Received.selector,
      "UNSAFE_RECIPIENT"
);

B: 240-250, 260-270.

require(
  to.code.length == 0 ||
    ERC721TokenReceiver(to).onERC721Received(
      msg.sender,
      address(0),
      id,
      ""
    ) ==
    ERC721TokenReceiver.onERC721Received.selector,
  "UNSAFE_RECIPIENT"
);

Style B will not exceed 120 characters if expanded like A. Consider maintaining the same code style for better readability.

[NC-15] Extra Space in Comment

There is an extra space in the comment L25 of TransferProxy.sol between is and Auth.

/src/TransferProxy.sol Links: 25.

25:	//only constructor we care about is  Auth

[NC-16] Power of Ten Literal > 10e3 Not In Scientific Notation

Power of ten literals > 10e3 are easier to read when expressed in scientific notation. Consider expressing large powers of ten in scientific notation (Ex. 10e5).

/src/PublicVault.sol Links: 604.

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

[NC-17] Duplicate Code

Duplicate code should be put into a function. L239-L242 and L257-L260 of PublicVault.sol have the same code.

[NC-18] Empty Comment

There is an empty comment before the second //. Consider removing the first empty comment on L831 of LienToken.sol.

/src/LienToken.sol Links: 831.

831:	//      // slope does not need to be updated if paying off the rest, since we neutralize slope in beforePayment()

[NC-19] require Matches Existing Modifier

In the mint function of WithdrawProxy.sol, require(msg.sender == VAULT(), "only vault can mint"); is used which already matches the existing modifier onlyVault. Consider re-using onlyVault instead of inlining the modifier.

[L-01] Lack of events

There is a general lack of events in the codebase especially in critical functions like a privileged actor role change (EX. 339).

Gas Report

Finding Summary

[G-01] && In If Statement(s)

Seperating if statements saves gas. Example

//From
if (a != HIGH && b != LOW) {
	//Do Something
}
//To
if (a != HIGH) {
	if (b != LOW) {
		//Do Something
	}
}

/src/PublicVault.sol Links: 586.

586:	if (v.depositCap != 0 && totalAssets() >= v.depositCap) {

/src/AstariaRouter.sol Links: 476.

476:	if (timeToSecondEpochEnd > 0 && details.duration > timeToSecondEpochEnd) {

/src/LienToken.sol Links: 268, 271.

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

[G-02] i--/i-=1 Used Over --i

--i increments i directly. When using i--/i-=1 solc must create a temporary value, thus resulting in higher gas.

/src/PublicVault.sol Links: 539.

539:	s.epochData[epoch].liensOpenForEpoch--;

Suggested Change

539:	--s.epochData[epoch].liensOpenForEpoch;

/lib/gpl/src/ERC721.sol Links: 136, 223.

136:	s._balanceOf[from]--;
223:	s._balanceOf[owner]--;

Suggested Change

136:	--s._balanceOf[from];
223:	--s._balanceOf[owner];

[G-03] x += y Used

x = x + y is cheaper than x += y in some cases like below.

/src/WithdrawProxy.sol Links: 237, 313.

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

/src/PublicVault.sol Links: 565, 583, 606, 624.

565:	s.slope += computedSlope.safeCastTo48();
583:	s.yIntercept += assets.safeCastTo88();
606:	s.strategistUnclaimedShares += feeInShares;
624:	s.yIntercept += params.increaseYIntercept.safeCastTo88();

/src/AstariaRouter.sol Links: 512.

512:	totalBorrowed += payout;

/src/LienToken.sol Links: 160, 210, 480, 524, 720, 735.

160:	potentialDebt += _getOwed(
210:	maxPotentialDebt += _getOwed(newStack[i], newStack[i].point.end);
480:	potentialDebt += _getOwed(newStack[j], newStack[j].point.end);
524:	totalSpent += spent;
720:	maxPotentialDebt += _getOwed(stack[i], stack[i].point.end);
735:	maxPotentialDebt += _getOwed(stack[i], end);

[G-04] && In Require Statement(s)

Seperating require statements saves gas. Example

//From
require(a != HIGH && b != LOW);
//To
require(a != HIGH);
require(b != LOW);

/src/Vault.sol Links: 65.

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

[G-05] Struct Can Be Packed Into Fewer Slots

Structs are divided into 32 slots. Minimizing the number of slots used per struct saves on gas.

src/interfaces/IAstariaRouter.sol

• The StrategyDetailsParam struct uses 3 32 byte slots: [[1], [32], [20]] respectively. The slots can be rearranged as follows to minimize the number of slots to 2: [[1, 20], [32]].