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: 24/103
Findings: 2
Award: $616.50
🌟 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
| Number | Issues Details | Context |
|---|---|---|
| [L-01] | ERC4626 's implmentation is not fully up to EIP-4626's specification | 7 |
| [L-02] | Loss of precision due to rounding | 1 |
| [L-03] | Front running attacks by the onlyVault | 1 |
| [L-04] | Solmate's SafeTransferLib doesn't check whether the ERC20 contract exists | 9 |
| [L-05] | Signature Malleability of EVM's ecrecover() | 1 |
| [L-06] | Missing Event for critical parameters init and change | 5 |
| [L-07] | Use 2StepSetNewGuardian instead of setNewGuardian | 1 |
| [L-08] | Lack of Input Validation | 1 |
| [L-09] | Stack too deep when compiling | 1 |
| [L-10] | Require messages are too short or not | 22 |
| [L-11] | Allows malleable SECP256K1 signatures | 1 |
| [L-12] | Due to the novelty of the ERC4626 standard, it is safer to use as upgradeable | 1 |
| [L-13] | Type uint256 defined in LienOpen event is type uint64 defined in emit, so truncate happens | 1 |
Total 13 issues
| Number | Issues Details | Context |
|---|---|---|
| [NC-01] | Insufficient coverage | All Contracts |
| [NC-02] | Critical Address Changes Should Use Two-step Procedure | 1 |
| [NC-03] | Initial value check is missing in Set Functions | 4 |
| [NC-04] | NatSpec comments should be increased in contracts | All Contracts |
| [NC-05] | Function writing that does not comply with the Solidity Style Guide | All contracts |
| [NC-06] | Add a timelock to critical functions | 3 |
| [NC-07] | Include return parameters in NatSpec comments | All Contracts |
| [NC-08] | Keccak Constant values should used to immutable rather than constant | 14 |
| [NC-09] | Need Fuzzing test | 33 |
| [NC-10] | Omissions in Events | 6 |
| [NC-11] | Mark visibility of initialize(...) functions as external | 2 |
| [NC-12] | Use underscores for number literals | 1 |
| [NC-13] | Lack of event emission after critical initialize() function | 2 |
| [NC-14] | Empty blocks should be removed or Emit something | 1 |
| [NC-15] | Tokens accidentally sent to the contract cannot be recovered | 1 |
| [NC-16] | Assembly Codes Specific – Should Have Comments | 12 |
| [NC-17] | Use a single file for all system-wide constants | 19 |
| [NC-18] | Take advantage of Custom Error's return value property | 23 |
| [NC-19] | Add NatSpec Mapping comment | 13 |
| [NC-20] | Incorrect NatSpec comments | 1 |
| [NC-21] | There is no need to cast a variable that is already an address, such as address(x) | 1 |
| [NC-22] | Repeated validation logic can be converted into a function modifier | 8 |
| [NC-23] | ABIEncoderV2 is still valid, but it is deprecated and has no effect | 2 |
| [NC-24] | Avoid shadowing inherited state variables | 1 |
| [NC-25] | Defining the event in the middle of the contract lowers auditing and code readability | 3 |
| [NC-26] | Incorrect SLOT NatSpec comment in RouterStorage struct | 1 |
Total 26 issues
| Number | Suggestion Details |
|---|---|
| [S-01] | Project Upgrade and Stop Scenario should be |
| [S-02] | Use descriptive names for Contracts and Libraries |
Total 2 suggestions
The maxDeposit and maxMint functions from the EIP-4626's specification are not available
Contracts implementing the IERC4626 specification;
7 results - 7 files src/AstariaRouter.sol: 39: import {IERC4626} from "core/interfaces/IERC4626.sol"; src/AstariaVaultBase.sol: 18: import {IERC4626} from "core/interfaces/IERC4626.sol"; src/PublicVault.sol: 24: import {IERC4626} from "core/interfaces/IERC4626.sol"; src/WithdrawProxy.sol: 25: import {IERC4626} from "core/interfaces/IERC4626.sol"; src/WithdrawVaultBase.sol: 18: import {IERC4626} from "core/interfaces/IERC4626.sol"; src/interfaces/IAstariaRouter.sol: 18: import {IERC4626} from "core/interfaces/IERC4626.sol"; src/interfaces/IWithdrawProxy.sol: 17: import {IERC4626} from "core/interfaces/IERC4626.sol";
Functions to be added;
function maxDeposit(address) public view virtual returns (uint256) { return type(uint256).max; } function maxMint(address) public view virtual returns (uint256) { return type(uint256).max; }
Could cause unexpected behavior in the future due to non-compliance with EIP-4626 standard.
Similar problem for Sentimentxyz ; https://github.com/sentimentxyz/protocol/pull/235/files
maxMint() and maxDeposit() should reflect the limitation of maxSupply.
Consider changing maxMint() and maxDeposit() to:
function maxMint(address) public view virtual returns (uint256) { - return type(uint256).max; + if (totalSupply >= maxSupply) return 0; + return maxSupply - totalSupply; } function maxDeposit(address) public view virtual returns (uint256) { - return type(uint256).max; + return convertToAssets(maxMint(address(0))); }
Add scalar so roundings are negligible
src/AstariaRouter.sol: 112: s.minInterestBPS = uint32((uint256(1e15) * 5) / (365 days));
onlyVaultProject has one possible attack vectors by the onlyVault:
With the function setWithdrawRatio the following is set: Withdraw ratio
When a user uses the platform expecting a low Withdraw Ratio, the authorities can run the "setWithdrawRatio()" function up front and raise the price significantly. If the size is large enough, this can be a substantial amount of money.
src/WithdrawProxy.sol: 301 302: function setWithdrawRatio(uint256 liquidationWithdrawRatio) public onlyVault { 303: _loadSlot().withdrawRatio = liquidationWithdrawRatio.safeCastTo88(); 304: }
Use a timelock to avoid instant changes of the parameters.
Solmate's SafeTransferLib, which is often used to interact with non-compliant/unsafe ERC20 tokens, does not check whether the ERC20 contract exists. The following code will not revert in case the token doesn't exist (yet).
This is stated in the Solmate library: https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.sol#L9
9 results - 9 files src/AstariaRouter.sol: 19: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol"; 57: using SafeTransferLib for ERC20; src/ClearingHouse.sol: 19: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol"; 34: using SafeTransferLib for ERC20; src/CollateralToken.sol: 32: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol"; 70: using SafeTransferLib for ERC20; src/LienToken.sol: 36: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol"; 48: using SafeTransferLib for ERC20; src/PublicVault.sol: 19: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol"; 50: using SafeTransferLib for ERC20; src/TransferProxy.sol: 17: import {SafeTransferLib, ERC20} from "solmate/utils/SafeTransferLib.sol"; 22: using SafeTransferLib for ERC20; src/Vault.sol: 17: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol"; 26: using SafeTransferLib for ERC20; src/VaultImplementation.sol: 19: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol"; 39: using SafeTransferLib for ERC20; src/WithdrawProxy.sol: 18: import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol"; 38: using SafeTransferLib for ERC20;
Add a contract exist control in functions;
pragma solidity >=0.8.0; function isContract(address _addr) private returns (bool isContract) { isContract = _addr.code.length > 0; }
Context:
src/VaultImplementation.sol: 245 VIData storage s = _loadVISlot(); 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: );
Description: Description: The function calls the Solidity ecrecover() function directly to verify the given signatures. However, the ecrecover() EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks.
Although a replay attack seems not possible for this contract, I recommend using the battle-tested OpenZeppelin's ECDSA library.
Recommendation:* Use the ecrecover function from OpenZeppelin's ECDSA library for signature verification. (Ensure using a version > 4.7.3 for there was a critical bug >= 4.1.0 < 4.7.3).
Context:
src/TransferProxy.sol: 24: constructor(Authority _AUTHORITY) Auth(msg.sender, _AUTHORITY) { 25: //only constructor we care about is Auth src/AstariaRouter.sol: 83: function initialize( src/CollateralToken.sol: 80: function initialize( src/LienToken.sol: 59: function initialize(Authority _AUTHORITY, ITransferProxy _TRANSFER_PROXY) src/VaultImplementation.sol: 190: function init(InitParams calldata params) external virtual {
Description: Events help non-contract tools to track changes, and events prevent users from being surprised by changes
Recommendation: Add Event-Emit
2StepSetNewGuardian instead of setNewGuardiansrc/AstariaRouter.sol: 338 339: function setNewGuardian(address _guardian) external { 340: RouterStorage storage s = _loadRouterSlot(); 341: require(msg.sender == s.guardian); 342: s.newGuardian = _guardian; 343: }
Description:
setNewGuardian function is used to change guardian address
Use a 2 structure which is safer.
Recommendation: Use 2-stage guardian address transfer :
abstract contract 2StepSetNewGuardian { address private _pendingGuardian; // Guardian address address private guardian; address public newGuardian; event GuardianTransferStarted(address indexed previousGuardian, address indexed newGuardian); /** * @dev Returns the address of the pending Guardian. */ function pendingGuardian() public view returns (address) { return _pendingGuardian; } /** * @dev Starts the Guardian transfer of the contract to a new account. Replaces the pending transfer if there is one. * Can only be called by the current Guardian. */ function transferGuardian(address newGuardian) public onlyGuardian { _pendingGuardian = newGuardian; emit GuardianTransferStarted(guardian(), newGuardian); } /** * @dev Transfers Guardian of the contract to a new account (`newGuardian`) and deletes any pending Guardian. * Internal function without access restriction. */ function _transferGuardian(address newGuardian) internal { delete _pendingGuardian; transferGuardian(newGuardian); } /** * @dev The new Guardian accepts the Guardian transfer. */ function accept Guardian() external { address sender = _msgSender(); require(pendingGuardian() == sender, " 2StepSetGuardian : caller is not the new Guardian"); _transferGuardian(sender); } }
For defence-in-depth purpose, it is recommended to perform additional validation against the amount that the user is attempting to deposit, mint, withdraw and redeem to ensure that the submitted amount is valid.
OpenZeppelinTokenizedVault.sol#L9
src/PublicVault.sol: 147 148: function _redeemFutureEpoch( 149: VaultData storage s, 150: uint256 shares, 151: address receiver, 152: address owner, 153: uint64 epoch 154: ) internal virtual returns (uint256 assets) { 155: // check to ensure that the requested epoch is not in the past 156: 157: ERC20Data storage es = _loadERC20Slot(); 158: 159: if (msg.sender != owner) { 160: uint256 allowed = es.allowance[owner][msg.sender]; // Saves gas for limited approvals. 161: + require(shares <= maxRedeem(owner), "redeem more than max"); 162: if (allowed != type(uint256).max) { 163: es.allowance[owner][msg.sender] = allowed - shares; 164: } 165: } 166: 167: if (epoch < s.currentEpoch) { 168: revert InvalidState(InvalidStates.EPOCH_TOO_LOW); 169: } 170: require((assets = previewRedeem(shares)) != 0, "ZERO_ASSETS"); 171: // check for rounding error since we round down in previewRedeem. 172: 173: //this will underflow if not enough balance 174: es.balanceOf[owner] -= shares; 175: 176: // Cannot overflow because the sum of all user 177: // balances can't exceed the max uint256 value. 178: unchecked { 179: es.balanceOf[address(this)] += shares; 180: } 181: 182: emit Transfer(owner, address(this), shares); 183: // Deploy WithdrawProxy if no WithdrawProxy exists for the specified epoch 184: _deployWithdrawProxyIfNotDeployed(s, epoch); 185: 186: emit Withdraw(msg.sender, receiver, owner, assets, shares); 187: 188: // WithdrawProxy shares are minted 1:1 with PublicVault shares 189: WithdrawProxy(s.epochData[epoch].withdrawProxy).mint(shares, receiver); 190: }
The project cannot be compiled due to the "stack too deep" error.
The “stack too deep” error is a limitation of the current code generator. The EVM stack only has 16 slots and that’s sometimes not enough to fit all the local variables, parameters and/or return variables. The solution is to move some of them to memory, which is more expensive but at least makes your code compile.
[⠊] Compiling... [⠃] Compiling 202 files with 0.8.17 [⠒] Solc 0.8.17 finished in 6.03s Error: Compiler run failed CompilerError: Stack too deep. Try compiling with `--via-ir` (cli) or the equivalent `viaIR: true` (standard JSON) while enabling the optimizer. Otherwise, try removing local variables. --> lib/seaport/contracts/lib/FulfillmentApplier.sol:223:9: | 223 | assembly { | ^ (Relevant source part starts here and spans across multiple lines).
ref:https://forum.openzeppelin.com/t/stack-too-deep-when-compiling-inline-assembly/11391/6
Context:
Description: The correct and clear error description explains to the user why the function reverts, but the error descriptions below in the project are not self-explanatory. These error descriptions are very important in the debug features of DApps like Tenderly. Error definitions should be added to the require block, not exceeding 32 bytes.
22 results - 7 files src/AstariaRouter.sol: 341: require(msg.sender == s.guardian); 347: require(msg.sender == s.guardian); 354: require(msg.sender == s.newGuardian); 361: require(msg.sender == address(s.guardian)); src/AuthInitializable.sol: 85: require( src/ClearingHouse.sol: 72: require(msg.sender == address(ASTARIA_ROUTER.LIEN_TOKEN())); 199: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN())); 216: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN())); 223: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN())); src/CollateralToken.sol: 266: require(ownerOf(collateralId) == msg.sender); 535: require(msg.sender == s.clearingHouse[collateralId]); 564: require(ERC721(msg.sender).ownerOf(tokenId_) == address(this)); src/LienToken.sol: 504: require( 860: require(position < length); src/PublicVault.sol: 241: require(s.allowList[receiver]); 259: require(s.allowList[receiver]); 508: require(msg.sender == owner()); //owner is "strategist" 672: require( 680: require(msg.sender == address(LIEN_TOKEN())); 687: require( src/Vault.sol: 65: require(s.allowList[msg.sender] && receiver == owner()); 71: require(msg.sender == owner());
Here, the ecrecover() method doesn't check the s range.
Homestead (EIP-2) added this limitation, however the precompile remained unaltered. The majority of libraries, including OpenZeppelin, do this check. Since an order can only be confirmed once and its hash is saved, there doesn't seem to be a serious danger in existing use cases.
lib/gpl/src/ERC20-Cloned.sol: 105 106: function permit( 107: address owner, 108: address spender, 109: uint256 value, 110: uint256 deadline, 111: uint8 v, 112: bytes32 r, 113: bytes32 s 114: ) public virtual { 115: require(deadline >= block.timestamp, "PERMIT_DEADLINE_EXPIRED"); 116: 117: // Unchecked because the only math done is incrementing 118: // the owner's nonce which cannot realistically overflow. 119: 120: unchecked { 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: ); 142: 143: require( 144: recoveredAddress != address(0) && recoveredAddress == owner, 145: "INVALID_SIGNER" 146: ); 147: 148: _loadERC20Slot().allowance[recoveredAddress][spender] = value; 149: } 150: 151: emit Approval(owner, spender, value); 152: }
ERC-4626 is a standard to optimize and unify the technical parameters of yield-bearing vaults. It provides a standard API for tokenized yield-bearing vaults that represent shares of a single underlying ERC-20 token. ERC-4626 also outlines an optional extension for tokenized vaults utilizing ERC-20, offering basic functionality for depositing, withdrawing tokens and reading balances.
ERC4626 Tokenized Vauldt was created 2021-12-22 so this standart is safer to use as upgrade
LienOpen event is type uint64 defined in emit, so truncate happenshttps://github.com/code-423n4/2023-01-astaria/blob/main/src/interfaces/IPublicVault.sol#L172
src/PublicVault.sol: 172: event LienOpen(uint256 lienId, uint256 epoch); 438 */ 439: function _afterCommitToLien( 440: uint40 lienEnd, 441: uint256 lienId, 442: uint256 lienSlope 443: ) internal virtual override { 444: VaultData storage s = _loadStorageSlot(); 445: 446: // increment slope for the new lien 447: _accrue(s); 448: unchecked { 449: uint48 newSlope = s.slope + lienSlope.safeCastTo48(); 450: _setSlope(s, newSlope); 451: } 452: 453: uint64 epoch = getLienEpoch(lienEnd); 454: 455: _increaseOpenLiens(s, epoch); 456: emit LienOpen(lienId, epoch); 457: }
Recommended Mitigation Steps
- 172: event LienOpen(uint256 lienId, uint256 epoch); + 172: event LienOpen(uint256 lienId, uint64 epoch);
Description: The test coverage rate of the project is ~90%. Testing all functions is best practice in terms of security criteria.
README.md: 95: - What is the overall line coverage percentage provided by your tests?: ~90 (`forge coverage` currently throws a "stack too deep" error on large codebases)
Due to its capacity, test coverage is expected to be 100%.
The critical procedures should be two step process.
src/VaultImplementation.sol: 209 210: function setDelegate(address delegate_) external { 211: require(msg.sender == owner()); //owner is "strategist" 212: VIData storage s = _loadVISlot(); 213: s.delegate = delegate_; 214: emit DelegateUpdated(delegate_); 215: emit AllowListUpdated(delegate_, true); 216: }
Recommended Mitigation Steps Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
Context:
4 results - 4 files src/AstariaRouter.sol: 339: function setNewGuardian(address _guardian) external { src/ClearingHouse.sol: 66: function setAuctionData(ILienToken.AuctionData calldata auctionData) src/VaultImplementation.sol: 210: function setDelegate(address delegate_) external { src/WithdrawProxy.sol: 302: function setWithdrawRatio(uint256 liquidationWithdrawRatio) public onlyVault {
Checking whether the current value and the new value are the same should be added
Context: All Contracts
Description: It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability. https://docs.soliditylang.org/en/v0.8.15/natspec-format.html
Recommendation: NatSpec comments should be increased in contracts
Function writing that does not comply with the Solidity Style GuideContext: All Contracts
Description: Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.
https://docs.soliditylang.org/en/v0.8.17/style-guide.html
Functions should be grouped according to their visibility and ordered:
constructor receive function (if exists) fallback function (if exists) external public internal private within a grouping, place the view and pure functions last
It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user). Consider adding a timelock to:
3 results - 3 files src/AstariaRouter.sol: 339: function setNewGuardian(address _guardian) external { src/VaultImplementation.sol: 210: function setDelegate(address delegate_) external { src/WithdrawProxy.sol: 302: function setWithdrawRatio(uint256 liquidationWithdrawRatio) public onlyVault {
Context: All Contracts
Description: It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.
https://docs.soliditylang.org/en/v0.8.15/natspec-format.html
Recommendation: Include return parameters in NatSpec comments
Recommendation Code Style: (from Uniswap3)
/// @notice Adds liquidity for the given recipient/tickLower/tickUpper position /// @dev The caller of this method receives a callback in the form of IUniswapV3MintCallback#uniswapV3MintCallback /// in which they must pay any token0 or token1 owed for the liquidity. The amount of token0/token1 due depends /// on tickLower, tickUpper, the amount of liquidity, and the current price. /// @param recipient The address for which the liquidity will be created /// @param tickLower The lower tick of the position in which to add liquidity /// @param tickUpper The upper tick of the position in which to add liquidity /// @param amount The amount of liquidity to mint /// @param data Any data that should be passed through to the callback /// @return amount0 The amount of token0 that was paid to mint the given amount of liquidity. Matches the value in the callback /// @return amount1 The amount of token1 that was paid to mint the given amount of liquidity. Matches the value in the callback function mint( address recipient, int24 tickLower, int24 tickUpper, uint128 amount, bytes calldata data ) external returns (uint256 amount0, uint256 amount1);
There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts.
While 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.
14 results - 10 files src/AstariaRouter.sol: 62: uint256 private constant ROUTER_SLOT = src/ClearingHouse.sol: 40: uint256 private constant CLEARING_HOUSE_STORAGE_SLOT = src/CollateralToken.sol: 73: uint256 private constant COLLATERAL_TOKEN_SLOT = src/LienToken.sol: 50: uint256 private constant LIEN_SLOT = 53: bytes32 constant ACTIVE_AUCTION = bytes32("ACTIVE_AUCTION"); src/PublicVault.sol: 53: uint256 private constant PUBLIC_VAULT_SLOT = src/VaultImplementation.sol: 44: bytes32 public constant STRATEGY_TYPEHASH = 47: bytes32 constant EIP_DOMAIN = 51: bytes32 constant VERSION = keccak256("0"); 57: uint256 private constant VI_SLOT = src/WithdrawProxy.sol: 49: uint256 private constant WITHDRAW_PROXY_SLOT = src/actions/UNIV3/ClaimFees.sol: 23: bytes32 private constant FLASH_ACTION_MAGIC = src/utils/Initializable.sol: 331: uint256 private constant INITIALIZER_SLOT = src/utils/Pausable.sol: 20: uint256 private constant PAUSE_SLOT =
Context:
33 results - 7 files src/AstariaRouter.sol: 266 _file(files[i]); 267: unchecked { 268 ++i; 386 emit FileUpdated(what, data); 387: unchecked { 388 ++i; 512 totalBorrowed += payout; 513: unchecked { 514 ++i; src/ClearingHouse.sol: 97 output[i] = type(uint256).max; 98: unchecked { 99 ++i; src/CollateralToken.sol: 199 _file(files[i]); 200: unchecked { 201 ++i; src/LienToken.sol: 171 172: unchecked { 173 --i; 222 } 223: unchecked { 224 ++i; 329 } 330: unchecked { 331 ++i; 478 479: unchecked { 480 potentialDebt += _getOwed(newStack[j], newStack[j].point.end); 485 486: unchecked { 487 --i; 521 uint256 spent; 522: unchecked { 523 spent = _paymentAH(s, token, stack, i, payment, payer); 681 if (newStack.length == oldLength) { 682: unchecked { 683 ++i; 720 maxPotentialDebt += _getOwed(stack[i], stack[i].point.end); 721: unchecked { 722 ++i; 735 maxPotentialDebt += _getOwed(stack[i], end); 736: unchecked { 737 ++i; 864 newStack[i] = stack[i]; 865: unchecked { 866 ++i; 869 for (; i < length - 1; ) { 870: unchecked { 871 newStack[i] = stack[i + 1]; src/PublicVault.sol: 177 // balances can't exceed the max uint256 value. 178: unchecked { 179 es.balanceOf[address(this)] += shares; 320 321: unchecked { 322 if (totalAssets() > expected) { 339 // increment epoch 340: unchecked { 341 s.currentEpoch++; 378 } else { 379: unchecked { 380 s.withdrawReserve -= withdrawBalance.safeCastTo88(); 403 ); 404: unchecked { 405 s.withdrawReserve -= drainBalance.safeCastTo88(); 447 _accrue(s); 448: unchecked { 449 uint48 newSlope = s.slope + lienSlope.safeCastTo48(); 465 function _accrue(VaultData storage s) internal returns (uint256) { 466: unchecked { 467 s.yIntercept = (_totalAssets(s)).safeCastTo88(); 521 522: unchecked { 523 uint48 newSlope = s.slope - params.lienSlope.safeCastTo48(); 556 function _increaseOpenLiens(VaultData storage s, uint64 epoch) internal { 557: unchecked { 558 s.epochData[epoch].liensOpenForEpoch++; 563 VaultData storage s = _loadStorageSlot(); 564: unchecked { 565 s.slope += computedSlope.safeCastTo48(); 581 582: unchecked { 583 s.yIntercept += assets.safeCastTo88(); 620 621: unchecked { 622 uint48 newSlope = s.slope - params.lienSlope.safeCastTo48(); 646 _accrue(s); 647: unchecked { 648 _setSlope(s, s.slope - params.lienSlope.safeCastTo48()); src/VaultImplementation.sol: 202 s.allowList[params.allowList[i]] = true; 203: unchecked { 204 ++i; 402 403: unchecked { 404 amount -= fee; src/WithdrawProxy.sol: 275 276: unchecked { 277 balance -= transferAmount; 311 312: unchecked { 313 s.expected += newLienExpectedValue.safeCastTo88();
Description: In total 7 contracts, 33 unchecked are used, the functions used are critical. For this reason, there must be fuzzing tests in the tests of the project. Not seen in tests.
Recommendation: Use should fuzzing test like Echidna.
As Alberto Cuesta Canada said: Fuzzing is not easy, the tools are rough, and the math is hard, but it is worth it. Fuzzing gives me a level of confidence in my smart contracts that I didn’t have before. Relying just on unit testing anymore and poking around in a testnet seems reckless now.
https://medium.com/coinmonks/smart-contract-fuzzing-d9b88e0b0a05
Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, some events are missing important parameters
The events should include the new value and old value where possible:
6 results - 4 files src/ClearingHouse.sol: 66: function setAuctionData(ILienToken.AuctionData calldata auctionData) 104: function setApprovalForAll(address operator, bool approved) external {} 220: function settleLiquidatorNFTClaim() external { src/CollateralToken.sol: 524: function settleAuction(uint256 collateralId) public { src/VaultImplementation.sol: 210: function setDelegate(address delegate_) external { src/WithdrawProxy.sol: 302: function setWithdrawRatio(uint256 liquidationWithdrawRatio) public onlyVault {
externalsrc/CollateralToken.sol: 79 80: function initialize( 81: Authority AUTHORITY_, 82: ITransferProxy TRANSFER_PROXY_, 83: ILienToken LIEN_TOKEN_, 84: ConsiderationInterface SEAPORT_ 85: ) public initializer { 86: __initAuth(msg.sender, address(AUTHORITY_)); 87: __initERC721("Astaria Collateral Token", "ACT"); 88: CollateralStorage storage s = _loadCollateralSlot(); 89: s.TRANSFER_PROXY = TRANSFER_PROXY_; 90: s.LIEN_TOKEN = LIEN_TOKEN_; 91: s.SEAPORT = SEAPORT_; 92: (, , address conduitController) = s.SEAPORT.information(); 93: bytes32 CONDUIT_KEY = Bytes32AddressLib.fillLast12Bytes(address(this)); 94: s.CONDUIT_KEY = CONDUIT_KEY; 95: s.CONDUIT_CONTROLLER = ConduitControllerInterface(conduitController); 96: 97: s.CONDUIT = s.CONDUIT_CONTROLLER.createConduit(CONDUIT_KEY, address(this)); 98: s.CONDUIT_CONTROLLER.updateChannel( 99: address(s.CONDUIT), 100: address(SEAPORT_), 101: true 102: ); 103: } src/LienToken.sol: 58 59: function initialize(Authority _AUTHORITY, ITransferProxy _TRANSFER_PROXY) 60: public 61: initializer 62: { 63: __initAuth(msg.sender, address(_AUTHORITY)); 64: __initERC721("Astaria Lien Token", "ALT"); 65: LienStorage storage s = _loadLienStorageSlot(); 66: s.TRANSFER_PROXY = _TRANSFER_PROXY; 67: s.maxLiens = uint8(5); 68: }
Description: If someone wants to extend via inheritance, it might make more sense that the overridden initialize(...) function calls the internal {...}_init function, not the parent public initialize(...) function.
External instead of public would give more the sense of the initialize(...) functions to behave like a constructor (only called on deployment, so should only be called externally)
Security point of view, it might be safer so that it cannot be called internally by accident in the child contract
It might cost a bit less gas to use external over public
It is possible to override a function from external to public (= "opening it up") ✅ but it is not possible to override a function from public to external (= "narrow it down"). ❌
For above reasons you can change initialize(...) to external
https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3750
1 result - 1 file src/PublicVault.sol: - 604: uint256 fee = x.mulDivDown(VAULT_FEE(), 10000); + uint256 fee = x.mulDivDown(VAULT_FEE(), 10_000); 605 uint88 feeInShares = convertToShares(fee).safeCastTo88();
Description: There is occasions where certain number have been hardcoded, either in variable or in the code itself. Large numbers can become hard to read.
Recommendation: Consider using underscores for number literals to improve its readability.
initialize() functionTo record the init parameters for off-chain monitoring and transparency reasons, please consider emitting an event after the initialize() function:
src/CollateralToken.sol: 79 80: function initialize( 81: Authority AUTHORITY_, 82: ITransferProxy TRANSFER_PROXY_, 83: ILienToken LIEN_TOKEN_, 84: ConsiderationInterface SEAPORT_ 85: ) public initializer { 86: __initAuth(msg.sender, address(AUTHORITY_)); 87: __initERC721("Astaria Collateral Token", "ACT"); 88: CollateralStorage storage s = _loadCollateralSlot(); 89: s.TRANSFER_PROXY = TRANSFER_PROXY_; 90: s.LIEN_TOKEN = LIEN_TOKEN_; 91: s.SEAPORT = SEAPORT_; 92: (, , address conduitController) = s.SEAPORT.information(); 93: bytes32 CONDUIT_KEY = Bytes32AddressLib.fillLast12Bytes(address(this)); 94: s.CONDUIT_KEY = CONDUIT_KEY; 95: s.CONDUIT_CONTROLLER = ConduitControllerInterface(conduitController); 96: 97: s.CONDUIT = s.CONDUIT_CONTROLLER.createConduit(CONDUIT_KEY, address(this)); 98: s.CONDUIT_CONTROLLER.updateChannel( 99: address(s.CONDUIT), 100: address(SEAPORT_), 101: true 102: ); 103: } src/LienToken.sol: 58 59: function initialize(Authority _AUTHORITY, ITransferProxy _TRANSFER_PROXY) 60: public 61: initializer 62: { 63: __initAuth(msg.sender, address(_AUTHORITY)); 64: __initERC721("Astaria Lien Token", "ALT"); 65: LienStorage storage s = _loadLienStorageSlot(); 66: s.TRANSFER_PROXY = _TRANSFER_PROXY; 67: s.maxLiens = uint8(5); 68: }
Empty blocks should be removed or Emit somethingDescription: Code contains empty block
src/BeaconProxy.sol: 86 */ 87: function _beforeFallback() internal virtual {} 88 }
Recommendation: The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.
It can't be recovered if the tokens accidentally arrive at the contract address, which has happened to many popular projects, so I recommend adding a recovery code to your critical contracts.
Add this code:
/** * @notice Sends ERC20 tokens trapped in contract to external address * @dev Onlyowner is allowed to make this function call * @param account is the receiving address * @param externalToken is the token being sent * @param amount is the quantity being sent * @return boolean value indicating whether the operation succeeded. * */ function rescueERC20(address account, address externalToken, uint256 amount) public onlyOwner returns (bool) { IERC20(externalToken).transfer(account, amount); return true; } }
Since this is a low level language that is more difficult to parse by readers, include extensive documentation, comments on the rationale behind its use, clearly explaining what each assembly instruction does
This will make it easier for users to trust the code, for reviewers to validate the code, and for developers to build on or update the code.
Note that using Aseembly removes several important security features of Solidity, which can make the code more insecure and more error-prone.
12 results - 10 files src/AstariaRouter.sol: 219: assembly { 414: assembly { src/BeaconProxy.sol: 30: assembly { 31: // Copy msg.data. We take full control of memory in this inline assembly src/ClearingHouse.sol: 61: assembly { src/CollateralToken.sol: 152: assembly { src/LienToken.sol: 77: assembly { src/PublicVault.sol: 430: assembly { src/VaultImplementation.sol: 85: assembly { src/WithdrawProxy.sol: 210: assembly { src/libraries/CollateralLookup.sol: 24: assembly { src/utils/Pausable.sol: 40: assembly {
There are many addresses and constants used in the system. It is recommended to put the most used ones in one file (for example constants.sol, use inheritance to access these values)
This will help with readability and easier maintenance for future changes. This also helps with any issues, as some of these hard-coded values are admin addresses.
constants.sol Use and import this file in contracts that require access to these values. This is just a suggestion, in some use cases this may result in higher gas usage in the distribution.
19 results - 13 files src/AstariaRouter.sol: 62: uint256 private constant ROUTER_SLOT = 66: uint256 private constant OUTOFBOUND_ERROR_SELECTOR = 68: uint256 private constant ONE_WORD = 0x20; src/ClearingHouse.sol: 40: uint256 private constant CLEARING_HOUSE_STORAGE_SLOT = src/CollateralToken.sol: 73: uint256 private constant COLLATERAL_TOKEN_SLOT = src/LienToken.sol: 50: uint256 private constant LIEN_SLOT = 53: bytes32 constant ACTIVE_AUCTION = bytes32("ACTIVE_AUCTION"); src/PublicVault.sol: 53: uint256 private constant PUBLIC_VAULT_SLOT = src/VaultImplementation.sol: 44: bytes32 public constant STRATEGY_TYPEHASH = 47: bytes32 constant EIP_DOMAIN = 51: bytes32 constant VERSION = keccak256("0"); 57: uint256 private constant VI_SLOT = src/WithdrawProxy.sol: 49: uint256 private constant WITHDRAW_PROXY_SLOT = src/actions/UNIV3/ClaimFees.sol: 23: bytes32 private constant FLASH_ACTION_MAGIC = src/libraries/Base64.sol: 4: bytes internal constant TABLE = src/strategies/CollectionValidator.sol: 32: uint8 public constant VERSION_TYPE = uint8(2); src/strategies/UNI_V3Validator.sol: 57: uint8 public constant VERSION_TYPE = uint8(3); src/strategies/UniqueValidator.sol: 33: uint8 public constant VERSION_TYPE = uint8(1); src/utils/Pausable.sol: 20: uint256 private constant PAUSE_SLOT =
An important feature of Custom Error is that values such as address, tokenID, msg.value can
be written inside the () sign, this kind of approach provides a serious advantage in
debugging and examining the revert details of dapps such as tenderly.
23 results - 3 files src/AstariaRouter.sol: 293: if (denominator < numerator) revert InvalidFileData(); 301: if (denominator < numerator) revert InvalidFileData(); 309: if (denominator < numerator) revert InvalidFileData(); 326: if (addr == address(0)) revert InvalidFileData(); 330: if (addr == address(0)) revert InvalidFileData(); 333: revert UnsupportedFile(); 369: if (addr == address(0)) revert InvalidFileData(); 373: if (addr == address(0)) revert InvalidFileData(); 377: if (addr == address(0)) revert InvalidFileData(); 381: if (addr == address(0)) revert InvalidFileData(); src/CollateralToken.sol: 312: revert FlashActionCallbackFailed(); 319: revert FlashActionSecurityCheckFailed(); 327: revert FlashActionNFTNotReturned(); 339: revert InvalidSender(); 510: revert InvalidConduitKey(); 513: revert InvalidZone(); 585: revert InvalidCollateral(); 598: revert(); 604: revert ProtocolPaused(); src/LienToken.sol: 91: revert UnsupportedFile(); 117: revert InvalidSender(); 135: revert InvalidRefinance(); 806: revert InvalidLoanState();
Description: Add NatSpec comments describing mapping keys and values
Context:
13 results - 5 files src/interfaces/IAstariaRouter.sol: 81: mapping(uint8 => address) strategyValidators; 82: mapping(uint8 => address) implementations; 84: mapping(address => bool) vaults; src/interfaces/ICollateralToken.sol: 59: mapping(uint256 => bytes32) collateralIdToAuction; 60: mapping(address => bool) flashEnabled; 62: mapping(uint256 => Asset) idToUnderlying; 64: mapping(address => address) securityHooks; 65: mapping(uint256 => address) clearingHouse; src/interfaces/ILienToken.sol: 42: mapping(uint256 => bytes32) collateralStateHash; 43: mapping(uint256 => AuctionData) auctionData; 44: mapping(uint256 => LienMeta) lienMeta; src/interfaces/IPublicVault.sol: 33: mapping(uint64 => EpochData) epochData; src/interfaces/IVaultImplementation.sol: 49: mapping(address => bool) allowList;
Recommendation:
/// @dev address(user) -> address(ERC0 Contract Address) -> uint256(allowance amount from user) mapping(address => mapping(address => uint256)) public allowance;
This NatSpec comments are incorrect . Below code block has 2 slots comment but Natspec comment was declaration to only 1 slot(//slot2)
Context:
src/interfaces/IAstariaRouter.sol: 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 +
function pullToken( address token, uint256 amount, address recipient ) public payable override { RouterStorage storage s = _loadRouterSlot(); s.TRANSFER_PROXY.tokenTransferFrom( - address(token), + token msg.sender, recipient, amount ); }
Description: There is no need to cast a variable that is already an address, such as address(token) tokenContract also address.
Recommendation: Use directly variable.
If a query or logic is repeated over many lines, using a modifier improves the readability and reusability of the code
8 results - 1 file src/AstariaRouter.sol: 326: if (addr == address(0)) revert InvalidFileData(); 330: if (addr == address(0)) revert InvalidFileData(); 369: if (addr == address(0)) revert InvalidFileData(); 373: if (addr == address(0)) revert InvalidFileData(); 377: if (addr == address(0)) revert InvalidFileData(); 381: if (addr == address(0)) revert InvalidFileData(); 397: if (impl == address(0)) { 444: if (strategyValidator == address(0)) {
2 results - 2 files src/CollateralToken.sol: 15 16: pragma experimental ABIEncoderV2; 17 src/LienToken.sol: 15 16: pragma experimental ABIEncoderV2; 17
You can choose to use the old behaviour using pragma abicoder v1;. The pragma pragma experimental ABIEncoderV2; is still valid, but it is deprecated and has no effect. If you want to be explicit, please use pragma abicoder v2; instead.
inherited state variablessrc/PublicVault.sol: 116 */ 117: function redeem( 118: uint256 shares, 119: address receiver, 120: address owner 121: ) public virtual override(ERC4626Cloned) returns (uint256 assets) { 122: VaultData storage s = _loadStorageSlot(); 123: assets = _redeemFutureEpoch(s, shares, receiver, owner, s.currentEpoch); 124: }
Description:
In src/PublicVault.sol #L123 , there is a local variable named owner, but there is a state varible named owner in the inherited AsteriaVaultBase.sol with the same name. This use causes compilers to issue warnings, negatively affecting checking and code readability.
src/AstariaVaultBase.sol: 35 36: function owner() public pure returns (address) { 37: return _getArgAddress(21); //ends at 44 38: }
Recommendation: Avoid using variables with the same name, including inherited in the same contract, if used, it must be specified in the NatSpec comments.
3 results - 1 file src/PublicVault.sol: 458 459: event SlopeUpdated(uint48 newSlope); 460 530 s.slope = newSlope; 531: emit SlopeUpdated(newSlope); 532 } 566 } 567: emit SlopeUpdated(s.slope); 568 }
It's common practice to collectively describe events per contracts, it's not a strict rule, but it helps with contract readability and organization. The SlopeUpdated event here is defined on line 459, it is recommended to put it at the beginning of the contract.
RouterStorage structThere is a bug in the 'RouterStorage' struct SLOT NatSpec comment when switching to slot3;
1 result - 1 file src/interfaces/IAstariaRouter.sol: 74 uint32 minInterestBPS; // was uint64 75: //slot 3 + 76 address guardian; //20
Recommendation Mitigation Step;
struct RouterStorage { //slot 1 uint32 auctionWindow; 4 //slot 1 uint32 auctionWindowBuffer; 4 //slot 1 uint32 liquidationFeeNumerator; 4 //slot 1 uint32 liquidationFeeDenominator; 4 //slot 1 uint32 maxEpochLength; 4 //slot 1 uint32 minEpochLength; 4 //slot 1 uint32 protocolFeeNumerator; 4 //slot 1 uint32 protocolFeeDenominator; 4 //slot 1 ERC20 WETH; // Non slot ICollateralToken COLLATERAL_TOKEN; // Non slot ILienToken LIEN_TOKEN; // Non slot ITransferProxy TRANSFER_PROXY; // Non slot address feeTo; 20 //slot 2 address BEACON_PROXY_IMPLEMENTATION; uint88 maxInterestRate; 11 //slot 2 uint32 minInterestBPS; 4 //slot 3 - //slot3 + address guardian; 20 //slot 3 + //slot3+ address newGuardian; 20 //slot 4 uint32 buyoutFeeNumerator; 4 //slot 4 uint32 buyoutFeeDenominator; 4 //slot 4 uint32 minDurationIncrease; 4 //slot 4 mapping(uint8 => address) strategyValidators; 32 //slot 5 mapping(uint8 => address) implementations; 32 //slot 6 //A strategist can have many deployed vaults mapping(address => bool) vaults; 32 //slot 7 }
At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an " EMERGENCY STOP (CIRCUIT BREAKER) PATTERN ".
https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol
This codebase will be difficult to navigate, as there are no descriptive naming conventions that specify which files should contain meaningful logic.
Prefixes should be added like this by filing:
Interface I_ absctract contracts Abs_ Libraries Lib_
We recommend that you implement this or a similar agreement.
#0 - c4-judge
2023-01-26T14:54:48Z
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
363.1567 USDC - $363.16
| Number | Optimization Details | Context |
|---|---|---|
| [G-01] | Remove the initializer modifier | 3 |
| [G-02] | Use hardcode address instead address(this) | 26 |
| [G-03] | Duplicated require()/if() checks should be refactored to a modifier or function | 13 |
| [G-04] | Structs can be packed into fewer storage slots | 1 |
| [G-05] | State variable can be used instead of struct | 1 |
| [G-06] | Using delete instead of setting info struct to 0 saves gas | 5 |
| [G-07] | Gas optimization by changing the emit order | 2 |
| [G-08] | Empty blocks should be removed or emit something | 7 |
| [G-09] | Using storage instead of memory for structs/arrays saves gas | 15 |
| [G-10] | ] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require or if statement | 2 |
| [G-11] | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 7 |
| [G-12] | Setting the constructor to payable | 4 |
| [G-13] | Functions guaranteed to revert_ when callled by normal users can be marked payable | 8 |
| [G-14] | Use a more recent version of solidity | 7 |
| [G-15] | Use double require instead of using && | 4 |
| [G-16] | Use nested if and, avoid multiple check combinations | 8 |
| [G-17] | Sort Solidity operations using short-circuit mode | 11 |
| [G-18] | Optimize names to save gas | All contracts |
| [G-19] | Upgrade Solidity's optimizer | |
| [G-20] | >= costs less gas than > | 1 |
Total 20 issues
initializer modifierif we can just ensure that the initialize() function could only be called from within the constructor, we shouldn't need to worry about it getting called again.
3 results - 3 files:
src/AstariaRouter.sol: 83: function initialize( 93 ) external initializer {
https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L83
src/CollateralToken.sol: 80: function initialize( 85 ) public initializer {
https://github.com/code-423n4/2023-01-astaria/blob/main/src/CollateralToken.sol#L80
src/LienToken.sol: 59: function initialize(Authority _AUTHORITY, ITransferProxy _TRANSFER_PROXY) 61 initializer
https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L59
In the EVM, the constructor's job is actually to return the bytecode that will live at the contract's address. So, while inside a constructor, your address (address(this)) will be the deployment address, but there will be no bytecode at that address! So if we check address(this).code.length before the constructor has finished, even from within a delegatecall, we will get 0. So now let's update our initialize() function to only run if we are inside a constructor:
src/LienToken.sol#L59 59: function initialize(Authority _AUTHORITY, ITransferProxy _TRANSFER_PROXY) - 61 initializer + require(address(this).code.length == 0, 'not in constructor');
Now the Proxy contract's constructor can still delegatecall initialize(), but if anyone attempts to call it again (after deployment) through the Proxy instance, or tries to call it directly on the above instance, it will revert because address(this).code.length will be nonzero.
Also, because we no longer need to write to any state to track whether initialize() has been called, we can avoid the 20k storage gas cost. In fact, the cost for checking our own code size is only 2 gas, which means we have a 10,000x gas savings over the standard version. Pretty neat!
address(this)Instead of address(this), it is more gas-efficient to pre-calculate and use the address with a hardcode. Foundry's script.sol and solmate````LibRlp.sol`` contracts can do this.
Reference: https://book.getfoundry.sh/reference/forge-std/compute-create-address https://twitter.com/transmissions11/status/1518507047943245824
26 results - 7 files:
lib\gpl\src\ERC20-Cloned.sol: 158 function computeDomainSeparator() internal view virtual returns (bytes32) { 167: address(this)
lib\gpl\src\ERC4626-Cloned.sol: 29: ERC20(asset()).safeTransferFrom(msg.sender, address(this), assets); 45: ERC20(asset()).safeTransferFrom(msg.sender, address(this), assets);
lib\gpl\src\ERC4626Router.sol: 30: pullToken(vault.asset(), amount, address(this)); 44: pullToken(address(asset), amount, address(this)); 57: pullToken(address(vault), amountShares, address(this));
src\AstariaRouter.sol: 734: address(this), 784: address(this)
https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L734
src\ClearingHouse.sol: 132: uint256 payment = ERC20(paymentToken).balanceOf(address(this)); 160: if (ERC20(paymentToken).balanceOf(address(this)) > 0) { 163: ERC20(paymentToken).balanceOf(address(this)) 217: ERC721(tokenContract).safeTransferFrom(address(this), target, tokenId);
https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L132
src\CollateralToken.sol: 93: bytes32 CONDUIT_KEY = Bytes32AddressLib.fillLast12Bytes(address(this)); 97: s.CONDUIT = s.CONDUIT_CONTROLLER.createConduit(CONDUIT_KEY, address(this)); 228: s.CONDUIT_KEY = Bytes32AddressLib.fillLast12Bytes(address(this)); 237: address(this) 464: zone: address(this), 512: if (listingOrder.parameters.zone != address(this)) { 564: require(ERC721(msg.sender).ownerOf(tokenId_) == address(this)); 579: address(this), 584: if (msg.sender == address(this) || msg.sender == address(s.LIEN_TOKEN)) {
https://github.com/code-423n4/2023-01-astaria/blob/main/src/CollateralToken.sol#L93
src\PublicVault.sol: 179: es.balanceOf[address(this)] += shares; 182: emit Transfer(owner, address(this), shares); 226: address(this), // vault 336: _burn(address(this), proxySupply); 372: uint256 withdrawBalance = ERC20(asset()).balanceOf(address(this));
https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L179
13 results 5 files:
src/AstariaRouter.sol: 341: require(msg.sender == s.guardian); 347: require(msg.sender == s.guardian);
AstariaRouter.sol#L341, AstariaRouter.sol#L347
src/ClearingHouse.sol: 199: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN())); 216: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN())); 223: require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));
ClearingHouse.sol#L199, ClearingHouse.sol#L216, ClearingHouse.sol#L223
src/PublicVault.sol: 508: require(msg.sender == owner()); //owner is "strategist»
src/Vault.sol: 71: require(msg.sender == owner());
src/VaultImplementation.sol: 78: require(msg.sender == owner()); //owner is "strategist" 96: require(msg.sender == owner()); //owner is "strategist" 105: require(msg.sender == owner()); //owner is "strategist" 114: require(msg.sender == owner()); //owner is "strategist" 147: require(msg.sender == owner()); //owner is "strategist" 211: require(msg.sender == owner()); //owner is "strategist»
VaultImplementation.sol#L78, VaultImplementation.sol#L96, VaultImplementation.sol#L105, VaultImplementation.sol#L114, VaultImplementation.sol#L147, VaultImplementation.sol#L211
Recommendation:
You can consider adding a modifier like below.
modifer checkOwner () { require(require(msg.sender == owner()); _; }
Here are the data available in the covered contracts. The use of this situation in contracts that are not covered will also provide gas optimization.
The StrategyDetailsParam struct can be packaged as suggested below.
src\interfaces\IAstariaRouter.sol: 101 struct StrategyDetailsParam { 102 uint8 version; // slot0 103 uint256 deadline; // slot1 104 address vault; // slot2 105 }
1 slot can be saved here
src\interfaces\IAstariaRouter.sol: 101 struct StrategyDetailsParam { - 102 uint8 version; 103 uint256 deadline; // slot0 + 102 uint8 version; // slot1 104 address vault; // slot1 105 }
Using the 'uint256 remainder' state variable instead of the 'LiquidationPaymentParams' structure saves gas.
src\interfaces\IPublicVault.sol: 54: struct LiquidationPaymentParams { 55 uint256 remaining; 56 }
info struct to 0 saves gas5 results - 2 files
src/PublicVault.sol: 308: s.liquidationWithdrawRatio = 0; 327: s.withdrawReserve = 0; 377: s.withdrawReserve = 0; 511: s.strategistUnclaimedShares = 0;
PublicVault.sol#L308, PublicVault.sol#L327, PublicVault.sol#L377, PublicVault.sol#L511
src/WithdrawProxy.sol: 284: s.finalAuctionEnd = 0;
2 results - 1 file:
Considering the risk of reentrancy, moving 'Emit' to the end of the transaction provides gas optimization to avoid cost in case of ‘revert' of the transaction.
lib\gpl\src\ERC4626-Cloned.sol#L54-L77
lib\gpl\src\ERC4626-Cloned.sol: 54 function withdraw( 70: beforeWithdraw(assets, shares); 72: _burn(owner, shares); 74: emit Withdraw(msg.sender, receiver, owner, assets, shares); 76: ERC20(asset()).safeTransfer(receiver, assets); 77: }
lib\gpl\src\ERC4626-Cloned.sol: 54 function withdraw( 70: beforeWithdraw(assets, shares); 72: _burn(owner, shares); - 74: emit Withdraw(msg.sender, receiver, owner, assets, shares); 76: ERC20(asset()).safeTransfer(receiver, assets); + 74: emit Withdraw(msg.sender, receiver, owner, assets, shares); 77: }
lib/gpl/src/ERC4626-Cloned.sol#L79-L103
lib/gpl/src/ERC4626-Cloned.sol: 79 function redeem( 96 beforeWithdraw(assets, shares); 98 _burn(owner, shares); 100: emit Withdraw(msg.sender, receiver, owner, assets, shares); 102 ERC20(asset()).safeTransfer(receiver, assets); 103 }
lib/gpl/src/ERC4626-Cloned.sol: 79 function redeem( 96 beforeWithdraw(assets, shares); 98 _burn(owner, shares); - 100: emit Withdraw(msg.sender, receiver, owner, assets, shares); 102 ERC20(asset()).safeTransfer(receiver, assets); + 100 emit Withdraw(msg.sender, receiver, owner, assets, shares); 103 }
The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}}). Empty receive()/fallback() payable functions that are not used, can be removed to save deployment gas.
7 results - 4 files:
lib/gpl/src/ERC4626-Cloned.sol: 165: function beforeWithdraw(uint256 assets, uint256 shares) internal virtual {} 167: function afterDeposit(uint256 assets, uint256 shares) internal virtual {}
ERC4626-Cloned.sol#L165, ERC4626-Cloned.sol#L167
src/BeaconProxy.sol: 87: function _beforeFallback() internal virtual {}
src/ClearingHouse.sol: 104: function setApprovalForAll(address operator, bool approved) external {} 180 function safeBatchTransferFrom( 186: ) public {}
ClearingHouse.sol#L104, ClearingHouse.sol#L186
src/VaultImplementation.sol: 268 function _afterCommitToLien( 272: ) internal virtual {} 274 function _beforeCommitToLien(IAstariaRouter.Commitment calldata) 277: {}
VaultImplementation.sol#L272, VaultImplementation.sol#L272
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
15 results - 4 files:
src/AstariaRouter.sol: 527: address[] memory allowList = new address[](1);
src/ClearingHouse.sol: 200: Order[] memory listings = new Order[](1);
src/CollateralToken.sol: 137: Asset memory underlying = s.idToUnderlying[collateralId]; 214: bytes memory data = incoming.data; 341: Asset memory underlying = s.idToUnderlying[collateralId]; 390: Asset memory underlying = _loadCollateralSlot().idToUnderlying[ 432: OfferItem[] memory offer = new OfferItem[](1); 434: Asset memory underlying = s.idToUnderlying[collateralId]; 444: ConsiderationItem[] memory considerationItems = new ConsiderationItem[](2); 484: uint256[] memory prices = new uint256[](2); 562: Asset memory incomingAsset = s.idToUnderlying[collateralId];
CollateralToken.sol#L137, CollateralToken.sol#L214, CollateralToken.sol#L341, CollateralToken.sol#L390, CollateralToken.sol#L432, CollateralToken.sol#L434, CollateralToken.sol#L444, CollateralToken.sol#L484, CollateralToken.sol#L562
src/LienToken.sol: 84: bytes memory data = incoming.data; 299: AuctionData memory auctionData; 305: AuctionStack memory auctionStack; 401: Stack memory newStackSlot;
LienToken.sol#L84, LienToken.sol#L299, LienToken.sol#L305, LienToken.sol#L401
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 } if(a <= b); x = b - a => if(a <= b); unchecked { x = b - a }
This will stop the check for overflow and underflow so it will save gas.
2 results - 1 file:
src/LienToken.sol: 623 function _paymentAH( 628 uint256 payment, 633 uint256 owing = stack[position].amountOwed; 637: if (owing > payment.safeCastTo88()) { 638 remaining = owing - payment;
https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L637
src/LienToken.sol: 790 function _payment( 829: if (stack.point.amount > amount) { 830 stack.point.amount -= amount.safeCastTo88();
https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L829
When using elements that are smaller than 32 bytes, your contracts 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
Use a larger size then downcast where needed.
7 results - 2 files:
src\AstariaRouter.sol: ///@audit uint8 nlrType 442: uint8 nlrType = uint8(_sliceUint(commitment.lienRequest.nlrDetails, 0)); //@audit uint8 vaultType 725: vaultType = uint8(ImplementationType.PublicVault); 727: vaultType = uint8(ImplementationType.PrivateVault);
AstariaRouter.sol#L442, AstariaRouter.sol#L725, AstariaRouter.sol#L727
src\PublicVault.sol: ///@audit uint64 epoch 227: epoch + 1 // claimable epoch ///@audit uint40 lienEnd 453: uint64 epoch = getLienEpoch(lienEnd); /// @audit uint88 feeInShares 606: s.strategistUnclaimedShares += feeInShares; ///@audit uint64 lienEpoch 657: uint256 timeToEnd = timeToEpochEnd(lienEpoch);
PublicVault.sol#L227, PublicVault.sol#L453, PublicVault.sol#L606, PublicVault.sol#L657
payableYou can cut out 10 opcodes in the creation-time EVM bytecode if you declare a constructor payable. Making the constructor payable eliminates the need for an initial check of msg.value == 0 and saves 13 gas on deployment with no security risks.
4 results - 4 files
src/AstariaRouter.sol: 70: constructor() {
https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L70
src/CollateralToken.sol: 76: constructor() {
https://github.com/code-423n4/2023-01-astaria/blob/main/src/CollateralToken.sol#L76
src/LienToken.sol: 55: constructor() {
https://github.com/code-423n4/2023-01-astaria/blob/main/src/LienToken.sol#L55
src/TransferProxy.sol: 24: constructor(Authority _AUTHORITY) Auth(msg.sender, _AUTHORITY) {
https://github.com/code-423n4/2023-01-astaria/blob/main/src/TransferProxy.sol#L24
Recommendation:
Set the constructor to payable
payableNote: Automated findings of this gas optimization by Code4rena were found. (https://gist.github.com/Picodes/d16459938599db69f9ad88c73a2d2990#gas-7-functions-guaranteed-to-revert-when-called-by-normal-users-can-be-marked-payable). However, when the codes within the scope were examined, it was noticed that the results below were not found in the Automated findings file. For this reason, eight results within the scope are reported.
If a function modifier or require such as onlyOwner-admin 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.
8 result - 3 files:
src/CollateralToken.sol: 270 function flashAction( 274: ) external onlyOwner(collateralId) { 331 function releaseToAddress(uint256 collateralId, address releaseTo) 334: onlyOwner(collateralId)
CollateralToken.sol#L274, CollateralToken.sol#L333-L334
src/PublicVault.sol: 515 function beforePayment(BeforePaymentParams calldata params) 517: onlyLienToken 615 function handleBuyoutLien(BuyoutLienParams calldata params) 617: onlyLienToken 632 function updateAfterLiquidationPayment( 634: ) external onlyLienToken { 640 function updateVaultAfterLiquidation( 643: ) public onlyLienToken returns (address withdrawProxyIfNearBoundary) {
PublicVault.sol#L517, PublicVault.sol#L617, PublicVault.sol#L634, PublicVault.sol#L643
src/WithdrawProxy.sol: 289 function drain(uint256 amount, address withdrawProxy) 291: onlyVault 306 function handleNewLiquidation( 309: ) public onlyVault {
WithdrawProxy.sol#L171, WithdrawProxy.sol#L192, WithdrawProxy.sol#L291, WithdrawProxy.sol#L309
Recommendation:
Functions guaranteed to revert when called by normal users can be marked payable (for only onlyOwner, onlyLienToken, onlyVault functions)
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.
Solidity 0.8.10 has a useful change that reduced gas costs of external calls which expect a return value.
In 0.8.15 the conditions necessary for inlining are relaxed. Benchmarks show that the change significantly decreases the bytecode size (which impacts the deployment cost) while the effect on the runtime gas usage is smaller.
In 0.8.17 prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call; Simplify the starting offset of zero-length operations to zero. More efficient overflow checks for multiplication.
7 results - 7 files:
lib/gpl/src/ERC20-Cloned.sol: 2: pragma solidity >=0.8.16;
lib/gpl/src/ERC721.sol: 2: pragma solidity ^0.8.16;
lib/gpl/src/ERC4626-Cloned.sol: 2: pragma solidity >=0.8.16;
lib/gpl/src/interfaces/IMulticall.sol: 4: pragma solidity >=0.7.5;
lib/gpl/src/interfaces/IUniswapV3Factory.sol: 2: pragma solidity >=0.5.0;
lib/gpl/src/interfaces/IUniswapV3PoolState.sol: 2: pragma solidity >=0.5.0;
lib/gpl/src/interfaces/IWETH9.sol: 1: pragma solidity ^0.8.13;
double require instead of using &&Using double require instead of operator && can save more gas When having a require statement with 2 or more expressions needed, place the expression that cost less gas first.
4 results - 3 files:
lib/gpl/src/ERC20-Cloned.sol: 143: require( 144: recoveredAddress != address(0) && recoveredAddress == owner, 145: "INVALID_SIGNER" 146: );
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: );
PublicVault.sol#L672-L675, PublicVault.sol#L687-L690
src/Vault.sol: 65: require(s.allowList[msg.sender] && receiver == owner());
Recommendation Code:
src/Vault.sol#L65 - 65: require(s.allowList[msg.sender] && receiver == owner()); + require(s.allowList[msg.sender]); + require(receiver == owner());
Using nested is cheaper than using && multiple check combinations. There are more advantages, such as easier to read code and better coverage reports.
8 results - 4 files:
src/CollateralToken.sol: 526: if ( 527 s.collateralIdToAuction[collateralId] == bytes32(0) && 528 ERC721(s.idToUnderlying[collateralId].tokenContract).ownerOf( 529 s.idToUnderlying[collateralId].tokenId 530 ) != 531 s.clearingHouse[collateralId] 532 ) {
src/PublicVault.sol: 295: if ( 296 address(previousWithdrawProxy) != address(0) && 297 previousWithdrawProxy.getFinalAuctionEnd() != 0 298 ) { 392: if ( 393 s.withdrawReserve > 0 && 394 timeToEpochEnd() == 0 && 395 withdrawProxy != address(0) 396 ) { 586: if (v.depositCap != 0 && totalAssets() >= v.depositCap) {
PublicVault.sol#L295-L298, PublicVault.sol#L392-L396, PublicVault.sol#L586
src/VaultImplementation.sol: 66: if (msg.sender != owner() && msg.sender != s.delegate) { 237: if ( 238 msg.sender != holder && 239 receiver != holder && 240 receiver != operator && 241 !CT.isApprovedForAll(holder, msg.sender) 242 ) { 258: if ( 259 (recovered != owner() && recovered != s.delegate) || 260 recovered == address(0) 261 ) {
VaultImplementation.sol#L66, VaultImplementation.sol#L237-L242, VaultImplementation.sol#L258-L261
src/AstariaRouter.sol: 476: if (timeToSecondEpochEnd > 0 && details.duration > timeToSecondEpochEnd) {
Recomendation Code:
src/AstariaRouter.sol#L476 - 476: if (timeToSecondEpochEnd > 0 && details.duration > timeToSecondEpochEnd) { + if (timeToSecondEpochEnd > 0) { + if (details.duration > timeToSecondEpochEnd) { + } + }
Short-circuiting is a solidity contract development model that uses OR/AND logic to sequence different cost operations. It puts low gas cost operations in the front and high gas cost operations in the back, so that if the front is low If the cost operation is feasible, you can skip (short-circuit) the subsequent high-cost Ethereum virtual machine operation.
//f(x) is a low gas cost operation //g(y) is a high gas cost operation //Sort operations with different gas costs as follows f(x) || g(y) f(x) && g(y)
11 results - 7 files:
lib/gpl/src/ERC20-Cloned.sol: 143 require( 144: recoveredAddress != address(0) && recoveredAddress == owner,
src/AstariaRouter.sol: 458: if (details.rate == uint256(0) || details.rate > s.maxInterestRate) { 476: if (timeToSecondEpochEnd > 0 && details.duration > timeToSecondEpochEnd) {
AstariaRouter.sol#L458 AstariaRouter.sol#L476
src/LienToken.sol: 268: if (stateHash == bytes32(0) && stack.length != 0) { 271: if (stateHash != bytes32(0) && keccak256(abi.encode(stack)) != stateHash) { 431 if ( 432: params.lien.details.liquidationInitialAsk < params.amount || 433 params.lien.details.liquidationInitialAsk == 0
LienToken.sol#L268, LienToken.sol#L271, LienToken.sol#L432
src/Vault.sol: 65: require(s.allowList[msg.sender] && receiver == owner());
lib/gpl/src/ERC721.sol: 117 require( 118: msg.sender == from || 119 s.isApprovedForAll[from][msg.sender] || 120 msg.sender == s.getApproved[id],
src/CollateralToken.sol: 116 if ( 117: s.collateralIdToAuction[collateralId] == bytes32(0) || 118 liquidator == address(0) 584: if (msg.sender == address(this) || msg.sender == address(s.LIEN_TOKEN)) {
CollateralToken.sol#L117, CollateralToken.sol#L584
src/VaultImplementation.sol: 258 if ( 259: (recovered != owner() && recovered != s.delegate) || 260 recovered == address(0)
Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions.
Context: All Contracts
Recommendation:
Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas
For example, the function IDs in the AstariaRouter.sol contract will be the most used; A lower method ID may be given.
Proof of Consept: https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92
AstariaRouter.sol function names can be named and sorted according to METHOD ID
Sighash | Function Signature ======================== 32299871 => _transferAndDepositAssetIfAble(RouterStorage,address,uint256) 64219450 => isValidVault(address) 4951ec4a => initialize(Authority,ICollateralToken,ILienToken,ITransferProxy,address,address,address,address,address) a7d76ed7 => mint(IERC4626,address,uint256,uint256) a625eea8 => deposit(IERC4626,address,uint256,uint256) 1e45b063 => withdraw(IERC4626,address,uint256,uint256) 8897310d => redeem(IERC4626,address,uint256,uint256) 83bc744b => redeemFutureEpoch(IPublicVault,uint256,address,uint64) 73d15414 => pullToken(address,uint256,address) e96472b2 => _loadRouterSlot() 017e7e58 => feeTo() 1ff0c9d6 => BEACON_PROXY_IMPLEMENTATION() 41700b97 => LIEN_TOKEN() 96d6401d => TRANSFER_PROXY() f5f1f1a7 => COLLATERAL_TOKEN() bce1e58b => __emergencyPause() c65a7ecf => __emergencyUnpause() 9c427620 => fileBatch(File[]) 7ec71dc0 => file(File) 82f0d462 => _file(File) e87f7c31 => setNewGuardian(address) 9c37b27f => __renounceGuardian() d54aa70a => __acceptGuardian() 2452a88c => fileGuardian(File[]) 0472a61d => getImpl(uint8) e9edc477 => getAuctionWindow(bool) 0a5a2e90 => _sliceUint(bytes,uint256) 010de84d => validateCommitment(IAstariaRouter.Commitment,uint256) 2d419ce6 => _validateCommitment(RouterStorage,IAstariaRouter.Commitment,uint256) 5463ff49 => commitToLiens(IAstariaRouter.Commitment[]) 54d84628 => newVault(address,address) 22ba6a33 => newPublicVault(uint256,address,address,uint256,bool,address[],uint256) e8c65c52 => requestLienPosition(IAstariaRouter.Commitment,address) fb2036ae => canLiquidate(ILienToken.Stack) 6af6521b => liquidate(ILienToken.Stack[],uint8) 0f0245ad => getProtocolFee(uint256) 1276cf53 => getLiquidatorFee(uint256) aaf6b93b => getBuyoutFee(uint256) c6ccd36d => isValidRefinance(ILienToken.Lien,uint8,ILienToken.Stack[]) 402c5629 => _newVault(RouterStorage,address,uint256,address,uint256,bool,address[],uint256) 70983c65 => _executeCommitment(RouterStorage,IAstariaRouter.Commitment)
Make sure Solidity’s optimizer is enabled. It reduces gas costs. If you want to gas optimize for contract deployment (costs less to deploy a contract) then set the Solidity optimizer at a low number. If you want to optimize for run-time gas costs (when functions are called on a contract) then set the optimizer to a high number.
>= costs less gas than >The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas.
src\PublicVault.sol: 603: uint256 x = (amount > interestOwing) ? interestOwing : amount;
#0 - c4-judge
2023-01-25T23:53:00Z
Picodes marked the issue as grade-a