Platform: Code4rena
Start Date: 05/01/2023
Pot Size: $90,500 USDC
Total HM: 55
Participants: 103
Period: 14 days
Judge: Picodes
Total Solo HM: 18
Id: 202
League: ETH
Rank: 40/103
Findings: 2
Award: $290.13
š Selected for report: 0
š Solo Findings: 0
š Selected for report: ladboy233
Also found by: 0x1f8b, 0xAgro, 0xSmartContract, 0xbepresent, 0xkato, Aymen0909, CodingNameKiki, Cryptor, Deekshith99, Deivitto, HE1M, IllIllI, Kaysoft, Koolex, PaludoX0, Qeew, RaymondFam, Rolezn, Sathish9098, Tointer, a12jmx, arialblack14, ast3ros, ayeslick, bin2chen, btk, caventa, ch0bu, chaduke, chrisdior4, delfin454000, descharre, evan, fatherOfBlocks, georgits, gz627, jasonxiale, joestakey, kaden, lukris02, nicobevi, nogo, oberon, oyc_109, pfapostol, rbserver, sakshamguruji, seeu, shark, simon135, slvDev, synackrst, tnevler, whilom, zaskoh
253.3371 USDC - $253.34
| Issue | Instances | |
|---|---|---|
| [Lā01] | Array lengths not checked | 1 |
| [Lā02] | Signatures vulnerable to malleability attacks | 2 |
| [Lā03] | tokenURI() does not follow EIP-721 | 1 |
| [Lā04] | _safeMint() should be used rather than _mint() wherever possible | 2 |
Total: 6 instances over 4 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | Invalid/extraneous/optional function definitions in interface | 1 |
| [Nā02] | Missing initializer modifier | 1 |
| [Nā03] | Adding a return statement when the function defines a named return variable, is redundant | 1 |
| [Nā04] | override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings | 8 |
| [Nā05] | constants should be defined rather than using magic numbers | 35 |
| [Nā06] | Events that mark critical parameter changes should contain both the old and the new value | 3 |
| [Nā07] | pragma experimental ABIEncoderV2 is deprecated | 2 |
| [Nā08] | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | 4 |
| [Nā09] | Inconsistent spacing in comments | 26 |
| [Nā10] | Lines are too long | 11 |
| [Nā11] | Inconsistent method of specifying a floating pragma | 5 |
| [Nā12] | Variable names that consist of all capital letters should be reserved for constant/immutable variables | 37 |
| [Nā13] | Using >/>= without specifying an upper bound is unsafe | 5 |
| [Nā14] | Typos | 6 |
| [Nā15] | Misplaced SPDX identifier | 1 |
| [Nā16] | File is missing NatSpec | 16 |
| [Nā17] | NatSpec is incomplete | 57 |
| [Nā18] | Not using the named return variables anywhere in the function is confusing | 25 |
| [Nā19] | Duplicated require()/revert() checks should be refactored to a modifier or function | 6 |
| [Nā20] | Consider using delete rather than assigning zero to clear values | 5 |
| [Nā21] | Avoid the use of sensitive terms | 1 |
| [Nā22] | Large assembly blocks should have extensive comments | 1 |
| [Nā23] | Contracts should have full test coverage | 1 |
| [Nā24] | Large or complicated code bases should implement fuzzing tests | 1 |
| [Nā25] | Function ordering does not follow the Solidity style guide | 69 |
| [Nā26] | Contract does not follow the Solidity style guide's suggested layout ordering | 21 |
Total: 349 instances over 26 issues
If the length of the arrays are not required to be of the same length, user operations may not be fully executed
There is 1 instance of this issue:
File: src/ClearingHouse.sol 90 function balanceOfBatch(address[] calldata accounts, uint256[] calldata ids) 91 external 92 view 93: returns (uint256[] memory output)
ecrecover() accepts as valid, two versions of signatures, meaning an attacker can use the same signature twice. Consider adding checks for signature malleability, or using OpenZeppelin's ECDSA library to perform the extra checks necessary in order to prevent this attack.
There are 2 instances of this issue:
File: lib/gpl/src/ERC20-Cloned.sol 121 address recoveredAddress = ecrecover( 122 keccak256( 123 abi.encodePacked( 124 "\x19\x01", 125 DOMAIN_SEPARATOR(), 126 keccak256( 127 abi.encode( 128 PERMIT_TYPEHASH, 129 owner, 130 spender, 131 value, 132 _loadERC20Slot().nonces[owner]++, 133 deadline 134 ) 135 ) 136 ) 137 ), 138 v, 139 r, 140 s 141: );
File: src/VaultImplementation.sol 246 address recovered = ecrecover( 247 keccak256( 248 _encodeStrategyData( 249 s, 250 params.lienRequest.strategy, 251 params.lienRequest.merkle.root 252 ) 253 ), 254 params.lienRequest.v, 255 params.lienRequest.r, 256 params.lienRequest.s 257: );
tokenURI() does not follow EIP-721The EIP states that tokenURI() "Throws if _tokenId is not a valid NFT", which the code below does not do. If the NFT has not yet been minted, tokenURI() should revert
There is 1 instance of this issue:
File: src/CollateralToken.sol 401 function tokenURI(uint256 collateralId) 402 public 403 view 404 virtual 405 override(ERC721, IERC721) 406 returns (string memory) 407 { 408 (address underlyingAsset, uint256 assetId) = getUnderlying(collateralId); 409 return ERC721(underlyingAsset).tokenURI(assetId); 410: }
_safeMint() should be used rather than _mint() wherever possible_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function
There are 2 instances of this issue:
File: src/CollateralToken.sol 588: _mint(from_, collateralId);
File: src/LienToken.sol 455: _mint(params.receiver, newLienId);
There is 1 instance of this issue:
File: src/interfaces/IERC721.sol /// @audit tokenURI(uint256) isn't defined with those arguments in the standard IERC721 definition 20: function tokenURI(uint256 id) external view returns (string memory);
initializer modifierThe contract extends Initializable/ReentrancyGuardUpgradeable but does not use the initializer modifier anywhere
There is 1 instance of this issue:
File: lib/gpl/src/ERC721.sol 10: abstract contract ERC721 is Initializable, IERC721 {
return statement when the function defines a named return variable, is redundantThere is 1 instance of this issue:
File: src/AstariaRouter.sol 758: return vaultAddr;
override function arguments that are unused should have the variable name removed or commented out to avoid compiler warningsThere are 8 instances of this issue:
File: src/ClearingHouse.sol 189: address operator_, 190: address from_, 191: uint256 tokenId_, 192: bytes calldata data_
File: src/PublicVault.sol 413: function _beforeCommitToLien(IAstariaRouter.Commitment calldata params) 575: function afterDeposit(uint256 assets, uint256 shares)
File: src/WithdrawProxy.sol 143: function deposit(uint256 assets, address receiver)
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 35 instances of this issue:
File: lib/gpl/src/ERC4626-Cloned.sol /// @audit 10e18 132: return supply == 0 ? 10e18 : shares.mulDivUp(totalAssets(), supply); /// @audit 10e18 140: return supply == 0 ? 10e18 : assets.mulDivUp(supply, totalAssets());
File: lib/gpl/src/ERC721.sol /// @audit 0x01ffc9a7 190: interfaceId == 0x01ffc9a7 || // ERC165 Interface ID for ERC165 /// @audit 0x80ac58cd 191: interfaceId == 0x80ac58cd || // ERC165 Interface ID for ERC721 /// @audit 0x5b5e139f 192: interfaceId == 0x5b5e139f; // ERC165 Interface ID for ERC721Metadata
File: src/AstariaRouter.sol /// @audit 1000 111: s.liquidationFeeDenominator = uint32(1000); /// @audit 7 113: s.minEpochLength = uint32(7 days); /// @audit 100 117: s.buyoutFeeNumerator = uint32(100); /// @audit 1000 118: s.buyoutFeeDenominator = uint32(1000); /// @audit 5 119: s.minDurationIncrease = uint32(5 days); /// @audit 0 418: mstore(0, OUTOFBOUND_ERROR_SELECTOR) /// @audit 0 419: revert(0, ONE_WORD) /// @audit 1_000 645: endingPrice: 1_000 wei
File: src/BeaconProxy.sol /// @audit 0 /// @audit 0 34: calldatacopy(0, 0, calldatasize()) /// @audit 0 /// @audit 0 /// @audit 0 38: let result := delegatecall(gas(), implementation, 0, calldatasize(), 0, 0) /// @audit 0 /// @audit 0 41: returndatacopy(0, 0, returndatasize()) /// @audit 0 46: revert(0, returndatasize()) /// @audit 0 45: case 0 { /// @audit 0 49: return(0, returndatasize())
File: src/CollateralToken.sol /// @audit 0xffffffff 167: : bytes4(0xffffffff); /// @audit 0xffffffff 182: : bytes4(0xffffffff);
File: src/LienToken.sol /// @audit 5 67: s.maxLiens = uint8(5); /// @audit 1000 342: auctionData.endAmount = uint88(1000 wei);
File: src/PublicVault.sol /// @audit 100 104: return 100 gwei; /// @audit 1e18 315: .mulDivDown(1e18, totalSupply()) /// @audit 1e18 333: totalAssets().mulDivDown(s.liquidationWithdrawRatio, 1e18) /// @audit 10000 604: uint256 fee = x.mulDivDown(VAULT_FEE(), 10000);
File: src/VaultImplementation.sol /// @audit 0x19 /// @audit 0x01 187: abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), hash);
File: src/WithdrawProxy.sol /// @audit 1e18 260: (s.expected - balance).mulWadDown(1e18 - s.withdrawRatio) /// @audit 1e18 264: (balance - s.expected).mulWadDown(1e18 - s.withdrawRatio)
This should especially be done if the new value is not required to be different from the old value
There are 3 instances of this issue:
File: lib/gpl/src/ERC721.sol /// @audit setApprovalForAll() 103: emit ApprovalForAll(msg.sender, operator, approved);
File: src/VaultImplementation.sol /// @audit setDelegate() 214: emit DelegateUpdated(delegate_); /// @audit setDelegate() 215: emit AllowListUpdated(delegate_, true);
pragma experimental ABIEncoderV2 is deprecatedUse pragma abicoder v2 instead
There are 2 instances of this issue:
File: src/CollateralToken.sol 16: pragma experimental ABIEncoderV2;
File: src/LienToken.sol 16: pragma experimental ABIEncoderV2;
keccak256(), should use immutable rather than constantWhile 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.
There are 4 instances of this issue:
File: lib/gpl/src/ERC20-Cloned.sol 15 bytes32 private constant PERMIT_TYPEHASH = 16 keccak256( 17 "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)" 18: );
File: src/VaultImplementation.sol 44 bytes32 public constant STRATEGY_TYPEHASH = 45: keccak256("StrategyDetails(uint256 nonce,uint256 deadline,bytes32 root)"); 47 bytes32 constant EIP_DOMAIN = 48 keccak256( 49 "EIP712Domain(string version,uint256 chainId,address verifyingContract)" 50: ); 51: bytes32 constant VERSION = keccak256("0");
Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file
There are 26 instances of this issue:
File: src/AstariaRouter.sol 65: // cast --to-bytes32 $(cast sig "OutOfBoundError()") 116: //63419583966; // 200% apy / second
File: src/ClearingHouse.sol 71: //only execute from the conduit 117: uint256 encodedMetaData, //retrieve token address from the encoded data 130: roundUp: true //we are a consideration we round up 174: bytes calldata data //empty from seaport 176: //data is empty and useless
File: src/CollateralToken.sol 120: //revert no auction 126: //revert auction params dont match 133: //auction hasn't ended yet 291: //look to see if we have a security handler for this asset 305: //trigger the flash action on the receiver 507: //get total Debt and ensure its being sold for more than that
File: src/interfaces/IAstariaRouter.sol 74: uint32 minInterestBPS; // was uint64
File: src/LienToken.sol 155: // should not be able to purchase lien if any lien in the stack is expired (and will be liquidated) 315: // update the public vault state and get the liquidation accountant back if any 804: // Blocking off payments for a lien that has exceeded the lien.end to prevent repayment unless the msg.sender() is the AuctionHouse 831: // // slope does not need to be updated if paying off the rest, since we neutralize slope in beforePayment() 838: // since the openLiens count is only positive when there are liens that haven't been paid off 839: // that should be liquidated, this lien should not be counted anymore
File: src/PublicVault.sol 173: //this will underflow if not enough balance 508: require(msg.sender == owner()); //owner is "strategist"
File: src/VaultImplementation.sol 123: address, // operator_ 124: address, // from_ 125: uint256, // tokenId_ 126: bytes calldata // data_
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
There are 11 instances of this issue:
File: lib/gpl/src/interfaces/IERC4626RouterBase.sol 10: The base router is a multicall style router inspired by Uniswap v3 with built-in features for permit, WETH9 wrap/unwrap, and ERC20 token pulling/sweeping/approving. 15: NOTE the router is capable of pulling any approved token from your wallet. This is only possible when your address is msg.sender, but regardless be careful when interacting with the router or ERC4626 Vaults.
File: src/AstariaRouter.sol 75: * @dev Setup transfer authority and set up addresses for deployed CollateralToken, LienToken, TransferProxy contracts, as well as PublicVault and SoloVault implementations to clone.
File: src/interfaces/ICollateralToken.sol 100: * @notice Executes a FlashAction using locked collateral. A valid FlashAction performs a specified action with the collateral within a single transaction and must end with the collateral being returned to the Vault it was locked in.
File: src/interfaces/IWithdrawProxy.sol 27: * @notice Called at epoch boundary, computes the ratio between the funds of withdrawing liquidity providers and the balance of the underlying PublicVault so that claim() proportionally pays optimized-out to all parties. 35: * @param finalAuctionDelta The timestamp by which the auction being added is guaranteed to end. As new auctions are added to the WithdrawProxy, this value will strictly increase as all auctions have the same maximum duration.
File: src/LienToken.sol 42: * @notice This contract handles the creation, payments, buyouts, and liquidations of tokenized NFT-collateralized debt (liens). Vaults which originate loans against supported collateral are issued a LienToken representing the right to loan repayments and auctioned funds on liquidation.
File: src/VaultImplementation.sol 282: * Starts by depositing collateral and take optimized-out a lien against it. Next, verifies the merkle proof for a loan commitment. Vault owners are then rewarded fees for successful loan origination. 363: * @notice Retrieves the recipient of loan repayments. For PublicVaults (VAULT_TYPE 2), this is always the vault address. For PrivateVaults, retrieves the owner() of the vault.
File: src/WithdrawProxy.sol 54: uint88 expected; // 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. 56: uint256 withdrawReserveReceived; // amount received from PublicVault. The WETH balance of this contract - withdrawReserveReceived = amount received from liquidations.
Some files use >=, some use ^. The instances below are examples of the method that has the fewest instances for a specific version. Note that using >= without also specifying <= will lead to failures to compile, or external project incompatability, when the major version changes and there are breaking-changes, so ^ should be preferred regardless of the instance counts
There are 5 instances of this issue:
File: lib/gpl/src/ERC4626RouterBase.sol 2: pragma solidity ^0.8.17;
File: lib/gpl/src/ERC4626Router.sol 2: pragma solidity ^0.8.17;
File: lib/gpl/src/ERC721.sol 2: pragma solidity ^0.8.16;
File: lib/gpl/src/interfaces/IERC4626RouterBase.sol 2: pragma solidity ^0.8.17;
File: lib/gpl/src/interfaces/IERC4626Router.sol 2: pragma solidity ^0.8.17;
constant/immutable variablesIf the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
There are 37 instances of this issue:
File: src/AstariaRouter.sol 84: Authority _AUTHORITY, 85: ICollateralToken _COLLATERAL_TOKEN, 86: ILienToken _LIEN_TOKEN, 87: ITransferProxy _TRANSFER_PROXY, 88: address _VAULT_IMPL, 89: address _SOLO_IMPL, 90: address _WITHDRAW_IMPL, 91: address _BEACON_PROXY_IMPL, 92: address _CLEARING_HOUSE_IMPL 329: (uint8 TYPE, address addr) = abi.decode(data, (uint8, address));
File: src/ClearingHouse.sol 221: IAstariaRouter ASTARIA_ROUTER = IAstariaRouter(_getArgAddress(0));
File: src/CollateralToken.sol 81: Authority AUTHORITY_, 82: ITransferProxy TRANSFER_PROXY_, 83: ILienToken LIEN_TOKEN_, 84: ConsiderationInterface SEAPORT_ 93: bytes32 CONDUIT_KEY = Bytes32AddressLib.fillLast12Bytes(address(this)); 140: ClearingHouse CH = ClearingHouse(payable(s.clearingHouse[collateralId]));
File: src/interfaces/IAstariaRouter.sol 67: ERC20 WETH; //20 68: ICollateralToken COLLATERAL_TOKEN; //20 69: ILienToken LIEN_TOKEN; //20 70: ITransferProxy TRANSFER_PROXY; //20 72: address BEACON_PROXY_IMPLEMENTATION; //20
File: src/interfaces/ICollateralToken.sol 52: ITransferProxy TRANSFER_PROXY; 53: ILienToken LIEN_TOKEN; 54: IAstariaRouter ASTARIA_ROUTER; 55: ConsiderationInterface SEAPORT; 56: ConduitControllerInterface CONDUIT_CONTROLLER; 57: address CONDUIT; 58: bytes32 CONDUIT_KEY;
File: src/interfaces/ILienToken.sol 38: address WETH; 39: ITransferProxy TRANSFER_PROXY; 40: IAstariaRouter ASTARIA_ROUTER; 41: ICollateralToken COLLATERAL_TOKEN;
File: src/LienToken.sol 59: function initialize(Authority _AUTHORITY, ITransferProxy _TRANSFER_PROXY)
File: src/TransferProxy.sol 24: constructor(Authority _AUTHORITY) Auth(msg.sender, _AUTHORITY) {
File: src/VaultImplementation.sol 234: ERC721 CT = ERC721(address(COLLATERAL_TOKEN()));
>/>= without specifying an upper bound is unsafeThere will be breaking changes in future versions of solidity, and at that point your code will no longer be compatable. While you may have the specific version to use in a configuration file, others that include your source files may not.
There are 5 instances of this issue:
File: lib/gpl/src/ERC20-Cloned.sol 2: pragma solidity >=0.8.16;
File: lib/gpl/src/ERC4626-Cloned.sol 2: pragma solidity >=0.8.16;
File: lib/gpl/src/interfaces/IMulticall.sol 4: pragma solidity >=0.7.5;
File: lib/gpl/src/interfaces/IUniswapV3Factory.sol 2: pragma solidity >=0.5.0;
File: lib/gpl/src/interfaces/IUniswapV3PoolState.sol 2: pragma solidity >=0.5.0;
There are 6 instances of this issue:
File: lib/gpl/src/interfaces/IUniswapV3Factory.sol /// @audit unenabled 38: /// @param fee The enabled fee, denominated in hundredths of a bip. Returns 0 in case of unenabled fee
File: src/interfaces/IERC4626.sol /// @audit redeemption 230: * @dev Allows an on-chain or off-chain user to simulate the effects of their redeemption at the current block,
File: src/interfaces/IV3PositionManager.sol /// @audit acheive 122: /// @return amount0 The amount of token0 to acheive resulting liquidity /// @audit acheive 123: /// @return amount1 The amount of token1 to acheive resulting liquidity
File: src/PublicVault.sol /// @audit calcualtion 307: // reset liquidationWithdrawRatio to prepare for re calcualtion
File: src/Vault.sol /// @audit vautls 90: //invalid action private vautls can only be the owner or strategist
The SPDX identifier should be on the very first line of the file
There is 1 instance of this issue:
File: lib/gpl/src/interfaces/IMulticall.sol 1: // forked from https://github.com/Uniswap/v3-periphery/blob/main/contracts/interfaces/IMulticall.sol
There are 16 instances of this issue:
File: lib/gpl/src/ERC4626-Cloned.sol
File: lib/gpl/src/ERC4626RouterBase.sol
File: lib/gpl/src/ERC4626Router.sol
File: src/AstariaVaultBase.sol
File: src/ClearingHouse.sol
File: src/interfaces/IAstariaVaultBase.sol
File: src/interfaces/IERC721.sol
File: src/interfaces/IFlashAction.sol
File: src/interfaces/IRouterBase.sol
File: src/interfaces/ISecurityHook.sol
File: src/interfaces/IStrategyValidator.sol
File: src/interfaces/ITransferProxy.sol
File: src/interfaces/IVaultImplementation.sol
File: src/TransferProxy.sol
File: src/Vault.sol
File: src/WithdrawVaultBase.sol
There are 57 instances of this issue:
File: src/AstariaRouter.sol /// @audit Missing: '@param _WITHDRAW_IMPL' /// @audit Missing: '@param _BEACON_PROXY_IMPL' /// @audit Missing: '@param _CLEARING_HOUSE_IMPL' 74 /** 75 * @dev Setup transfer authority and set up addresses for deployed CollateralToken, LienToken, TransferProxy contracts, as well as PublicVault and SoloVault implementations to clone. 76 * @param _AUTHORITY The authority manager. 77 * @param _COLLATERAL_TOKEN The address of the deployed CollateralToken contract. 78 * @param _LIEN_TOKEN The address of the deployed LienToken contract. 79 * @param _TRANSFER_PROXY The address of the deployed TransferProxy contract. 80 * @param _VAULT_IMPL The address of a base implementation of VaultImplementation for cloning. 81 * @param _SOLO_IMPL The address of a base implementation of a PrivateVault for cloning. 82 */ 83 function initialize( 84 Authority _AUTHORITY, 85 ICollateralToken _COLLATERAL_TOKEN, 86 ILienToken _LIEN_TOKEN, 87 ITransferProxy _TRANSFER_PROXY, 88 address _VAULT_IMPL, 89 address _SOLO_IMPL, 90 address _WITHDRAW_IMPL, 91 address _BEACON_PROXY_IMPL, 92 address _CLEARING_HOUSE_IMPL 93: ) external initializer { /// @audit Missing: '@param s' /// @audit Missing: '@param underlying' /// @audit Missing: '@param vaultFee' /// @audit Missing: '@param allowList' /// @audit Missing: '@param depositCap' 705 /** 706 * @dev Deploys a new Vault. 707 * @param epochLength The length of each epoch for a new PublicVault. If 0, deploys a PrivateVault. 708 * @param delegate The address of the Vault delegate. 709 * @param allowListEnabled Whether or not the Vault has an LP whitelist. 710 * @return vaultAddr The address for the new Vault. 711 */ 712 function _newVault( 713 RouterStorage storage s, 714 address underlying, 715 uint256 epochLength, 716 address delegate, 717 uint256 vaultFee, 718 bool allowListEnabled, 719 address[] memory allowList, 720 uint256 depositCap 721: ) internal returns (address vaultAddr) {
File: src/CollateralToken.sol /// @audit Missing: '@param s' /// @audit Missing: '@param underlyingAsset' /// @audit Missing: '@param collateralId' 348 /** 349 * @dev Transfers locked collateral to a specified address and deletes the reference to the CollateralToken for that NFT. 350 * @param releaseTo The address to send the NFT to. 351 */ 352 function _releaseToAddress( 353 CollateralStorage storage s, 354 Asset memory underlyingAsset, 355 uint256 collateralId, 356: address releaseTo /// @audit Missing: '@param address' /// @audit Missing: '@param bytes' 547 /** 548 * @dev Mints a new CollateralToken wrapping an NFT. 549 * @param from_ the owner of the collateral deposited 550 * @param tokenId_ The NFT token ID 551 * @return a static return of the receive signature 552 */ 553 function onERC721Received( 554 address, /* operator_ */ 555 address from_, 556 uint256 tokenId_, 557 bytes calldata // calldata data_ 558: ) external override whenNotPaused returns (bytes4) {
File: src/interfaces/IAstariaRouter.sol /// @audit Missing: '@param timeToSecondEpochEnd' 129 /** 130 * @notice Validates the incoming loan commitment. 131 * @param commitment The commitment proofs and requested loan data for each loan. 132 * @return lien the new Lien data. 133 */ 134 function validateCommitment( 135 IAstariaRouter.Commitment calldata commitment, 136 uint256 timeToSecondEpochEnd 137: ) external returns (ILienToken.Lien memory lien); /// @audit Missing: '@return' 147 * @param depositCap the deposit cap for the vault if any 148 */ 149 function newPublicVault( 150 uint256 epochLength, 151 address delegate, 152 address underlying, 153 uint256 vaultFee, 154 bool allowListEnabled, 155 address[] calldata allowList, 156 uint256 depositCap 157: ) external returns (address); /// @audit Missing: '@param recipient' 183 /** 184 * @notice Create a new lien against a CollateralToken. 185 * @param params The valid proof and lien details for the new loan. 186 * @return The ID of the created lien. 187 */ 188 function requestLienPosition( 189 IAstariaRouter.Commitment calldata params, 190 address recipient 191 ) 192 external 193 returns ( 194 uint256, 195 ILienToken.Stack[] memory, 196: uint256 /// @audit Missing: '@return' 209 * @param includeBuffer Adds the current auctionWindowBuffer if true. 210 */ 211: function getAuctionWindow(bool includeBuffer) external view returns (uint256); /// @audit Missing: '@param implType' 274 /** 275 * @notice Returns the address for the current implementation of a contract from the ImplementationType enum. 276 * @return impl The address of the clone implementation. 277 */ 278: function getImpl(uint8 implType) external view returns (address impl); /// @audit Missing: '@return' 286 * @param stack The Stack of existing Liens against the CollateralToken. 287 */ 288 function isValidRefinance( 289 ILienToken.Lien calldata newLien, 290 uint8 position, 291 ILienToken.Stack[] calldata stack 292: ) external view returns (bool);
File: src/interfaces/ICollateralToken.sol /// @audit Missing: '@return' 129 * @param params The auction data. 130 */ 131 function auctionVault(AuctionVaultParams calldata params) 132 external 133: returns (OrderParameters memory);
File: src/interfaces/ILienToken.sol /// @audit Missing: '@param auctionWindow' /// @audit Missing: '@param stack' /// @audit Missing: '@param liquidator' 117 /** 118 * @notice Stops accruing interest for all liens against a single CollateralToken. 119 * @param collateralId The ID for the CollateralToken of the NFT used as collateral for the liens. 120 */ 121 function stopLiens( 122 uint256 collateralId, 123 uint256 auctionWindow, 124 Stack[] calldata stack, 125: address liquidator /// @audit Missing: '@return' 130 * @param stack the lien 131 */ 132 function getBuyout(Stack calldata stack) 133 external 134 view 135: returns (uint256 owed, uint256 buyout); /// @audit Missing: '@return' 157 * @param stack the Lien 158 */ 159: function getInterest(Stack calldata stack) external returns (uint256); /// @audit Missing: '@return' 163 * @param collateralId the Lien to compute a point for 164 */ 165 function getCollateralState(uint256 collateralId) 166 external 167 view 168: returns (bytes32); /// @audit Missing: '@return' 172 * @param stack the Lien to compute a point for 173 */ 174 function getAmountOwingAtLiquidation(ILienToken.Stack calldata stack) 175 external 176 view 177: returns (uint256); /// @audit Missing: '@return' 181 * @param params LienActionEncumber data containing CollateralToken information and lien parameters (rate, duration, and amount, rate, and debt caps). 182 */ 183 function createLien(LienActionEncumber memory params) 184 external 185 returns ( 186 uint256 lienId, 187 Stack[] memory stack, 188: uint256 slope /// @audit Missing: '@return' 193 * @param params The LienActionBuyout data specifying the lien position, receiver address, and underlying CollateralToken information of the lien. 194 */ 195 function buyoutLien(LienActionBuyout memory params) 196 external 197: returns (Stack[] memory, Stack memory); /// @audit Missing: '@param token' /// @audit Missing: '@param auctionStack' 199 /** 200 * @notice Called by the ClearingHouse (through Seaport) to pay back debt with auction funds. 201 * @param collateralId The CollateralId of the liquidated NFT. 202 * @param payment The payment amount. 203 */ 204 function payDebtViaClearingHouse( 205 address token, 206 uint256 collateralId, 207 uint256 payment, 208: AuctionStack[] memory auctionStack /// @audit Missing: '@param collateralId' /// @audit Missing: '@return' 211 /** 212 * @notice Make a payment for the debt against a CollateralToken. 213 * @param stack the stack to pay against 214 * @param amount The amount to pay against the debt. 215 */ 216 function makePayment( 217 uint256 collateralId, 218 Stack[] memory stack, 219 uint256 amount 220: ) external returns (Stack[] memory newStack); /// @audit Missing: '@return' 246 * @param collateralId The ID of the CollateralToken. 247 */ 248 function getAuctionData(uint256 collateralId) 249 external 250 view 251: returns (AuctionData memory); /// @audit Missing: '@return' 255 * @param collateralId The ID of the CollateralToken. 256 */ 257 function getAuctionLiquidator(uint256 collateralId) 258 external 259 view 260: returns (address liquidator); /// @audit Missing: '@return' 264 * @param stack The stack data for active liens against the CollateralToken. 265 */ 266 function getMaxPotentialDebtForCollateral(ILienToken.Stack[] memory stack) 267 external 268 view 269: returns (uint256); /// @audit Missing: '@return' 274 * @param end The timestamp to accrue potential debt until. 275 */ 276 function getMaxPotentialDebtForCollateral( 277 ILienToken.Stack[] memory stack, 278 uint256 end 279: ) external view returns (uint256);
File: src/interfaces/IPublicVault.sol /// @audit Missing: '@return' 92 * @param end time to compute the end for 93 */ 94: function getLienEpoch(uint64 end) external view returns (uint64);
File: src/interfaces/IWithdrawProxy.sol /// @audit Missing: '@return' 45 * @param withdrawProxy The address of the withdrawProxy to drain to. 46 */ 47 function drain(uint256 amount, address withdrawProxy) 48 external 49: returns (uint256);
File: src/LienToken.sol /// @audit Missing: '@return' 253 * @param timestamp The timestamp at which to compute interest for. 254 */ 255 function _getInterest(Stack memory stack, uint256 timestamp) 256 internal 257 pure 258: returns (uint256) /// @audit Missing: '@param s' /// @audit Missing: '@return' 658 /** 659 * @dev Have a specified payer make a payment for the debt against a CollateralToken. 660 * @param stack the stack for the payment 661 * @param totalCapitalAvailable The amount to pay against the debts 662 */ 663 function _makePayment( 664 LienStorage storage s, 665 Stack[] calldata stack, 666 uint256 totalCapitalAvailable 667: ) internal returns (Stack[] memory newStack) { /// @audit Missing: '@param timestamp' 756 /** 757 * @dev Computes the debt owed to a Lien at a specified timestamp. 758 * @param stack The specified Lien. 759 * @return The amount owed to the Lien at the specified timestamp. 760 */ 761 function _getOwed(Stack memory stack, uint256 timestamp) 762 internal 763 pure 764: returns (uint88) /// @audit Missing: '@param s' /// @audit Missing: '@param position' /// @audit Missing: '@return' 784 /** 785 * @dev Make a payment from a payer to a specific lien against a CollateralToken. 786 * @param activeStack The stack 787 * @param amount The amount to pay against the debt. 788 * @param payer The address to make the payment. 789 */ 790 function _payment( 791 LienStorage storage s, 792 Stack[] memory activeStack, 793 uint8 position, 794 uint256 amount, 795 address payer 796: ) internal returns (Stack[] memory, uint256) {
File: src/PublicVault.sol /// @audit Missing: '@return' 249 * @param receiver The receiver of the resulting VaultToken shares. 250 */ 251 function deposit(uint256 amount, address receiver) 252 public 253 override(ERC4626Cloned) 254 whenNotPaused 255: returns (uint256) /// @audit Missing: '@param lienEnd' /// @audit Missing: '@param lienSlope' 435 /** 436 * @dev Hook for updating the slope of the PublicVault after a LienToken is issued. 437 * @param lienId The ID of the lien. 438 */ 439 function _afterCommitToLien( 440 uint40 lienEnd, 441 uint256 lienId, 442: uint256 lienSlope /// @audit Missing: '@param s' 592 /** 593 * @dev Handles the dilutive fees (on lien repayments) for strategists in VaultTokens. 594 * @param interestOwing the owingInterest for the lien 595 * @param amount The amount that was paid. 596 */ 597 function _handleStrategistInterestReward( 598 VaultData storage s, 599 uint256 interestOwing, 600: uint256 amount
File: src/VaultImplementation.sol /// @audit Missing: '@param strategy' /// @audit Missing: '@param root' /// @audit Missing: '@return' 164 /* 165 * @notice encodes the data for a 712 signature 166 * @param tokenContract The address of the token contract 167 * @param tokenId The id of the token 168 * @param amount The amount of the token 169 */ 170 function encodeStrategyData( 171 IAstariaRouter.StrategyDetailsParam calldata strategy, 172 bytes32 root 173: ) external view returns (bytes memory) { /// @audit Missing: '@param stack' /// @audit Missing: '@return' 308 /** 309 * @notice Buy optimized-out a lien to replace it with new terms. 310 * @param position The position of the specified lien. 311 * @param incomingTerms The loan terms of the new lien. 312 */ 313 function buyoutLien( 314 ILienToken.Stack[] calldata stack, 315 uint8 position, 316 IAstariaRouter.Commitment calldata incomingTerms 317 ) 318 external 319 whenNotPaused 320: returns (ILienToken.Stack[] memory, ILienToken.Stack memory) /// @audit Missing: '@return' 377 * @param receiver The borrower requesting the loan. 378 */ 379 function _requestLienAndIssuePayout( 380 IAstariaRouter.Commitment calldata c, 381 address receiver 382 ) 383 internal 384 returns ( 385 uint256 newLienId, 386 ILienToken.Stack[] memory stack, 387 uint256 slope, 388: uint256 payout
File: src/WithdrawProxy.sol /// @audit Missing: '@return' 130 * @param shares The number of shares to mint. 131 */ 132 function mint(uint256 shares, address receiver) 133 public 134 virtual 135 override(ERC4626Cloned, IERC4626) 136: returns (uint256 assets)
Consider changing the variable to be an unnamed one
There are 25 instances of this issue:
File: lib/gpl/src/ERC4626Router.sol /// @audit sharesOut 24 function depositToVault( 25 IERC4626 vault, 26 address to, 27 uint256 amount, 28 uint256 minSharesOut 29: ) external payable override returns (uint256 sharesOut) { /// @audit sharesOut 35 function depositMax( 36 IERC4626 vault, 37 address to, 38 uint256 minSharesOut 39: ) public payable override returns (uint256 sharesOut) { /// @audit amountOut 49 function redeemMax( 50 IERC4626 vault, 51 address to, 52 uint256 minAmountOut 53: ) public payable override returns (uint256 amountOut) {
File: src/AstariaRouter.sol /// @audit amountIn 123 function mint( 124 IERC4626 vault, 125 address to, 126 uint256 shares, 127 uint256 maxAmountIn 128 ) 129 public 130 payable 131 virtual 132 override 133 validVault(address(vault)) 134: returns (uint256 amountIn) /// @audit sharesOut 139 function deposit( 140 IERC4626 vault, 141 address to, 142 uint256 amount, 143 uint256 minSharesOut 144 ) 145 public 146 payable 147 virtual 148 override 149 validVault(address(vault)) 150: returns (uint256 sharesOut) /// @audit sharesOut 155 function withdraw( 156 IERC4626 vault, 157 address to, 158 uint256 amount, 159 uint256 maxSharesOut 160 ) 161 public 162 payable 163 virtual 164 override 165 validVault(address(vault)) 166: returns (uint256 sharesOut) /// @audit amountOut 171 function redeem( 172 IERC4626 vault, 173 address to, 174 uint256 shares, 175 uint256 minAmountOut 176 ) 177 public 178 payable 179 virtual 180 override 181 validVault(address(vault)) 182: returns (uint256 amountOut) /// @audit assets 187 function redeemFutureEpoch( 188 IPublicVault vault, 189 uint256 shares, 190 address receiver, 191 uint64 epoch 192: ) public virtual validVault(address(vault)) returns (uint256 assets) { /// @audit lien 426 function validateCommitment( 427 IAstariaRouter.Commitment calldata commitment, 428 uint256 timeToSecondEpochEnd 429: ) public view returns (ILienToken.Lien memory lien) { /// @audit stack /// @audit payout 761 function _executeCommitment( 762 RouterStorage storage s, 763 IAstariaRouter.Commitment memory c 764 ) 765 internal 766 returns ( 767 uint256, 768 ILienToken.Stack[] memory stack, 769: uint256 payout
File: src/CollateralToken.sol /// @audit validOrderMagicValue 157 function isValidOrder( 158 bytes32 orderHash, 159 address caller, 160 address offerer, 161 bytes32 zoneHash 162: ) external view returns (bytes4 validOrderMagicValue) { /// @audit validOrderMagicValue 171 function isValidOrderIncludingExtraData( 172 bytes32 orderHash, 173 address caller, 174 AdvancedOrder calldata order, 175 bytes32[] calldata priorOrderHashes, 176 CriteriaResolver[] calldata criteriaResolvers 177: ) external view returns (bytes4 validOrderMagicValue) {
File: src/LienToken.sol /// @audit newStack 107 function buyoutLien(ILienToken.LienActionBuyout calldata params) 108 external 109 validateStack(params.encumber.lien.collateralId, params.encumber.stack) 110: returns (Stack[] memory, Stack memory newStack) /// @audit newSlot 424 function _createLien( 425 LienStorage storage s, 426 ILienToken.LienActionEncumber memory params 427: ) internal returns (uint256 newLienId, ILienToken.Stack memory newSlot) { /// @audit owed /// @audit buyout 577 function getBuyout(Stack calldata stack) 578 public 579 view 580: returns (uint256 owed, uint256 buyout) /// @audit newStack 596 function makePayment( 597 uint256 collateralId, 598 Stack[] calldata stack, 599 uint256 amount 600 ) 601 public 602 validateStack(collateralId, stack) 603: returns (Stack[] memory newStack) /// @audit maxPotentialDebt 706 function getMaxPotentialDebtForCollateral(Stack[] memory stack) 707 public 708 view 709 validateStack(stack[0].lien.collateralId, stack) 710: returns (uint256 maxPotentialDebt)
File: src/PublicVault.sol /// @audit assets 138 function redeemFutureEpoch( 139 uint256 shares, 140 address receiver, 141 address owner, 142 uint64 epoch 143: ) public virtual returns (uint256 assets) { /// @audit timeToSecondEpochEnd 713 function _timeToSecondEndIfPublic() 714 internal 715 view 716 override 717: returns (uint256 timeToSecondEpochEnd)
File: src/VaultImplementation.sol /// @audit timeToSecondEpochEnd 353 function _timeToSecondEndIfPublic() 354 internal 355 view 356 virtual 357: returns (uint256 timeToSecondEpochEnd)
File: src/WithdrawProxy.sol /// @audit assets 132 function mint(uint256 shares, address receiver) 133 public 134 virtual 135 override(ERC4626Cloned, IERC4626) 136: returns (uint256 assets) /// @audit shares 163 function withdraw( 164 uint256 assets, 165 address receiver, 166 address owner 167 ) 168 public 169 virtual 170 override(ERC4626Cloned, IERC4626) 171 onlyWhenNoActiveAuction 172: returns (uint256 shares) /// @audit assets 184 function redeem( 185 uint256 shares, 186 address receiver, 187 address owner 188 ) 189 public 190 virtual 191 override(ERC4626Cloned, IERC4626) 192 onlyWhenNoActiveAuction 193: returns (uint256 assets)
require()/revert() checks should be refactored to a modifier or functionThe compiler will inline the function, which will avoid JUMP instructions usually associated with functions
There are 6 instances of this issue:
File: lib/gpl/src/ERC721.sol 200: require(to != address(0), "INVALID_RECIPIENT");
File: src/AstariaRouter.sol 347: require(msg.sender == s.guardian);
File: src/ClearingHouse.sol 216: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));
File: src/PublicVault.sol 259: require(s.allowList[receiver]); 687 require( 688 currentEpoch != 0 && 689 msg.sender == s.epochData[currentEpoch - 1].withdrawProxy 690: );
File: src/VaultImplementation.sol 96: require(msg.sender == owner()); //owner is "strategist"
delete rather than assigning zero to clear valuesThe delete keyword more closely matches the semantics of what is being done, and draws more attention to the changing of state, which may lead to a more thorough audit of its associated logic
There are 5 instances of this issue:
File: src/PublicVault.sol 308: s.liquidationWithdrawRatio = 0; 327: s.withdrawReserve = 0; 377: s.withdrawReserve = 0; 511: s.strategistUnclaimedShares = 0;
File: src/WithdrawProxy.sol 284: s.finalAuctionEnd = 0;
Use alternative variants, e.g. allowlist/denylist instead of whitelist/blacklist
There is 1 instance of this issue:
File: src/AstariaRouter.sol 709: * @param allowListEnabled Whether or not the Vault has an LP whitelist.
Assembly blocks are take a lot more time to audit than normal Solidity code, and often have gotchas and side-effects that the Solidity versions of the same code do not. Consider adding more comments explaining what is being done in every step of the assembly code
There is 1 instance of this issue:
File: src/AstariaRouter.sol 414 assembly { 415 let end := add(ONE_WORD, start) 416 417 if lt(length, end) { 418 mstore(0, OUTOFBOUND_ERROR_SELECTOR) 419 revert(0, ONE_WORD) 420 } 421 422 x := mload(add(bs, end)) 423: }
While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
There is 1 instance of this issue:
File: Various Files
Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement fuzzing tests. Fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.
There is 1 instance of this issue:
File: Various Files
According to the Solidity style guide, functions should be laid out in the following order :constructor(), receive(), fallback(), external, public, internal, private, but the cases below do not follow this pattern
There are 69 instances of this issue:
File: lib/gpl/src/ERC20-Cloned.sol /// @audit _loadERC20Slot() came earlier 35: function balanceOf(address account) external view returns (uint256) { /// @audit transfer() came earlier 67 function allowance(address owner, address spender) 68 external 69 view 70: returns (uint256)
File: lib/gpl/src/ERC4626Router.sol /// @audit pullToken() came earlier 24 function depositToVault( 25 IERC4626 vault, 26 address to, 27 uint256 amount, 28 uint256 minSharesOut 29: ) external payable override returns (uint256 sharesOut) {
File: lib/gpl/src/ERC721.sol /// @audit isApprovedForAll() came earlier 38: function tokenURI(uint256 id) external view virtual returns (string memory); /// @audit _loadERC721Slot() came earlier 52: function ownerOf(uint256 id) public view virtual returns (address owner) { /// @audit __initERC721() came earlier 79: function name() public view returns (string memory) { /// @audit symbol() came earlier 87: function approve(address spender, uint256 id) external virtual { /// @audit _transfer() came earlier 148 function safeTransferFrom( 149 address from, 150 address to, 151: uint256 id /// @audit _safeMint() came earlier 277 function onERC721Received( 278 address, 279 address, 280 uint256, 281 bytes calldata 282: ) external virtual returns (bytes4) {
File: src/AstariaRouter.sol /// @audit _loadRouterSlot() came earlier 224: function feeTo() public view returns (address) { /// @audit COLLATERAL_TOKEN() came earlier 252: function __emergencyPause() external requiresAuth whenNotPaused { /// @audit _file() came earlier 339: function setNewGuardian(address _guardian) external { /// @audit _sliceUint() came earlier 426 function validateCommitment( 427 IAstariaRouter.Commitment calldata commitment, 428 uint256 timeToSecondEpochEnd 429: ) public view returns (ILienToken.Lien memory lien) { /// @audit _validateCommitment() came earlier 490 function commitToLiens(IAstariaRouter.Commitment[] memory commitments) 491 public 492 whenNotPaused 493: returns (uint256[] memory lienIds, ILienToken.Stack[] memory stack) /// @audit commitToLiens() came earlier 522 function newVault(address delegate, address underlying) 523 external 524 whenNotPaused 525: returns (address) /// @audit newPublicVault() came earlier 577 function requestLienPosition( 578 IAstariaRouter.Commitment calldata params, 579 address receiver 580 ) 581 external 582 whenNotPaused 583 validVault(msg.sender) 584 returns ( 585 uint256, 586 ILienToken.Stack[] memory, 587: uint256 /// @audit liquidate() came earlier 650: function getProtocolFee(uint256 amountIn) external view returns (uint256) {
File: src/BeaconProxy.sol /// @audit _fallback() came earlier 69 fallback() external payable virtual { 70: _fallback();
File: src/ClearingHouse.sol /// @audit _getStorage() came earlier 66: function setAuctionData(ILienToken.AuctionData calldata auctionData) /// @audit _execute() came earlier 169 function safeTransferFrom( 170 address from, // the from is the offerer 171 address to, 172 uint256 identifier, 173 uint256 amount, 174: bytes calldata data //empty from seaport /// @audit safeBatchTransferFrom() came earlier 188 function onERC721Received( 189 address operator_, 190 address from_, 191 uint256 tokenId_, 192 bytes calldata data_ 193: ) external override returns (bytes4) {
File: src/CollateralToken.sol /// @audit SEAPORT() came earlier 109: function liquidatorNFTClaim(OrderParameters memory params) external { /// @audit _loadCollateralSlot() came earlier 157 function isValidOrder( 158 bytes32 orderHash, 159 address caller, 160 address offerer, 161 bytes32 zoneHash 162: ) external view returns (bytes4 validOrderMagicValue) { /// @audit supportsInterface() came earlier 196: function fileBatch(File[] calldata files) external requiresAuth { /// @audit _file() came earlier 270 function flashAction( 271 IFlashAction receiver, 272 uint256 collateralId, 273 bytes calldata data 274: ) external onlyOwner(collateralId) { /// @audit _releaseToAddress() came earlier 370: function getConduitKey() public view returns (bytes32) { /// @audit securityHooks() came earlier 416 function getClearingHouse(uint256 collateralId) 417 external 418 view 419: returns (ClearingHouse) /// @audit _generateValidOrderParameters() came earlier 477 function auctionVault(AuctionVaultParams calldata params) 478 external 479 requiresAuth 480: returns (OrderParameters memory orderParameters) /// @audit _listUnderlyingOnSeaport() came earlier 524: function settleAuction(uint256 collateralId) public { /// @audit _settleAuction() came earlier 553 function onERC721Received( 554 address, /* operator_ */ 555 address from_, 556 uint256 tokenId_, 557 bytes calldata // calldata data_ 558: ) external override whenNotPaused returns (bytes4) {
File: src/LienToken.sol /// @audit _loadLienStorageSlot() came earlier 82: function file(File calldata incoming) external requiresAuth { /// @audit supportsInterface() came earlier 107 function buyoutLien(ILienToken.LienActionBuyout calldata params) 108 external 109 validateStack(params.encumber.lien.collateralId, params.encumber.stack) 110: returns (Stack[] memory, Stack memory newStack) /// @audit _replaceStackAtPositionWithNewLien() came earlier 246: function getInterest(Stack calldata stack) public view returns (uint256) { /// @audit _getInterest() came earlier 277 function stopLiens( 278 uint256 collateralId, 279 uint256 auctionWindow, 280 Stack[] calldata stack, 281 address liquidator 282: ) external validateStack(collateralId, stack) requiresAuth { /// @audit _stopLiens() came earlier 348 function tokenURI(uint256 tokenId) 349 public 350 view 351 override(ERC721, IERC721) 352: returns (string memory) /// @audit _exists() came earlier 389 function createLien(ILienToken.LienActionEncumber memory params) 390 external 391 requiresAuth 392 validateStack(params.lien.collateralId, params.stack) 393 returns ( 394 uint256 lienId, 395 Stack[] memory newStack, 396: uint256 lienSlope /// @audit _appendStack() came earlier 497 function payDebtViaClearingHouse( 498 address token, 499 uint256 collateralId, 500 uint256 payment, 501: AuctionStack[] memory auctionStack /// @audit _payDebt() came earlier 531 function getAuctionData(uint256 collateralId) 532 external 533 view 534: returns (AuctionData memory) /// @audit validateLien() came earlier 569 function getCollateralState(uint256 collateralId) 570 external 571 view 572: returns (bytes32) /// @audit _getBuyout() came earlier 596 function makePayment( 597 uint256 collateralId, 598 Stack[] calldata stack, 599 uint256 amount 600 ) 601 public 602 validateStack(collateralId, stack) 603: returns (Stack[] memory newStack) /// @audit makePayment() came earlier 608 function makePayment( 609 uint256 collateralId, 610 Stack[] calldata stack, 611 uint8 position, 612 uint256 amount 613 ) 614 external 615 validateStack(collateralId, stack) 616: returns (Stack[] memory newStack) /// @audit _updateCollateralStateHash() came earlier 702: function calculateSlope(Stack memory stack) public pure returns (uint256) { /// @audit _getMaxPotentialDebtForCollateralUpToNPositions() came earlier 727 function getMaxPotentialDebtForCollateral(Stack[] memory stack, uint256 end) 728 public 729 view 730 validateStack(stack[0].lien.collateralId, stack) 731: returns (uint256 maxPotentialDebt) /// @audit getMaxPotentialDebtForCollateral() came earlier 742: function getOwed(Stack memory stack) external view returns (uint88) { /// @audit _isPublicVault() came earlier 893: function getPayee(uint256 lienId) public view returns (address) {
File: src/PublicVault.sol /// @audit _redeemFutureEpoch() came earlier 192: function getWithdrawProxy(uint64 epoch) public view returns (WithdrawProxy) { /// @audit _deployWithdrawProxyIfNotDeployed() came earlier 233 function mint(uint256 shares, address receiver) 234 public 235 override(ERC4626Cloned) 236 whenNotPaused 237: returns (uint256) /// @audit computeDomainSeparator() came earlier 275 function processEpoch() public { 276 // check to make sure epoch is over 277: if (timeToEpochEnd() > 0) { /// @audit _afterCommitToLien() came earlier 461: function accrue() public returns (uint256) { /// @audit _accrue() came earlier 479 function totalAssets() 480 public 481 view 482 virtual 483 override(ERC4626Cloned) 484: returns (uint256) /// @audit _totalAssets() came earlier 495 function totalSupply() 496 public 497 view 498 virtual 499 override(IERC20, ERC20Cloned) 500: returns (uint256) /// @audit totalSupply() came earlier 507 function claim() external { 508: require(msg.sender == owner()); //owner is "strategist" /// @audit _setSlope() came earlier 534: function decreaseEpochLienCount(uint64 epoch) public onlyLienToken { /// @audit _decreaseEpochLienCount() came earlier 546: function getLienEpoch(uint64 end) public pure returns (uint64) { /// @audit _increaseOpenLiens() came earlier 562: function afterPayment(uint256 computedSlope) public onlyLienToken { /// @audit _handleStrategistInterestReward() came earlier 611: function LIEN_TOKEN() public view returns (ILienToken) { /// @audit handleBuyoutLien() came earlier 632 function updateAfterLiquidationPayment( 633 LiquidationPaymentParams calldata params 634: ) external onlyLienToken { /// @audit _setYIntercept() came earlier 699: function timeToEpochEnd() public view returns (uint256) { /// @audit _timeToSecondEndIfPublic() came earlier 722: function timeToSecondEpochEnd() public view returns (uint256) {
File: src/VaultImplementation.sol /// @audit _loadVISlot() came earlier 95: function modifyAllowList(address depositor, bool enabled) external virtual { /// @audit domainSeparator() came earlier 170 function encodeStrategyData( 171 IAstariaRouter.StrategyDetailsParam calldata strategy, 172 bytes32 root 173: ) external view returns (bytes memory) { /// @audit _encodeStrategyData() came earlier 190: function init(InitParams calldata params) external virtual { /// @audit _beforeCommitToLien() came earlier 287 function commitToLien( 288 IAstariaRouter.Commitment calldata params, 289 address receiver 290 ) 291 external 292 whenNotPaused 293: returns (uint256 lienId, ILienToken.Stack[] memory stack, uint256 payout) /// @audit _timeToSecondEndIfPublic() came earlier 366: function recipient() public view returns (address) {
File: src/Vault.sol /// @audit deposit() came earlier 70: function withdraw(uint256 amount) external {
File: src/WithdrawProxy.sol /// @audit redeem() came earlier 198 function supportsInterface(bytes4 interfaceId) 199 external 200 view 201 virtual 202: returns (bool) /// @audit _loadSlot() came earlier 215: function getFinalAuctionEnd() public view returns (uint256) { /// @audit getExpected() came earlier 235: function increaseWithdrawReserveReceived(uint256 amount) external onlyVault {
File: src/WithdrawVaultBase.sol /// @audit symbol() came earlier 26: function ROUTER() external pure returns (IAstariaRouter) {
The style guide says that, within a contract, the ordering should be 1) Type declarations, 2) State variables, 3) Events, 4) Modifiers, and 5) Functions, but the contract(s) below do not follow this ordering
There are 21 instances of this issue:
File: src/AstariaRouter.sol /// @audit function redeemFutureEpoch came earlier 196 modifier validVault(address targetVault) { 197 if (!isValidVault(targetVault)) { 198 revert InvalidVault(targetVault); 199 } 200 _; 201: }
File: src/CollateralToken.sol /// @audit function _file came earlier 253 modifier releaseCheck(uint256 collateralId) { 254 CollateralStorage storage s = _loadCollateralSlot(); 255 256 if (s.LIEN_TOKEN.getCollateralState(collateralId) != bytes32(0)) { 257 revert InvalidCollateralState(InvalidCollateralStates.ACTIVE_LIENS); 258 } 259 if (s.collateralIdToAuction[collateralId] != bytes32(0)) { 260 revert InvalidCollateralState(InvalidCollateralStates.AUCTION_ACTIVE); 261 } 262 _; 263: } /// @audit function onERC721Received came earlier 602 modifier whenNotPaused() { 603 if (_loadCollateralSlot().ASTARIA_ROUTER.paused()) { 604 revert ProtocolPaused(); 605 } 606 _; 607: }
File: src/interfaces/IAstariaRouter.sol /// @audit event FileUpdated came earlier 87 enum ImplementationType { 88 PrivateVault, 89 PublicVault, 90 WithdrawProxy, 91 ClearingHouse 92: } /// @audit function isValidRefinance came earlier 294: event Liquidation(uint256 collateralId, uint256 position); /// @audit event NewVault came earlier 316 enum LienState { 317 HEALTHY, 318 AUCTION 319: }
File: src/interfaces/ICollateralToken.sol /// @audit event ReleaseTo came earlier 74 enum FileType { 75 NotSupported, 76 AstariaRouter, 77 SecurityHook, 78 FlashEnabled, 79 Seaport 80: } /// @audit function liquidatorNFTClaim came earlier 175 enum InvalidCollateralStates { 176 NO_AUTHORITY, 177 NO_AUCTION, 178 FLASH_DISABLED, 179 AUCTION_ACTIVE, 180 INVALID_AUCTION_PARAMS, 181 ACTIVE_LIENS 182: }
File: src/interfaces/ILienToken.sol /// @audit function file came earlier 294 event AddLien( 295 uint256 indexed collateralId, 296 uint8 position, 297 uint256 indexed lienId, 298 Stack stack 299: ); /// @audit event AddLien came earlier 300 enum StackAction { 301 CLEAR, 302 ADD, 303 REMOVE, 304 REPLACE 305: } /// @audit event PayeeChanged came earlier 324 enum InvalidStates { 325 NO_AUTHORITY, 326 COLLATERAL_MISMATCH, 327 ASSET_MISMATCH, 328 NOT_ENOUGH_FUNDS, 329 INVALID_LIEN_ID, 330 COLLATERAL_AUCTION, 331 COLLATERAL_NOT_DEPOSITED, 332 LIEN_NO_DEBT, 333 EXPIRED_LIEN, 334 DEBT_LIMIT, 335 MAX_LIENS, 336 INVALID_HASH, 337 INVALID_LIQUIDATION_INITIAL_ASK, 338 INITIAL_ASK_EXCEEDED, 339 EMPTY_STATE, 340 PUBLIC_VAULT_RECIPIENT, 341 COLLATERAL_NOT_LIQUIDATED 342: }
File: src/interfaces/IPublicVault.sol /// @audit function updateVaultAfterLiquidation came earlier 157 enum InvalidStates { 158 EPOCH_TOO_LOW, 159 EPOCH_TOO_HIGH, 160 EPOCH_NOT_OVER, 161 WITHDRAW_RESERVE_NOT_ZERO, 162 LIENS_OPEN_FOR_EPOCH_NOT_ZERO, 163 LIQUIDATION_ACCOUNTANT_FINAL_AUCTION_OPEN, 164 LIQUIDATION_ACCOUNTANT_ALREADY_DEPLOYED_FOR_EPOCH, 165 DEPOSIT_CAP_EXCEEDED 166: }
File: src/LienToken.sol /// @audit function _getInterest came earlier 265 modifier validateStack(uint256 collateralId, Stack[] memory stack) { 266 LienStorage storage s = _loadLienStorageSlot(); 267 bytes32 stateHash = s.collateralStateHash[collateralId]; 268 if (stateHash == bytes32(0) && stack.length != 0) { 269 revert InvalidState(InvalidStates.EMPTY_STATE); 270 } 271 if (stateHash != bytes32(0) && keccak256(abi.encode(stack)) != stateHash) { 272 revert InvalidState(InvalidStates.INVALID_HASH); 273 } 274 _; 275: }
File: src/PublicVault.sol /// @audit function _afterCommitToLien came earlier 459: event SlopeUpdated(uint48 newSlope); /// @audit function increaseYIntercept came earlier 679 modifier onlyLienToken() { 680 require(msg.sender == address(LIEN_TOKEN())); 681 _; 682: }
File: src/VaultImplementation.sol /// @audit function symbol came earlier 57 uint256 private constant VI_SLOT = 58: uint256(keccak256("xyz.astaria.VaultImplementation.storage.location")) - 1; /// @audit function onERC721Received came earlier 131 modifier whenNotPaused() { 132 if (ROUTER().paused()) { 133 revert InvalidRequest(InvalidRequestReason.PAUSED); 134 } 135 136 if (_loadVISlot().isShutdown) { 137 revert InvalidRequest(InvalidRequestReason.SHUTDOWN); 138 } 139 _; 140: }
File: src/WithdrawProxy.sol /// @audit event Claimed came earlier 49 uint256 private constant WITHDRAW_PROXY_SLOT = 50: uint256(keccak256("xyz.astaria.WithdrawProxy.storage.location")) - 1; /// @audit variable WITHDRAW_PROXY_SLOT came earlier 59 enum InvalidStates { 60 PROCESS_EPOCH_NOT_COMPLETE, 61 FINAL_AUCTION_NOT_OVER, 62 NOT_CLAIMED, 63 CANT_CLAIM 64: } /// @audit function deposit came earlier 152 modifier onlyWhenNoActiveAuction() { 153 WPStorage storage s = _loadSlot(); 154 // If auction funds have been collected to the WithdrawProxy 155 // but the PublicVault hasn't claimed its share, too much money will be sent to LPs 156 if (s.finalAuctionEnd != 0) { 157 // if finalAuctionEnd is 0, no auctions were added 158 revert InvalidState(InvalidStates.NOT_CLAIMED); 159 } 160 _; 161: } /// @audit function getExpected came earlier 230 modifier onlyVault() { 231 require(msg.sender == VAULT(), "only vault can call"); 232 _; 233: }
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | |
|---|---|---|
| [Lā01] | safeApprove() is deprecated | 6 |
Total: 6 instances over 1 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | require()/revert() statements should have descriptive reason strings | 29 |
| [Nā02] | public functions not called by the contract should be declared external instead | 52 |
| [Nā03] | constants should be defined rather than using magic numbers | 21 |
| [Nā04] | Event is missing indexed fields | 32 |
Total: 134 instances over 4 issues
safeApprove() is deprecatedDeprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead. The function may currently work, but if a bug is found in this version of OpenZeppelin, and the version that you're forced to upgrade to no longer has this function, you'll encounter unnecessary delays in porting and testing replacement contracts.
There are 6 instances of this issue:
File: lib/gpl/src/ERC4626RouterBase.sol /// @audit (valid but excluded finding) 21: ERC20(vault.asset()).safeApprove(address(vault), shares); /// @audit (valid but excluded finding) 34: ERC20(vault.asset()).safeApprove(address(vault), amount); /// @audit (valid but excluded finding) 48: ERC20(address(vault)).safeApprove(address(vault), amount); /// @audit (valid but excluded finding) 62: ERC20(address(vault)).safeApprove(address(vault), shares);
File: src/ClearingHouse.sol /// @audit (valid but excluded finding) 148: ERC20(paymentToken).safeApprove(
File: src/VaultImplementation.sol /// @audit (valid but excluded finding) 334: ERC20(asset()).safeApprove(address(ROUTER().TRANSFER_PROXY()), buyout);
require()/revert() statements should have descriptive reason stringsThere are 29 instances of this issue:
File: src/AstariaRouter.sol /// @audit (valid but excluded finding) 341: require(msg.sender == s.guardian); /// @audit (valid but excluded finding) 347: require(msg.sender == s.guardian); /// @audit (valid but excluded finding) 354: require(msg.sender == s.newGuardian); /// @audit (valid but excluded finding) 361: require(msg.sender == address(s.guardian));
File: src/ClearingHouse.sol /// @audit (valid but excluded finding) 72: require(msg.sender == address(ASTARIA_ROUTER.LIEN_TOKEN())); /// @audit (valid but excluded finding) 199: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN())); /// @audit (valid but excluded finding) 216: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN())); /// @audit (valid but excluded finding) 223: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));
File: src/CollateralToken.sol /// @audit (valid but excluded finding) 266: require(ownerOf(collateralId) == msg.sender); /// @audit (valid but excluded finding) 535: require(msg.sender == s.clearingHouse[collateralId]); /// @audit (valid but excluded finding) 564: require(ERC721(msg.sender).ownerOf(tokenId_) == address(this)); /// @audit (valid but excluded finding) 598: revert();
File: src/LienToken.sol /// @audit (valid but excluded finding) 504 require( 505 msg.sender == address(s.COLLATERAL_TOKEN.getClearingHouse(collateralId)) 506: ); /// @audit (valid but excluded finding) 860: require(position < length);
File: src/PublicVault.sol /// @audit (valid but excluded finding) 241: require(s.allowList[receiver]); /// @audit (valid but excluded finding) 259: require(s.allowList[receiver]); /// @audit (valid but excluded finding) 508: require(msg.sender == owner()); //owner is "strategist" /// @audit (valid but excluded finding) 672 require( 673 currentEpoch != 0 && 674 msg.sender == s.epochData[currentEpoch - 1].withdrawProxy 675: ); /// @audit (valid but excluded finding) 680: require(msg.sender == address(LIEN_TOKEN())); /// @audit (valid but excluded finding) 687 require( 688 currentEpoch != 0 && 689 msg.sender == s.epochData[currentEpoch - 1].withdrawProxy 690: );
File: src/VaultImplementation.sol /// @audit (valid but excluded finding) 78: require(msg.sender == owner()); //owner is "strategist" /// @audit (valid but excluded finding) 96: require(msg.sender == owner()); //owner is "strategist" /// @audit (valid but excluded finding) 105: require(msg.sender == owner()); //owner is "strategist" /// @audit (valid but excluded finding) 114: require(msg.sender == owner()); //owner is "strategist" /// @audit (valid but excluded finding) 147: require(msg.sender == owner()); //owner is "strategist" /// @audit (valid but excluded finding) 191: require(msg.sender == address(ROUTER())); /// @audit (valid but excluded finding) 211: require(msg.sender == owner()); //owner is "strategist"
File: src/Vault.sol /// @audit (valid but excluded finding) 65: require(s.allowList[msg.sender] && receiver == owner()); /// @audit (valid but excluded finding) 71: require(msg.sender == owner());
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There are 52 instances of this issue:
File: src/AstariaRouter.sol /// @audit (valid but excluded finding) 224: function feeTo() public view returns (address) { /// @audit (valid but excluded finding) 229: function BEACON_PROXY_IMPLEMENTATION() public view returns (address) { /// @audit (valid but excluded finding) 234: function LIEN_TOKEN() public view returns (ILienToken) { /// @audit (valid but excluded finding) 239: function TRANSFER_PROXY() public view returns (ITransferProxy) { /// @audit (valid but excluded finding) 273: function file(File calldata incoming) public requiresAuth { /// @audit (valid but excluded finding) 402: function getAuctionWindow(bool includeBuffer) public view returns (uint256) { /// @audit (valid but excluded finding) 426 function validateCommitment( 427 IAstariaRouter.Commitment calldata commitment, 428 uint256 timeToSecondEpochEnd 429: ) public view returns (ILienToken.Lien memory lien) { /// @audit (valid but excluded finding) 490 function commitToLiens(IAstariaRouter.Commitment[] memory commitments) 491 public 492 whenNotPaused 493: returns (uint256[] memory lienIds, ILienToken.Stack[] memory stack) /// @audit (valid but excluded finding) 544 function newPublicVault( 545 uint256 epochLength, 546 address delegate, 547 address underlying, 548 uint256 vaultFee, 549 bool allowListEnabled, 550 address[] calldata allowList, 551 uint256 depositCap 552: ) public whenNotPaused returns (address) { /// @audit (valid but excluded finding) 621 function liquidate(ILienToken.Stack[] memory stack, uint8 position) 622 public 623: returns (OrderParameters memory listedOrder) /// @audit (valid but excluded finding) 684 function isValidRefinance( 685 ILienToken.Lien calldata newLien, 686 uint8 position, 687 ILienToken.Stack[] calldata stack 688: ) public view returns (bool) {
File: src/ClearingHouse.sol /// @audit (valid but excluded finding) 169 function safeTransferFrom( 170 address from, // the from is the offerer 171 address to, 172 uint256 identifier, 173 uint256 amount, 174: bytes calldata data //empty from seaport /// @audit (valid but excluded finding) 180 function safeBatchTransferFrom( 181 address from, 182 address to, 183 uint256[] calldata ids, 184 uint256[] calldata amounts, 185: bytes calldata data
File: src/CollateralToken.sol /// @audit (valid but excluded finding) 80 function initialize( 81 Authority AUTHORITY_, 82 ITransferProxy TRANSFER_PROXY_, 83 ILienToken LIEN_TOKEN_, 84 ConsiderationInterface SEAPORT_ 85: ) public initializer { /// @audit (valid but excluded finding) 105: function SEAPORT() public view returns (ConsiderationInterface) { /// @audit (valid but excluded finding) 206: function file(File calldata incoming) public requiresAuth { /// @audit (valid but excluded finding) 331 function releaseToAddress(uint256 collateralId, address releaseTo) 332 public 333 releaseCheck(collateralId) 334: onlyOwner(collateralId) /// @audit (valid but excluded finding) 370: function getConduitKey() public view returns (bytes32) { /// @audit (valid but excluded finding) 375: function getConduit() public view returns (address) { /// @audit (valid but excluded finding) 412: function securityHooks(address target) public view returns (address) { /// @audit (valid but excluded finding) 524: function settleAuction(uint256 collateralId) public {
File: src/LienToken.sol /// @audit (valid but excluded finding) 59 function initialize(Authority _AUTHORITY, ITransferProxy _TRANSFER_PROXY) 60 public 61: initializer /// @audit (valid but excluded finding) 246: function getInterest(Stack calldata stack) public view returns (uint256) { /// @audit (valid but excluded finding) 377: function ASTARIA_ROUTER() public view returns (IAstariaRouter) { /// @audit (valid but excluded finding) 550 function getAmountOwingAtLiquidation(ILienToken.Stack calldata stack) 551 public 552 view 553: returns (uint256) /// @audit (valid but excluded finding) 577 function getBuyout(Stack calldata stack) 578 public 579 view 580: returns (uint256 owed, uint256 buyout) /// @audit (valid but excluded finding) 596 function makePayment( 597 uint256 collateralId, 598 Stack[] calldata stack, 599 uint256 amount 600 ) 601 public 602 validateStack(collateralId, stack) 603: returns (Stack[] memory newStack) /// @audit (valid but excluded finding) 706 function getMaxPotentialDebtForCollateral(Stack[] memory stack) 707 public 708 view 709 validateStack(stack[0].lien.collateralId, stack) 710: returns (uint256 maxPotentialDebt) /// @audit (valid but excluded finding) 727 function getMaxPotentialDebtForCollateral(Stack[] memory stack, uint256 end) 728 public 729 view 730 validateStack(stack[0].lien.collateralId, stack) 731: returns (uint256 maxPotentialDebt) /// @audit (valid but excluded finding) 893: function getPayee(uint256 lienId) public view returns (address) {
File: src/PublicVault.sol /// @audit (valid but excluded finding) 192: function getWithdrawProxy(uint64 epoch) public view returns (WithdrawProxy) { /// @audit (valid but excluded finding) 196: function getCurrentEpoch() public view returns (uint64) { /// @audit (valid but excluded finding) 200: function getSlope() public view returns (uint256) { /// @audit (valid but excluded finding) 204: function getWithdrawReserve() public view returns (uint256) { /// @audit (valid but excluded finding) 208: function getLiquidationWithdrawRatio() public view returns (uint256) { /// @audit (valid but excluded finding) 212: function getYIntercept() public view returns (uint256) { /// @audit (valid but excluded finding) 461: function accrue() public returns (uint256) { /// @audit (valid but excluded finding) 534: function decreaseEpochLienCount(uint64 epoch) public onlyLienToken { /// @audit (valid but excluded finding) 552: function getEpochEnd(uint256 epoch) public pure returns (uint64) { /// @audit (valid but excluded finding) 562: function afterPayment(uint256 computedSlope) public onlyLienToken { /// @audit (valid but excluded finding) 615 function handleBuyoutLien(BuyoutLienParams calldata params) 616 public 617: onlyLienToken /// @audit (valid but excluded finding) 640 function updateVaultAfterLiquidation( 641 uint256 maxAuctionWindow, 642 AfterLiquidationParams calldata params 643: ) public onlyLienToken returns (address withdrawProxyIfNearBoundary) { /// @audit (valid but excluded finding) 669: function increaseYIntercept(uint256 amount) public { /// @audit (valid but excluded finding) 684: function decreaseYIntercept(uint256 amount) public { /// @audit (valid but excluded finding) 722: function timeToSecondEpochEnd() public view returns (uint256) {
File: src/WithdrawProxy.sol /// @audit (valid but excluded finding) 215: function getFinalAuctionEnd() public view returns (uint256) { /// @audit (valid but excluded finding) 220: function getWithdrawRatio() public view returns (uint256) { /// @audit (valid but excluded finding) 225: function getExpected() public view returns (uint256) { /// @audit (valid but excluded finding) 240 function claim() public { 241: WPStorage storage s = _loadSlot(); /// @audit (valid but excluded finding) 289 function drain(uint256 amount, address withdrawProxy) 290 public 291 onlyVault 292: returns (uint256) /// @audit (valid but excluded finding) 302: function setWithdrawRatio(uint256 liquidationWithdrawRatio) public onlyVault { /// @audit (valid but excluded finding) 306 function handleNewLiquidation( 307 uint256 newLienExpectedValue, 308 uint256 finalAuctionDelta 309: ) public onlyVault {
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 21 instances of this issue:
File: src/AstariaRouter.sol /// @audit 130 - (valid but excluded finding) 110: s.liquidationFeeNumerator = uint32(130); /// @audit 1e15 - (valid but excluded finding) /// @audit 5 - (valid but excluded finding) 112: s.minInterestBPS = uint32((uint256(1e15) * 5) / (365 days)); /// @audit 45 - (valid but excluded finding) 114: s.maxEpochLength = uint32(45 days); /// @audit 1e16 - (valid but excluded finding) /// @audit 200 - (valid but excluded finding) 115: s.maxInterestRate = ((uint256(1e16) * 200) / (365 days)).safeCastTo88();
File: src/AstariaVaultBase.sol /// @audit 20 - (valid but excluded finding) 33: return _getArgUint8(20); //ends at 21 /// @audit 21 - (valid but excluded finding) 37: return _getArgAddress(21); //ends at 44 /// @audit 41 - (valid but excluded finding) 47: return _getArgAddress(41); //ends at 64 /// @audit 61 - (valid but excluded finding) 51: return _getArgUint256(61); /// @audit 93 - (valid but excluded finding) 55: return _getArgUint256(93); //ends at 116 /// @audit 125 - (valid but excluded finding) 59: return _getArgUint256(125);
File: src/BeaconProxy.sol /// @audit 20 - (valid but excluded finding) 62: _delegate(_getBeacon().getImpl(_getArgUint8(20)));
File: src/ClearingHouse.sol /// @audit 21 - (valid but excluded finding) 48: return _getArgUint256(21); /// @audit 20 - (valid but excluded finding) 52: return _getArgUint8(20); /// @audit 21 - (valid but excluded finding) 136: uint256 collateralId = _getArgUint256(21);
File: src/PublicVault.sol /// @audit 18 - (valid but excluded finding) 103: if (ERC20(asset()).decimals() == uint8(18)) {
File: src/WithdrawVaultBase.sol /// @audit 20 - (valid but excluded finding) 31: return _getArgUint8(20); /// @audit 21 - (valid but excluded finding) 35: return _getArgAddress(21); /// @audit 41 - (valid but excluded finding) 39: return _getArgAddress(41); /// @audit 61 - (valid but excluded finding) 43: return _getArgUint64(61);
indexed fieldsIndex 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.
There are 32 instances of this issue:
File: src/interfaces/IAstariaRouter.sol /// @audit (valid but excluded finding) 54: event FileUpdated(FileType what, bytes data); /// @audit (valid but excluded finding) 294: event Liquidation(uint256 collateralId, uint256 position); /// @audit (valid but excluded finding) 295 event NewVault( 296 address strategist, 297 address delegate, 298 address vault, 299 uint8 vaultType 300: );
File: src/interfaces/ICollateralToken.sol /// @audit (valid but excluded finding) 32: event ListedOnSeaport(uint256 collateralId, Order listingOrder); /// @audit (valid but excluded finding) 33: event FileUpdated(FileType what, bytes data); /// @audit (valid but excluded finding) 40 event ReleaseTo( 41 address indexed underlyingAsset, 42 uint256 assetId, 43 address indexed to 44: );
File: src/interfaces/IERC1155.sol /// @audit (valid but excluded finding) 42 event ApprovalForAll( 43 address indexed account, 44 address indexed operator, 45 bool approved 46: ); /// @audit (valid but excluded finding) 55: event URI(string value, uint256 indexed id);
File: src/interfaces/IERC20.sol /// @audit (valid but excluded finding) 16: event Transfer(address indexed from, address indexed to, uint256 value); /// @audit (valid but excluded finding) 22: event Approval(address indexed owner, address indexed spender, uint256 value);
File: src/interfaces/IERC4626.sol /// @audit (valid but excluded finding) 15 event Deposit( 16 address indexed sender, 17 address indexed owner, 18 uint256 assets, 19 uint256 shares 20: );
File: src/interfaces/IERC721.sol /// @audit (valid but excluded finding) 14 event ApprovalForAll( 15 address indexed owner, 16 address indexed operator, 17 bool approved 18: );
File: src/interfaces/ILienToken.sol /// @audit (valid but excluded finding) 34: event FileUpdated(FileType what, bytes data); /// @audit (valid but excluded finding) 294 event AddLien( 295 uint256 indexed collateralId, 296 uint8 position, 297 uint256 indexed lienId, 298 Stack stack 299: ); /// @audit (valid but excluded finding) 306 event LienStackUpdated( 307 uint256 indexed collateralId, 308 uint8 position, 309 StackAction action, 310 uint8 stackLength 311: ); /// @audit (valid but excluded finding) 313: event Payment(uint256 indexed lienId, uint256 amount); /// @audit (valid but excluded finding) 314: event BuyoutLien(address indexed buyer, uint256 lienId, uint256 buyout);
File: src/interfaces/IPublicVault.sol /// @audit (valid but excluded finding) 168: event StrategistFee(uint88 feeInShares); /// @audit (valid but excluded finding) 169: event LiensOpenForEpochRemaining(uint64 epoch, uint256 liensOpenForEpoch); /// @audit (valid but excluded finding) 170: event YInterceptChanged(uint88 newYintercept); /// @audit (valid but excluded finding) 171: event WithdrawReserveTransferred(uint256 amount); /// @audit (valid but excluded finding) 172: event LienOpen(uint256 lienId, uint256 epoch);
File: src/interfaces/IV3PositionManager.sol /// @audit (valid but excluded finding) 10 event IncreaseLiquidity( 11 uint256 indexed tokenId, 12 uint128 liquidity, 13 uint256 amount0, 14 uint256 amount1 15: ); /// @audit (valid but excluded finding) 21 event DecreaseLiquidity( 22 uint256 indexed tokenId, 23 uint128 liquidity, 24 uint256 amount0, 25 uint256 amount1 26: ); /// @audit (valid but excluded finding) 33 event Collect( 34 uint256 indexed tokenId, 35 address recipient, 36 uint256 amount0, 37 uint256 amount1 38: );
File: src/interfaces/IVaultImplementation.sol /// @audit (valid but excluded finding) 52: event AllowListUpdated(address, bool); /// @audit (valid but excluded finding) 54: event AllowListEnabled(bool); /// @audit (valid but excluded finding) 56: event DelegateUpdated(address); /// @audit (valid but excluded finding) 58: event NonceUpdated(uint256 nonce); /// @audit (valid but excluded finding) 60: event IncrementNonce(uint256 nonce);
File: src/PublicVault.sol /// @audit (valid but excluded finding) 459: event SlopeUpdated(uint48 newSlope);
File: src/WithdrawProxy.sol /// @audit (valid but excluded finding) 42 event Claimed( 43 address withdrawProxy, 44 uint256 withdrawProxyAmount, 45 address publicVault, 46 uint256 publicVaultAmount 47: );
#0 - c4-judge
2023-01-26T14:10:33Z
Picodes marked the issue as grade-a
š Selected for report: c3phas
Also found by: 0x1f8b, 0xAgro, 0xSmartContract, 0xackermann, 0xkato, Aymen0909, Bnke0x0, CloudX, IllIllI, PaludoX0, Rageur, Rahoz, RaymondFam, ReyAdmirado, Rolezn, SadBase, SaeedAlipoor01988, caventa, chaduke, chrisdior4, fatherOfBlocks, fs0c, kaden, nogo, pfapostol, shark, synackrst
36.79 USDC - $36.79
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | Structs can be packed into fewer storage slots | 2 | - |
| [Gā02] | Using storage instead of memory for structs/arrays saves gas | 2 | 8400 |
| [Gā03] | The result of function calls should be cached rather than re-calling the function | 3 | - |
| [Gā04] | internal functions only called once can be inlined to save gas | 21 | 420 |
| [Gā05] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement | 1 | 85 |
| [Gā06] | keccak256() should only need to be called on a specific string literal once | 3 | 126 |
| [Gā07] | Optimize names to save gas | 18 | 396 |
| [Gā08] | Use a more recent version of solidity | 3 | - |
| [Gā09] | Remove unused local variable | 1 | - |
| [Gā10] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 3 | 15 |
| [Gā11] | Splitting require() statements that use && saves gas | 4 | 12 |
| [Gā12] | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 2 | - |
| [Gā13] | Inverting the condition of an if-else-statement wastes gas | 1 | - |
| [Gā14] | require() or revert() statements that check input arguments should be at the top of the function | 1 | - |
| [Gā15] | Use custom errors rather than revert()/require() strings to save gas | 8 | - |
| [Gā16] | Functions guaranteed to revert when called by normal users can be marked payable | 21 | 441 |
Total: 94 instances over 16 issues with 9895 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings
There are 2 instances of this issue:
File: src/interfaces/IAstariaRouter.sol /// @audit Variable ordering with 11 slots instead of the current 12: /// mapping(32):strategyValidators, mapping(32):implementations, mapping(32):vaults, user-defined(20):WETH, uint88(11):maxInterestRate, address(20):COLLATERAL_TOKEN, uint32(4):auctionWindow, uint32(4):auctionWindowBuffer, uint32(4):liquidationFeeNumerator, address(20):LIEN_TOKEN, uint32(4):liquidationFeeDenominator, uint32(4):maxEpochLength, uint32(4):minEpochLength, address(20):TRANSFER_PROXY, uint32(4):protocolFeeNumerator, uint32(4):protocolFeeDenominator, uint32(4):minInterestBPS, address(20):feeTo, uint32(4):buyoutFeeNumerator, uint32(4):buyoutFeeDenominator, uint32(4):minDurationIncrease, address(20):BEACON_PROXY_IMPLEMENTATION, address(20):guardian, address(20):newGuardian 56 struct RouterStorage { 57 //slot 1 58 uint32 auctionWindow; 59 uint32 auctionWindowBuffer; 60 uint32 liquidationFeeNumerator; 61 uint32 liquidationFeeDenominator; 62 uint32 maxEpochLength; 63 uint32 minEpochLength; 64 uint32 protocolFeeNumerator; 65 uint32 protocolFeeDenominator; 66 //slot 2 67 ERC20 WETH; //20 68 ICollateralToken COLLATERAL_TOKEN; //20 69 ILienToken LIEN_TOKEN; //20 70 ITransferProxy TRANSFER_PROXY; //20 71 address feeTo; //20 72 address BEACON_PROXY_IMPLEMENTATION; //20 73 uint88 maxInterestRate; //6 74 uint32 minInterestBPS; // was uint64 75 //slot 3 + 76 address guardian; //20 77 address newGuardian; //20 78 uint32 buyoutFeeNumerator; 79 uint32 buyoutFeeDenominator; 80 uint32 minDurationIncrease; 81 mapping(uint8 => address) strategyValidators; 82 mapping(uint8 => address) implementations; 83 //A strategist can have many deployed vaults 84 mapping(address => bool) vaults; 85: } /// @audit Variable ordering with 2 slots instead of the current 3: /// uint256(32):deadline, address(20):vault, uint8(1):version 101 struct StrategyDetailsParam { 102 uint8 version; 103 uint256 deadline; 104 address vault; 105: }
storage instead of memory for structs/arrays saves gasWhen 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
There are 2 instances of this issue:
File: src/CollateralToken.sol 137: Asset memory underlying = s.idToUnderlying[collateralId]; 562: Asset memory incomingAsset = s.idToUnderlying[collateralId];
The instances below point to the second+ call of the function within a single function
There are 3 instances of this issue:
File: src/ClearingHouse.sol /// @audit ASTARIA_ROUTER.COLLATERAL_TOKEN() on line 162 166: ASTARIA_ROUTER.COLLATERAL_TOKEN().settleAuction(collateralId); /// @audit ASTARIA_ROUTER.COLLATERAL_TOKEN() on line 199 204: ASTARIA_ROUTER.COLLATERAL_TOKEN().getConduit(), /// @audit ASTARIA_ROUTER.COLLATERAL_TOKEN() on line 199 207: ASTARIA_ROUTER.COLLATERAL_TOKEN().SEAPORT().validate(listings);
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 21 instances of this issue:
File: src/AstariaRouter.sol 407 function _sliceUint(bytes memory bs, uint256 start) 408 internal 409 pure 410: returns (uint256 x) 761 function _executeCommitment( 762 RouterStorage storage s, 763 IAstariaRouter.Commitment memory c 764 ) 765 internal 766 returns ( 767 uint256, 768 ILienToken.Stack[] memory stack, 769: uint256 payout 788 function _transferAndDepositAssetIfAble( 789 RouterStorage storage s, 790 address tokenContract, 791: uint256 tokenId
File: src/BeaconProxy.sol 20: function _getBeacon() internal pure returns (IBeacon) {
File: src/ClearingHouse.sol 114 function _execute( 115 address tokenContract, // collateral token sending the fake nft 116 address to, // buyer 117 uint256 encodedMetaData, //retrieve token address from the encoded data 118: uint256 // space to encode whatever is needed,
File: src/CollateralToken.sol 425 function _generateValidOrderParameters( 426 CollateralStorage storage s, 427 address settlementToken, 428 uint256 collateralId, 429 uint256[] memory prices, 430 uint256 maxDuration 431: ) internal returns (OrderParameters memory orderParameters) { 502 function _listUnderlyingOnSeaport( 503 CollateralStorage storage s, 504 uint256 collateralId, 505: Order memory listingOrder 541: function _settleAuction(CollateralStorage storage s, uint256 collateralId)
File: src/LienToken.sol 122 function _buyoutLien( 123 LienStorage storage s, 124 ILienToken.LienActionBuyout calldata params 125: ) internal returns (Stack[] memory newStack, Stack memory newLien) { 233 function _replaceStackAtPositionWithNewLien( 234 LienStorage storage s, 235 ILienToken.Stack[] calldata stack, 236 uint256 position, 237 Stack memory newLien, 238 uint256 oldLienId 239: ) internal returns (ILienToken.Stack[] memory newStack) { 292 function _stopLiens( 293 LienStorage storage s, 294 uint256 collateralId, 295 uint256 auctionWindow, 296 Stack[] calldata stack, 297: address liquidator 459 function _appendStack( 460 LienStorage storage s, 461 Stack[] memory stack, 462 Stack memory newSlot 463: ) internal returns (Stack[] memory newStack) { 512 function _payDebt( 513 LienStorage storage s, 514 address token, 515 uint256 payment, 516 address payer, 517 AuctionStack[] memory stack 518: ) internal returns (uint256 totalSpent) { 623 function _paymentAH( 624 LienStorage storage s, 625 address token, 626 AuctionStack[] memory stack, 627 uint256 position, 628 uint256 payment, 629 address payer 630: ) internal returns (uint256) { 663 function _makePayment( 664 LienStorage storage s, 665 Stack[] calldata stack, 666 uint256 totalCapitalAvailable 667: ) internal returns (Stack[] memory newStack) { 715 function _getMaxPotentialDebtForCollateralUpToNPositions( 716 Stack[] memory stack, 717 uint256 n 718: ) internal pure returns (uint256 maxPotentialDebt) { 775 function _getRemainingInterest(LienStorage storage s, Stack memory stack) 776 internal 777 view 778: returns (uint256) 855 function _removeStackPosition(Stack[] memory stack, uint8 position) 856 internal 857: returns (Stack[] memory newStack) 911 function _setPayee( 912 LienStorage storage s, 913 uint256 lienId, 914: address newPayee
File: src/PublicVault.sol 556: function _increaseOpenLiens(VaultData storage s, uint64 epoch) internal { 713 function _timeToSecondEndIfPublic() 714 internal 715 view 716 override 717: returns (uint256 timeToSecondEpochEnd)
unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statementrequire(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }
There is 1 instance of this issue:
File: src/WithdrawProxy.sol /// @audit if-condition on line 258 260: (s.expected - balance).mulWadDown(1e18 - s.withdrawRatio)
keccak256() should only need to be called on a specific string literal onceIt should be saved to an immutable variable, and the variable used instead. If the hash is being used as a part of a function selector, the cast to bytes4 should also only be done once
There are 3 instances of this issue:
File: lib/gpl/src/ERC20-Cloned.sol 162 keccak256( 163 "EIP712Domain(string version,uint256 chainId,address verifyingContract)" 164: ), 165: keccak256("1"),
File: src/CollateralToken.sol 310: ) != keccak256("FlashAction.onFlashAction")
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There are 18 instances of this issue:
File: lib/gpl/src/interfaces/IERC4626Router.sol /// @audit depositToVault(), depositMax(), redeemMax() 10: interface IERC4626Router {
File: src/AstariaRouter.sol /// @audit initialize(), redeemFutureEpoch(), feeTo(), BEACON_PROXY_IMPLEMENTATION(), LIEN_TOKEN(), TRANSFER_PROXY(), COLLATERAL_TOKEN(), __emergencyPause(), __emergencyUnpause(), fileBatch(), file(), setNewGuardian(), __renounceGuardian(), __acceptGuardian(), fileGuardian(), getImpl(), getAuctionWindow(), validateCommitment(), commitToLiens(), newVault(), newPublicVault(), requestLienPosition(), canLiquidate(), liquidate(), getProtocolFee(), getLiquidatorFee(), getBuyoutFee(), isValidVault(), isValidRefinance() 50: contract AstariaRouter is
File: src/AstariaVaultBase.sol /// @audit ROUTER(), IMPL_TYPE(), START(), EPOCH_LENGTH(), VAULT_FEE(), COLLATERAL_TOKEN() 23: abstract contract AstariaVaultBase is Clone, IAstariaVaultBase {
File: src/ClearingHouse.sol /// @audit ROUTER(), COLLATERAL_ID(), IMPL_TYPE(), setAuctionData(), validateOrder(), transferUnderlying(), settleLiquidatorNFTClaim() 32: contract ClearingHouse is AmountDeriver, Clone, IERC1155, IERC721Receiver {
File: src/CollateralToken.sol /// @audit initialize(), SEAPORT(), liquidatorNFTClaim(), isValidOrder(), isValidOrderIncludingExtraData(), fileBatch(), file(), flashAction(), releaseToAddress(), getConduitKey(), getConduit(), getUnderlying(), securityHooks(), getClearingHouse(), auctionVault(), settleAuction() 63: contract CollateralToken is
File: src/interfaces/IAstariaRouter.sol /// @audit validateCommitment(), newPublicVault(), newVault(), feeTo(), commitToLiens(), requestLienPosition(), LIEN_TOKEN(), TRANSFER_PROXY(), BEACON_PROXY_IMPLEMENTATION(), COLLATERAL_TOKEN(), getAuctionWindow(), getProtocolFee(), getBuyoutFee(), getLiquidatorFee(), liquidate(), canLiquidate(), isValidVault(), fileBatch(), file(), setNewGuardian(), fileGuardian(), getImpl(), isValidRefinance() 28: interface IAstariaRouter is IPausable, IBeacon {
File: src/interfaces/IAstariaVaultBase.sol /// @audit COLLATERAL_TOKEN(), START(), EPOCH_LENGTH(), VAULT_FEE() 20: interface IAstariaVaultBase is IRouterBase {
File: src/interfaces/ICollateralToken.sol /// @audit fileBatch(), file(), flashAction(), securityHooks(), getConduit(), getConduitKey(), getClearingHouse(), auctionVault(), settleAuction(), SEAPORT(), getUnderlying(), releaseToAddress(), liquidatorNFTClaim() 31: interface ICollateralToken is IERC721 {
File: src/interfaces/ILienToken.sol /// @audit validateLien(), ASTARIA_ROUTER(), COLLATERAL_TOKEN(), calculateSlope(), stopLiens(), getBuyout(), getOwed(), getOwed(), getInterest(), getCollateralState(), getAmountOwingAtLiquidation(), createLien(), buyoutLien(), payDebtViaClearingHouse(), makePayment(), makePayment(), getAuctionData(), getAuctionLiquidator(), getMaxPotentialDebtForCollateral(), getMaxPotentialDebtForCollateral(), getPayee(), file() 22: interface ILienToken is IERC721 {
File: src/interfaces/IPublicVault.sol /// @audit updateAfterLiquidationPayment(), redeemFutureEpoch(), beforePayment(), decreaseEpochLienCount(), getLienEpoch(), afterPayment(), claim(), timeToEpochEnd(), timeToSecondEpochEnd(), transferWithdrawReserve(), processEpoch(), increaseYIntercept(), decreaseYIntercept(), handleBuyoutLien(), updateVaultAfterLiquidation() 19: interface IPublicVault is IVaultImplementation {
File: src/interfaces/IRouterBase.sol /// @audit ROUTER(), IMPL_TYPE() 18: interface IRouterBase {
File: src/interfaces/IVaultImplementation.sol /// @audit getShutdown(), incrementNonce(), commitToLien(), buyoutLien(), recipient(), setDelegate(), init(), encodeStrategyData(), domainSeparator(), modifyDepositCap(), getStrategistNonce(), STRATEGY_TYPEHASH() 21: interface IVaultImplementation is IAstariaVaultBase, IERC165 {
File: src/interfaces/IWithdrawProxy.sol /// @audit VAULT(), CLAIMABLE_EPOCH(), setWithdrawRatio(), handleNewLiquidation(), drain(), claim(), increaseWithdrawReserveReceived(), getExpected(), getWithdrawRatio(), getFinalAuctionEnd() 21: interface IWithdrawProxy is IRouterBase, IERC165, IERC4626 {
File: src/LienToken.sol /// @audit initialize(), file(), buyoutLien(), getInterest(), stopLiens(), ASTARIA_ROUTER(), COLLATERAL_TOKEN(), createLien(), payDebtViaClearingHouse(), getAuctionData(), getAuctionLiquidator(), getAmountOwingAtLiquidation(), validateLien(), getCollateralState(), getBuyout(), makePayment(), makePayment(), calculateSlope(), getMaxPotentialDebtForCollateral(), getMaxPotentialDebtForCollateral(), getOwed(), getOwed(), getPayee() 44: contract LienToken is ERC721, ILienToken, AuthInitializable {
File: src/PublicVault.sol /// @audit redeemFutureEpoch(), getWithdrawProxy(), getCurrentEpoch(), getSlope(), getWithdrawReserve(), getLiquidationWithdrawRatio(), getYIntercept(), processEpoch(), transferWithdrawReserve(), accrue(), claim(), beforePayment(), decreaseEpochLienCount(), getLienEpoch(), getEpochEnd(), afterPayment(), LIEN_TOKEN(), handleBuyoutLien(), updateAfterLiquidationPayment(), updateVaultAfterLiquidation(), increaseYIntercept(), decreaseYIntercept(), timeToEpochEnd(), timeToEpochEnd(), timeToSecondEpochEnd() 48: contract PublicVault is VaultImplementation, IPublicVault, ERC4626Cloned {
File: src/VaultImplementation.sol /// @audit getStrategistNonce(), incrementNonce(), modifyDepositCap(), modifyAllowList(), disableAllowList(), enableAllowList(), getShutdown(), domainSeparator(), encodeStrategyData(), init(), setDelegate(), commitToLien(), buyoutLien(), recipient() 34: abstract contract VaultImplementation is
File: src/WithdrawProxy.sol /// @audit getFinalAuctionEnd(), getWithdrawRatio(), getExpected(), increaseWithdrawReserveReceived(), claim(), drain(), setWithdrawRatio(), handleNewLiquidation() 37: contract WithdrawProxy is ERC4626Cloned, WithdrawVaultBase {
File: src/WithdrawVaultBase.sol /// @audit ROUTER(), VAULT(), CLAIMABLE_EPOCH() 21: abstract contract WithdrawVaultBase is Clone, IWithdrawProxy {
Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
There are 3 instances of this issue:
File: lib/gpl/src/interfaces/IMulticall.sol 4: pragma solidity >=0.7.5;
File: lib/gpl/src/interfaces/IUniswapV3Factory.sol 2: pragma solidity >=0.5.0;
File: lib/gpl/src/interfaces/IUniswapV3PoolState.sol 2: pragma solidity >=0.5.0;
There is 1 instance of this issue:
File: src/ClearingHouse.sol /// @audit stack 139: ILienToken.AuctionStack[] storage stack = s.auctionStack.stack;
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There are 3 instances of this issue:
File: lib/gpl/src/ERC721.sol 136: s._balanceOf[from]--; 223: s._balanceOf[owner]--;
File: src/PublicVault.sol 539: s.epochData[epoch].liensOpenForEpoch--;
require() statements that use && saves gasSee 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
There are 4 instances of this issue:
File: lib/gpl/src/ERC20-Cloned.sol 143 require( 144 recoveredAddress != address(0) && recoveredAddress == owner, 145 "INVALID_SIGNER" 146: );
File: src/PublicVault.sol 672 require( 673 currentEpoch != 0 && 674 msg.sender == s.epochData[currentEpoch - 1].withdrawProxy 675: ); 687 require( 688 currentEpoch != 0 && 689 msg.sender == s.epochData[currentEpoch - 1].withdrawProxy 690: );
File: src/Vault.sol 65: require(s.allowList[msg.sender] && receiver == owner());
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen 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
There are 2 instances of this issue:
File: src/AstariaRouter.sol /// @audit uint8 vaultType 725: vaultType = uint8(ImplementationType.PublicVault); /// @audit uint8 vaultType 727: vaultType = uint8(ImplementationType.PrivateVault);
if-else-statement wastes gasFlipping the true and false blocks instead saves 3 gas
There is 1 instance of this issue:
File: src/CollateralToken.sol 234 if (!exists) { 235 s.CONDUIT = s.CONDUIT_CONTROLLER.createConduit( 236 s.CONDUIT_KEY, 237 address(this) 238 ); 239 } else { 240 s.CONDUIT = conduit; 241: }
require() or revert() statements that check input arguments should be at the top of the functionChecks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas*) in a function that may ultimately revert in the unhappy case.
There is 1 instance of this issue:
File: lib/gpl/src/ERC721.sol /// @audit expensive op on line 111 115: require(to != address(0), "INVALID_RECIPIENT");
revert()/require() strings to save gasCustom errors are available from solidity version 0.8.4. 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
There are 8 instances of this issue:
File: lib/gpl/src/ERC20-Cloned.sol 143 require( 144 recoveredAddress != address(0) && recoveredAddress == owner, 145 "INVALID_SIGNER" 146: );
File: lib/gpl/src/ERC721.sol 53 require( 54 (owner = _loadERC721Slot()._ownerOf[id]) != address(0), 55 "NOT_MINTED" 56: ); 90 require( 91 msg.sender == owner || s.isApprovedForAll[owner][msg.sender], 92 "NOT_AUTHORIZED" 93: ); 117 require( 118 msg.sender == from || 119 s.isApprovedForAll[from][msg.sender] || 120 msg.sender == s.getApproved[id], 121 "NOT_AUTHORIZED" 122: ); 155 require( 156 to.code.length == 0 || 157 ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") == 158 ERC721TokenReceiver.onERC721Received.selector, 159 "UNSAFE_RECIPIENT" 160: ); 171 require( 172 to.code.length == 0 || 173 ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) == 174 ERC721TokenReceiver.onERC721Received.selector, 175 "UNSAFE_RECIPIENT" 176: ); 240 require( 241 to.code.length == 0 || 242 ERC721TokenReceiver(to).onERC721Received( 243 msg.sender, 244 address(0), 245 id, 246 "" 247 ) == 248 ERC721TokenReceiver.onERC721Received.selector, 249 "UNSAFE_RECIPIENT" 250: ); 260 require( 261 to.code.length == 0 || 262 ERC721TokenReceiver(to).onERC721Received( 263 msg.sender, 264 address(0), 265 id, 266 data 267 ) == 268 ERC721TokenReceiver.onERC721Received.selector, 269 "UNSAFE_RECIPIENT" 270: );
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 21 instances of this issue:
File: src/AstariaRouter.sol 252: function __emergencyPause() external requiresAuth whenNotPaused { 259: function __emergencyUnpause() external requiresAuth whenPaused { 263: function fileBatch(File[] calldata files) external requiresAuth { 273: function file(File calldata incoming) public requiresAuth {
File: src/CollateralToken.sol 196: function fileBatch(File[] calldata files) external requiresAuth { 206: function file(File calldata incoming) public requiresAuth { 270 function flashAction( 271 IFlashAction receiver, 272 uint256 collateralId, 273 bytes calldata data 274: ) external onlyOwner(collateralId) { 331 function releaseToAddress(uint256 collateralId, address releaseTo) 332 public 333 releaseCheck(collateralId) 334: onlyOwner(collateralId) 477 function auctionVault(AuctionVaultParams calldata params) 478 external 479 requiresAuth 480: returns (OrderParameters memory orderParameters)
File: src/LienToken.sol 82: function file(File calldata incoming) external requiresAuth { 277 function stopLiens( 278 uint256 collateralId, 279 uint256 auctionWindow, 280 Stack[] calldata stack, 281 address liquidator 282: ) external validateStack(collateralId, stack) requiresAuth { 389 function createLien(ILienToken.LienActionEncumber memory params) 390 external 391 requiresAuth 392 validateStack(params.lien.collateralId, params.stack) 393 returns ( 394 uint256 lienId, 395 Stack[] memory newStack, 396: uint256 lienSlope
File: src/PublicVault.sol 515 function beforePayment(BeforePaymentParams calldata params) 516 external 517: onlyLienToken 615 function handleBuyoutLien(BuyoutLienParams calldata params) 616 public 617: onlyLienToken 632 function updateAfterLiquidationPayment( 633 LiquidationPaymentParams calldata params 634: ) external onlyLienToken { 640 function updateVaultAfterLiquidation( 641 uint256 maxAuctionWindow, 642 AfterLiquidationParams calldata params 643: ) public onlyLienToken returns (address withdrawProxyIfNearBoundary) {
File: src/TransferProxy.sol 28 function tokenTransferFrom( 29 address token, 30 address from, 31 address to, 32 uint256 amount 33: ) external requiresAuth {
File: src/WithdrawProxy.sol 163 function withdraw( 164 uint256 assets, 165 address receiver, 166 address owner 167 ) 168 public 169 virtual 170 override(ERC4626Cloned, IERC4626) 171 onlyWhenNoActiveAuction 172: returns (uint256 shares) 184 function redeem( 185 uint256 shares, 186 address receiver, 187 address owner 188 ) 189 public 190 virtual 191 override(ERC4626Cloned, IERC4626) 192 onlyWhenNoActiveAuction 193: returns (uint256 assets) 289 function drain(uint256 amount, address withdrawProxy) 290 public 291 onlyVault 292: returns (uint256) 306 function handleNewLiquidation( 307 uint256 newLienExpectedValue, 308 uint256 finalAuctionDelta 309: ) public onlyVault {
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | Using calldata instead of memory for read-only arguments in external functions saves gas | 10 | 1200 |
| [Gā02] | <array>.length should not be looked up in every loop of a for-loop | 10 | 30 |
| [Gā03] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 5 | 25 |
| [Gā04] | Using private rather than public for constants, saves gas | 1 | - |
| [Gā05] | Use custom errors rather than revert()/require() strings to save gas | 15 | - |
| [Gā06] | Functions guaranteed to revert when called by normal users can be marked payable | 4 | 84 |
Total: 45 instances over 6 issues with 1339 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
calldata instead of memory for read-only arguments in external functions saves gasWhen a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.
If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one
Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved
There are 10 instances of this issue:
File: src/AstariaRouter.sol /// @audit commitments - (valid but excluded finding) 490 function commitToLiens(IAstariaRouter.Commitment[] memory commitments) 491 public 492 whenNotPaused 493: returns (uint256[] memory lienIds, ILienToken.Stack[] memory stack) /// @audit stack - (valid but excluded finding) 621 function liquidate(ILienToken.Stack[] memory stack, uint8 position) 622 public 623: returns (OrderParameters memory listedOrder)
File: src/ClearingHouse.sol /// @audit order - (valid but excluded finding) 197: function validateOrder(Order memory order) external {
File: src/CollateralToken.sol /// @audit params - (valid but excluded finding) 109: function liquidatorNFTClaim(OrderParameters memory params) external {
File: src/LienToken.sol /// @audit params - (valid but excluded finding) 389 function createLien(ILienToken.LienActionEncumber memory params) 390 external 391 requiresAuth 392 validateStack(params.lien.collateralId, params.stack) 393 returns ( 394 uint256 lienId, 395 Stack[] memory newStack, 396: uint256 lienSlope /// @audit auctionStack - (valid but excluded finding) 497 function payDebtViaClearingHouse( 498 address token, 499 uint256 collateralId, 500 uint256 payment, 501: AuctionStack[] memory auctionStack /// @audit stack - (valid but excluded finding) 706 function getMaxPotentialDebtForCollateral(Stack[] memory stack) 707 public 708 view 709 validateStack(stack[0].lien.collateralId, stack) 710: returns (uint256 maxPotentialDebt) /// @audit stack - (valid but excluded finding) 727 function getMaxPotentialDebtForCollateral(Stack[] memory stack, uint256 end) 728 public 729 view 730 validateStack(stack[0].lien.collateralId, stack) 731: returns (uint256 maxPotentialDebt) /// @audit stack - (valid but excluded finding) 742: function getOwed(Stack memory stack) external view returns (uint88) { /// @audit stack - (valid but excluded finding) 747 function getOwed(Stack memory stack, uint256 timestamp) 748 external 749 view 750: returns (uint88)
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)CALLDATALOAD (3 gas)Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There are 10 instances of this issue:
File: src/AstariaRouter.sol /// @audit (valid but excluded finding) 265: for (; i < files.length; ) { /// @audit (valid but excluded finding) 364: for (; i < file.length; ) { /// @audit (valid but excluded finding) 506: for (; i < commitments.length; ) {
File: src/ClearingHouse.sol /// @audit (valid but excluded finding) 96: for (uint256 i; i < accounts.length; ) {
File: src/CollateralToken.sol /// @audit (valid but excluded finding) 198: for (; i < files.length; ) {
File: src/LienToken.sol /// @audit (valid but excluded finding) 304: for (; i < stack.length; ) { /// @audit (valid but excluded finding) 520: for (; i < stack.length;) { /// @audit (valid but excluded finding) 669: for (uint256 i; i < newStack.length; ) { /// @audit (valid but excluded finding) 734: for (; i < stack.length; ) {
File: src/VaultImplementation.sol /// @audit (valid but excluded finding) 201: for (; i < params.allowList.length; ) {
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There are 5 instances of this issue:
File: lib/gpl/src/ERC721.sol /// @audit (valid but excluded finding) 138: s._balanceOf[to]++; /// @audit (valid but excluded finding) 206: s._balanceOf[to]++;
File: src/PublicVault.sol /// @audit (valid but excluded finding) 341: s.currentEpoch++; /// @audit (valid but excluded finding) 558: s.epochData[epoch].liensOpenForEpoch++;
File: src/VaultImplementation.sol /// @audit (valid but excluded finding) 69: s.strategistNonce++;
private rather than public for constants, saves gasIf needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table
There is 1 instance of this issue:
File: src/VaultImplementation.sol /// @audit (valid but excluded finding) 44 bytes32 public constant STRATEGY_TYPEHASH = 45: keccak256("StrategyDetails(uint256 nonce,uint256 deadline,bytes32 root)");
revert()/require() strings to save gasCustom errors are available from solidity version 0.8.4. 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
There are 15 instances of this issue:
File: lib/gpl/src/ERC20-Cloned.sol /// @audit (valid but excluded finding) 115: require(deadline >= block.timestamp, "PERMIT_DEADLINE_EXPIRED");
File: lib/gpl/src/ERC4626-Cloned.sol /// @audit (valid but excluded finding) 25: require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES"); /// @audit (valid but excluded finding) 27: require(shares > minDepositAmount(), "VALUE_TOO_SMALL"); /// @audit (valid but excluded finding) 43: require(assets > minDepositAmount(), "VALUE_TOO_SMALL"); /// @audit (valid but excluded finding) 94: require((assets = previewRedeem(shares)) != 0, "ZERO_ASSETS");
File: lib/gpl/src/ERC721.sol /// @audit (valid but excluded finding) 60: require(owner != address(0), "ZERO_ADDRESS"); /// @audit (valid but excluded finding) 113: require(from == s._ownerOf[id], "WRONG_FROM"); /// @audit (valid but excluded finding) 115: require(to != address(0), "INVALID_RECIPIENT"); /// @audit (valid but excluded finding) 200: require(to != address(0), "INVALID_RECIPIENT"); /// @audit (valid but excluded finding) 202: require(s._ownerOf[id] == address(0), "ALREADY_MINTED"); /// @audit (valid but excluded finding) 219: require(owner != address(0), "NOT_MINTED");
File: src/ClearingHouse.sol /// @audit (valid but excluded finding) 134: require(payment >= currentOfferPrice, "not enough funds received");
File: src/PublicVault.sol /// @audit (valid but excluded finding) 170: require((assets = previewRedeem(shares)) != 0, "ZERO_ASSETS");
File: src/WithdrawProxy.sol /// @audit (valid but excluded finding) 138: require(msg.sender == VAULT(), "only vault can mint"); /// @audit (valid but excluded finding) 231: require(msg.sender == VAULT(), "only vault can call");
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 4 instances of this issue:
File: src/PublicVault.sol /// @audit (valid but excluded finding) 534: function decreaseEpochLienCount(uint64 epoch) public onlyLienToken { /// @audit (valid but excluded finding) 562: function afterPayment(uint256 computedSlope) public onlyLienToken {
File: src/WithdrawProxy.sol /// @audit (valid but excluded finding) 235: function increaseWithdrawReserveReceived(uint256 amount) external onlyVault { /// @audit (valid but excluded finding) 302: function setWithdrawRatio(uint256 liquidationWithdrawRatio) public onlyVault {
#0 - c4-judge
2023-01-25T23:28:29Z
Picodes marked the issue as grade-b