Platform: Code4rena
Start Date: 16/12/2022
Pot Size: $60,500 USDC
Total HM: 12
Participants: 58
Period: 5 days
Judge: Trust
Total Solo HM: 4
Id: 196
League: ETH
Rank: 14/58
Findings: 2
Award: $866.64
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: yixxas
Also found by: 0x52, 0xAgro, 0xSmartContract, 0xhacksmithh, Aymen0909, Bnke0x0, Bobface, Breeje, Diana, Franfran, HE1M, HollaDieWaldfee, IllIllI, Jeiwan, RaymondFam, Rolezn, SaharDevep, Secureverse, SmartSek, ak1, bin2chen, brgltd, chrisdior4, gz627, imare, ladboy233, lukris02, oyc_109, rvierdiiev, shark, tnevler, unforgiven, wait
353.8487 USDC - $353.85
| Issue | Instances | |
|---|---|---|
| [L‑01] | Return value of ecrecover() is not checked | 1 |
Total: 1 instances over 1 issues
| Issue | Instances | |
|---|---|---|
| [N‑01] | constants should be defined rather than using magic numbers | 16 |
| [N‑02] | Missing event and or timelock for critical parameter change | 1 |
| [N‑03] | Events that mark critical parameter changes should contain both the old and the new value | 2 |
| [N‑04] | Use a more recent version of solidity | 3 |
| [N‑05] | Use a more recent version of solidity | 1 |
| [N‑06] | Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18) | 2 |
| [N‑07] | Non-library/interface files should use fixed compiler versions, not floating ones | 4 |
| [N‑08] | Using >/>= without specifying an upper bound is unsafe | 10 |
| [N‑09] | Typos | 3 |
| [N‑10] | File is missing NatSpec | 3 |
| [N‑11] | NatSpec is incomplete | 10 |
| [N‑12] | Consider using delete rather than assigning zero to clear values | 2 |
| [N‑13] | Contracts should have full test coverage | 1 |
| [N‑14] | Large or complicated code bases should implement fuzzing tests | 1 |
Total: 59 instances over 14 issues
ecrecover() is not checkedWhen the signature doesn't match, 0 is returned as the signer. Usually this is fine, but in this case, the immutable oracleSigner address is not verified to be non-zero, so if a mistake is made during deployment, the end result will be that anyone can specify whatever underwriting price they want, allowing them to manipulate the pool to have whatever value they wish
There is 1 instance of this issue:
File: /src/ReservoirOracleUnderwriter.sol 88: if (signerAddress != oracleSigner) {
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 16 instances of this issue:
File: src/libraries/OracleLibrary.sol /// @audit 192 22: ? FullMath.mulDiv(ratioX192, baseAmount, 1 << 192) /// @audit 192 23: : FullMath.mulDiv(1 << 192, baseAmount, ratioX192); /// @audit 64 25: uint256 ratioX128 = FullMath.mulDiv(sqrtRatioX96, sqrtRatioX96, 1 << 64); /// @audit 128 27: ? FullMath.mulDiv(ratioX128, baseAmount, 1 << 128) /// @audit 128 28: : FullMath.mulDiv(1 << 128, baseAmount, ratioX128);
File: src/libraries/UniswapHelpers.sol /// @audit 96 111: return TickMath.getSqrtRatioAtTick(TickMath.getTickAtSqrtRatio(uint160((token1ONE << 96) / token0ONE)) / 2);
File: src/PaprController.sol /// @audit 18 87: initSqrtRatio = UniswapHelpers.oneToOneSqrtRatio(underlyingONE, 10 ** 18); /// @audit 18 89: initSqrtRatio = UniswapHelpers.oneToOneSqrtRatio(10 ** 18, underlyingONE); /// @audit 10000 92: address _pool = UniswapHelpers.deployAndInitPool(address(underlying), address(papr), 10000, initSqrtRatio); /// @audit 1e17 76: NFTEDAStarterIncentive(1e17) /// @audit 200 473: if (newDebt >= 1 << 200) revert IPaprController.DebtAmountExceedsUint200();
File: src/PaprToken.sol /// @audit 18 19: ERC20(string.concat("papr ", name), string.concat("papr", symbol), 18)
File: src/UniswapOracleFundingRateController.sol /// @audit 4 41: _setFundingPeriod(4 weeks); /// @audit 7 123: if (_fundingPeriod < 7 days) revert FundingPeriodTooShort(); /// @audit 90 124: if (_fundingPeriod > 90 days) revert FundingPeriodTooLong(); /// @audit 1e18 138: return OracleLibrary.getQuoteAtTick(twapTick, 1e18, address(papr), address(underlying));
Events help non-contract tools to track changes, and events prevent users from being surprised by changes
There is 1 instance of this issue:
File: src/PaprController.sol 360 function setLiquidationsLocked(bool locked) external override onlyOwner { 361 liquidationsLocked = locked; 362: }
This should especially be done if the new value is not required to be different from the old value
There are 2 instances of this issue:
File: src/PaprController.sol /// @audit setAllowedCollateral() 374: emit AllowCollateral(collateralConfigs[i].collateral, collateralConfigs[i].allowed);
File: src/UniswapOracleFundingRateController.sol /// @audit updateTarget() 59: emit UpdateTarget(nTarget);
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
There are 3 instances of this issue:
File: src/libraries/UniswapHelpers.sol 2: pragma solidity >=0.8.0;
File: src/NFTEDA/libraries/EDAPrice.sol 2: pragma solidity >=0.8.0;
File: src/NFTEDA/NFTEDA.sol 2: pragma solidity >=0.8.0;
Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)
Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)
There is 1 instance of this issue:
File: src/libraries/PoolAddress.sol 4: pragma solidity >=0.8.0;
1e18) rather than exponentiation (e.g. 10**18)While the compiler knows to optimize away the exponentiation, it's still better coding practice to use idioms that do not require compiler optimization, if they exist
There are 2 instances of this issue:
File: src/PaprController.sol 87: initSqrtRatio = UniswapHelpers.oneToOneSqrtRatio(underlyingONE, 10 ** 18); 89: initSqrtRatio = UniswapHelpers.oneToOneSqrtRatio(10 ** 18, underlyingONE);
There are 4 instances of this issue:
File: src/PaprController.sol 2: pragma solidity ^0.8.17;
File: src/PaprToken.sol 2: pragma solidity ^0.8.17;
File: src/ReservoirOracleUnderwriter.sol 2: pragma solidity ^0.8.17;
File: src/UniswapOracleFundingRateController.sol 2: pragma solidity ^0.8.17;
>/>= 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 10 instances of this issue:
File: src/interfaces/IFundingRateController.sol 2: pragma solidity >=0.8.0;
File: src/interfaces/IPaprController.sol 2: pragma solidity >=0.8.0;
File: src/interfaces/IUniswapOracleFundingRateController.sol 2: pragma solidity >=0.8.0;
File: src/libraries/OracleLibrary.sol 2: pragma solidity >=0.8.0;
File: src/libraries/PoolAddress.sol 4: pragma solidity >=0.8.0;
File: src/libraries/UniswapHelpers.sol 2: pragma solidity >=0.8.0;
File: src/NFTEDA/extensions/NFTEDAStarterIncentive.sol 2: pragma solidity >=0.8.0;
File: src/NFTEDA/interfaces/INFTEDA.sol 2: pragma solidity >=0.8.0;
File: src/NFTEDA/libraries/EDAPrice.sol 2: pragma solidity >=0.8.0;
File: src/NFTEDA/NFTEDA.sol 2: pragma solidity >=0.8.0;
There are 3 instances of this issue:
File: src/NFTEDA/interfaces/INFTEDA.sol /// @audit Identitical 58: /// @dev Derived from the auction. Identitical auctions cannot exist simultaneously
File: src/NFTEDA/NFTEDA.sol /// @audit defintion 46: /// @param auction The defintion of the auction
File: src/PaprController.sol /// @audit succesful 158: /// @return selector indicating succesful receiving of the NFT
There are 3 instances of this issue:
File: src/libraries/OracleLibrary.sol
File: src/NFTEDA/libraries/EDAPrice.sol
File: src/PaprToken.sol
https://github.com/with-backed/papr/blob/1933da2e38ff9d47c17e2749d6088bbbd40bfa68/src/PaprToken.sol
There are 10 instances of this issue:
File: src/interfaces/IPaprController.sol /// @audit Missing: '@param oracleInfo' 173 /// @notice purchases a liquidation auction with the controller's papr token 174 /// @dev oracleInfo price must be type TWAP 175 /// @param auction auction to purchase 176 /// @param maxPrice maximum price to pay for the auction 177 /// @param sendTo address to send the collateral to if auction is won 178 function purchaseLiquidationAuctionNFT( 179 INFTEDA.Auction calldata auction, 180 uint256 maxPrice, 181 address sendTo, 182: ReservoirOracleUnderwriter.OracleInfo calldata oracleInfo /// @audit Missing: '@return' 229 /// @param collateral address of the collateral 230 /// @param tokenId tokenId of the collateral 231: function collateralOwner(ERC721 collateral, uint256 tokenId) external view returns (address); /// @audit Missing: '@return' 233 /// @notice returns whether a token address is allowed to serve as collateral for a vault 234 /// @param collateral address of the collateral token 235: function isAllowed(address collateral) external view returns (bool);
File: src/NFTEDA/interfaces/INFTEDA.sol /// @audit Missing: '@return' 63 /// @notice Returns the time at which startAuction was most recently successfully called for the given auction id 64 /// @param id The id of the auction 65: function auctionStartTime(uint256 id) external view returns (uint256);
File: src/NFTEDA/NFTEDA.sol /// @audit Missing: '@param sendTo' /// @audit Missing: '@return' 69 /// @notice purchases the NFT being sold in `auction`, reverts if current auction price exceed maxPrice 70 /// @param auction The auction selling the NFT 71 /// @param maxPrice The maximum the caller is willing to pay 72 function _purchaseNFT(INFTEDA.Auction memory auction, uint256 maxPrice, address sendTo) 73 internal 74 virtual 75: returns (uint256 startTime, uint256 price) /// @audit Missing: '@param id' 108 /// @notice Returns the current price of the passed auction, reverts if no such auction exists 109 /// @dev startTime is passed, optimized for cases where the auctionId has already been computed 110 /// @dev and startTime looked it up 111 /// @param startTime The start time of the auction 112 /// @param auction The auction for which the caller wants to know the current price 113 /// @return price the current amount required to purchase the NFT being sold in this auction 114 function _auctionCurrentPrice(uint256 id, uint256 startTime, INFTEDA.Auction memory auction) 115 internal 116 view 117 virtual 118: returns (uint256)
File: src/PaprController.sol /// @audit Missing: '@param address' 152 /// @notice Handler for safeTransferFrom of an NFT 153 /// @dev Should be preferred to `addCollateral` if only one NFT is being added 154 /// to avoid approval call and save gas 155 /// @param from the current owner of the nft 156 /// @param _id the id of the NFT 157 /// @param data encoded IPaprController.OnERC721ReceivedArgs 158 /// @return selector indicating succesful receiving of the NFT 159 function onERC721Received(address from, address, uint256 _id, bytes calldata data) 160 external 161 override 162: returns (bytes4)
File: src/UniswapOracleFundingRateController.sol /// @audit Missing: '@param _mark_' /// @audit Missing: '@param cachedTarget' 149 /// @notice The multiplier to apply to target() to get newTarget() 150 /// @dev Computes the funding rate for the time since _lastUpdates 151 /// 1 = 1e18, i.e. 152 /// > 1e18 means positive funding rate 153 /// < 1e18 means negative funding rate 154 /// sub 1e18 to get percent change 155 /// @return multiplier used to obtain newTarget() 156: function _multiplier(uint256 _mark_, uint256 cachedTarget) internal view returns (uint256) {
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 2 instances of this issue:
File: src/libraries/OracleLibrary.sol 58: secondAgos[0] = 0;
File: src/PaprController.sol 526: _vaultInfo[auction.nftOwner][auction.auctionAssetContract].latestAuctionStartTime = 0;
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
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] | Missing checks for address(0x0) when assigning values to address state variables | 3 |
Total: 3 instances over 1 issues
| Issue | Instances | |
|---|---|---|
| [N‑01] | require()/revert() statements should have descriptive reason strings | 2 |
| [N‑02] | public functions not called by the contract should be declared external instead | 1 |
| [N‑03] | Event is missing indexed fields | 7 |
Total: 10 instances over 3 issues
address(0x0) when assigning values to address state variablesThere are 3 instances of this issue:
File: src/ReservoirOracleUnderwriter.sol /// @audit (valid but excluded finding) 52: oracleSigner = _oracleSigner; /// @audit (valid but excluded finding) 53: quoteCurrency = _quoteCurrency;
File: src/UniswapOracleFundingRateController.sol /// @audit (valid but excluded finding) 115: pool = _pool;
require()/revert() statements should have descriptive reason stringsThere are 2 instances of this issue:
File: src/libraries/PoolAddress.sol /// @audit (valid but excluded finding) 32: require(key.token0 < key.token1);
File: src/PaprController.sol /// @audit (valid but excluded finding) 245: require(amount1Delta > 0); // swaps entirely within 0-liquidity regions are not supported
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 is 1 instance of this issue:
File: src/UniswapOracleFundingRateController.sol /// @audit (valid but excluded finding) 72: function mark() public view returns (uint256) {
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 7 instances of this issue:
File: src/interfaces/IFundingRateController.sol /// @audit (valid but excluded finding) 9: event UpdateTarget(uint256 newTarget); /// @audit (valid but excluded finding) 11: event SetFundingPeriod(uint256 fundingPeriod);
File: src/interfaces/IPaprController.sol /// @audit (valid but excluded finding) 67: event IncreaseDebt(address indexed account, ERC721 indexed collateralAddress, uint256 amount); /// @audit (valid but excluded finding) 85: event ReduceDebt(address indexed account, ERC721 indexed collateralAddress, uint256 amount); /// @audit (valid but excluded finding) 90: event AllowCollateral(address indexed collateral, bool isAllowed);
File: src/NFTEDA/extensions/NFTEDAStarterIncentive.sol /// @audit (valid but excluded finding) 21: event SetAuctionCreatorDiscount(uint256 discount);
File: src/NFTEDA/interfaces/INFTEDA.sol /// @audit (valid but excluded finding) 50: event EndAuction(uint256 indexed auctionID, uint256 price);
#0 - c4-judge
2022-12-25T13:36:11Z
trust1995 marked the issue as grade-a
🌟 Selected for report: IllIllI
Also found by: 0xSmartContract, 0xhacksmithh, Awesome, Aymen0909, Mukund, RaymondFam, Rolezn, TomJ, c3phas, eyexploit, noot, rbitbytes, rjs, saneryee
512.7906 USDC - $512.79
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [G‑01] | Avoid contract existence checks by using low level calls | 7 | 700 |
| [G‑02] | internal functions only called once can be inlined to save gas | 5 | 100 |
| [G‑03] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement | 1 | 85 |
| [G‑04] | keccak256() should only need to be called on a specific string literal once | 2 | 84 |
| [G‑05] | Optimize names to save gas | 8 | 176 |
| [G‑06] | Use a more recent version of solidity | 10 | - |
| [G‑07] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 1 | 5 |
| [G‑08] | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 5 | - |
| [G‑09] | Using private rather than public for constants, saves gas | 2 | - |
| [G‑10] | Division by two should use bit shifting | 1 | 20 |
| [G‑11] | Functions guaranteed to revert when called by normal users can be marked payable | 8 | 168 |
Total: 50 instances over 11 issues with 1338 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.
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external function calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence
There are 7 instances of this issue:
File: src/libraries/OracleLibrary.sol /// @audit observe() 59: (int56[] memory tickCumulatives,) = IUniswapV3Pool(pool).observe(secondAgos);
File: src/libraries/UniswapHelpers.sol /// @audit swap() 40: (int256 amount0, int256 amount1) = IUniswapV3Pool(pool).swap( /// @audit slot0() 83: (, int24 tick,,,,,) = IUniswapV3Pool(pool).slot0(); /// @audit token0() /// @audit token0() 93: return IUniswapV3Pool(pool1).token0() == IUniswapV3Pool(pool2).token0() /// @audit token1() /// @audit token1() 94: && IUniswapV3Pool(pool1).token1() == IUniswapV3Pool(pool2).token1();
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 5 instances of this issue:
File: src/PaprController.sol 424 function _removeCollateral( 425 address sendTo, 426 IPaprController.Collateral calldata collateral, 427 uint256 oraclePrice, 428: uint256 cachedTarget 493 function _increaseDebtAndSell( 494 address account, 495 address proceedsTo, 496 ERC721 collateralAsset, 497 IPaprController.SwapParams memory params, 498 ReservoirOracleUnderwriter.OracleInfo memory oracleInfo 499: ) internal returns (uint256 amountOut) { 519 function _purchaseNFTAndUpdateVaultIfNeeded(Auction calldata auction, uint256 maxPrice, address sendTo) 520 internal 521: returns (uint256) 532 function _handleExcess(uint256 excess, uint256 neededToSaveVault, uint256 debtCached, Auction calldata auction) 533 internal 534: returns (uint256 remaining)
File: src/UniswapOracleFundingRateController.sol 156: function _multiplier(uint256 _mark_, uint256 cachedTarget) internal view returns (uint256) {
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/PaprController.sol /// @audit if-condition on line 542 546: papr.transfer(auction.nftOwner, totalOwed - debtCached);
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 2 instances of this issue:
File: src/ReservoirOracleUnderwriter.sol 75: keccak256("Message(bytes32 id,bytes payload,uint256 timestamp)"), 94: keccak256("ContractWideCollectionPrice(uint8 kind,uint256 twapSeconds,address contract)"),
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 8 instances of this issue:
File: src/interfaces/IFundingRateController.sol /// @audit updateTarget(), lastUpdated(), target(), newTarget(), mark(), papr(), fundingPeriod() 6: interface IFundingRateController {
File: src/interfaces/IPaprController.sol /// @audit addCollateral(), removeCollateral(), increaseDebt(), reduceDebt(), increaseDebtAndSell(), buyAndReduceDebt(), purchaseLiquidationAuctionNFT(), startLiquidationAuction(), setPool(), setFundingPeriod(), setLiquidationsLocked(), setAllowedCollateral(), sendPaprFromAuctionFees(), burnPaprFromAuctionFees(), collateralOwner(), isAllowed(), liquidationsLocked(), token0IsUnderlying(), maxLTV(), liquidationAuctionMinSpacing(), perPeriodAuctionDecayWAD(), auctionDecayPeriod(), auctionStartPriceMultiplier(), liquidationPenaltyBips(), maxDebt(), vaultInfo() 9: interface IPaprController {
File: src/interfaces/IUniswapOracleFundingRateController.sol /// @audit pool() 6: interface IUniswapOracleFundingRateController is IFundingRateController {
File: src/NFTEDA/interfaces/INFTEDA.sol /// @audit auctionCurrentPrice(), auctionID(), auctionStartTime() 7: interface INFTEDA {
File: src/NFTEDA/NFTEDA.sol /// @audit auctionCurrentPrice(), auctionID(), auctionStartTime() 11: abstract contract NFTEDA is INFTEDA {
File: src/PaprController.sol /// @audit uniswapV3SwapCallback() 18: contract PaprController is
File: src/ReservoirOracleUnderwriter.sol /// @audit underwritePriceForCollateral() 7: contract ReservoirOracleUnderwriter {
File: src/UniswapOracleFundingRateController.sol /// @audit mark() 15: contract UniswapOracleFundingRateController is IUniswapOracleFundingRateController {
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 10 instances of this issue:
File: src/interfaces/IFundingRateController.sol 2: pragma solidity >=0.8.0;
File: src/interfaces/IPaprController.sol 2: pragma solidity >=0.8.0;
File: src/interfaces/IUniswapOracleFundingRateController.sol 2: pragma solidity >=0.8.0;
File: src/libraries/OracleLibrary.sol 2: pragma solidity >=0.8.0;
File: src/libraries/PoolAddress.sol 4: pragma solidity >=0.8.0;
File: src/libraries/UniswapHelpers.sol 2: pragma solidity >=0.8.0;
File: src/NFTEDA/extensions/NFTEDAStarterIncentive.sol 2: pragma solidity >=0.8.0;
File: src/NFTEDA/interfaces/INFTEDA.sol 2: pragma solidity >=0.8.0;
File: src/NFTEDA/libraries/EDAPrice.sol 2: pragma solidity >=0.8.0;
File: src/NFTEDA/NFTEDA.sol 2: pragma solidity >=0.8.0;
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There is 1 instance of this issue:
File: src/libraries/OracleLibrary.sol 48: twat--;
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 5 instances of this issue:
File: src/libraries/OracleLibrary.sol /// @audit int24 twat 44: twat = int24(delta / twapDuration);
File: src/PaprController.sol /// @audit uint16 newCount 438: newCount = _vaultInfo[msg.sender][collateral.addr].count - 1;
File: src/UniswapOracleFundingRateController.sol /// @audit uint48 _lastUpdated 55: _lastUpdated = uint48(block.timestamp); /// @audit uint48 _lastUpdated 100: _lastUpdated = uint48(block.timestamp); /// @audit int56 tickCumulative 143: tickCumulative = OracleLibrary.latestCumulativeTick(pool);
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 are 2 instances of this issue:
File: src/UniswapOracleFundingRateController.sol 25: uint256 public immutable targetMarkRatioMax; 27: uint256 public immutable targetMarkRatioMin;
<x> / 2 is the same as <x> >> 1. While the compiler uses the SHR opcode to accomplish both, the version that uses division incurs an overhead of 20 gas due to JUMPs to and from a compiler utility function that introduces checks which can be avoided by using unchecked {} around the division by two
There is 1 instance of this issue:
File: src/libraries/UniswapHelpers.sol 111: return TickMath.getSqrtRatioAtTick(TickMath.getTickAtSqrtRatio(uint160((token1ONE << 96) / token0ONE)) / 2);
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 8 instances of this issue:
File: src/PaprController.sol 350: function setPool(address _pool) external override onlyOwner { 355: function setFundingPeriod(uint256 _fundingPeriod) external override onlyOwner { 360: function setLiquidationsLocked(bool locked) external override onlyOwner { 365 function setAllowedCollateral(IPaprController.CollateralAllowedConfig[] calldata collateralConfigs) 366 external 367 override 368: onlyOwner 382: function sendPaprFromAuctionFees(address to, uint256 amount) external override onlyOwner { 386: function burnPaprFromAuctionFees(uint256 amount) external override onlyOwner {
File: src/PaprToken.sol 24: function mint(address to, uint256 amount) external onlyController { 28: function burn(address account, uint256 amount) external onlyController {
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 | 1 | 120 |
| [G‑02] | State variables should be cached in stack variables rather than re-reading them from storage | 2 | 194 |
| [G‑03] | <array>.length should not be looked up in every loop of a for-loop | 3 | 9 |
| [G‑04] | Using bools for storage incurs overhead | 3 | 51300 |
| [G‑05] | Using private rather than public for constants, saves gas | 1 | - |
| [G‑06] | Use custom errors rather than revert()/require() strings to save gas | 1 | - |
Total: 11 instances over 6 issues with 51623 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 is 1 instance of this issue:
File: src/ReservoirOracleUnderwriter.sol /// @audit oracleInfo - (valid but excluded finding) 64 function underwritePriceForCollateral(ERC721 asset, PriceKind priceKind, OracleInfo memory oracleInfo) 65 public 66: returns (uint256)
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 2 instances of this issue:
File: src/UniswapOracleFundingRateController.sol /// @audit pool on line 102 - (valid but excluded finding) 103: _lastTwapTick = UniswapHelpers.poolCurrentTick(pool); /// @audit pool on line 112 - (valid but excluded finding) 112: if (pool != address(0) && !UniswapHelpers.poolsHaveSameTokens(pool, _pool)) revert PoolTokensDoNotMatch();
<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 3 instances of this issue:
File: src/PaprController.sol /// @audit (valid but excluded finding) 99: for (uint256 i = 0; i < collateralArr.length;) { /// @audit (valid but excluded finding) 118: for (uint256 i = 0; i < collateralArr.length;) { /// @audit (valid but excluded finding) 370: for (uint256 i = 0; i < collateralConfigs.length;) {
bools for storage incurs overhead// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past
There are 3 instances of this issue:
File: src/PaprController.sol /// @audit (valid but excluded finding) 32: bool public override liquidationsLocked; /// @audit (valid but excluded finding) 35: bool public immutable override token0IsUnderlying; /// @audit (valid but excluded finding) 60: mapping(address => bool) public override isAllowed;
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/PaprController.sol /// @audit (valid but excluded finding) 30: uint256 public constant BIPS_ONE = 1e4;
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 is 1 instance of this issue:
File: src/libraries/OracleLibrary.sol /// @audit (valid but excluded finding) 39: require(twapDuration != 0, "BP");
#0 - c4-judge
2022-12-25T13:34:30Z
trust1995 marked the issue as grade-a
#1 - c4-judge
2022-12-25T18:10:14Z
trust1995 marked the issue as selected for report
#2 - trust1995
2022-12-25T18:10:25Z
Please also view #13 for an excellent gas report.