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: 15/58
Findings: 2
Award: $748.30
🌟 Selected for report: 0
🚀 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 | Contexts | |
|---|---|---|
| LOW‑1 | Use _safeMint instead of _mint | 1 |
| LOW‑2 | Contracts are not using their OZ Upgradeable counterparts | 1 |
| LOW‑3 | Missing parameter validation in constructor | 2 |
| LOW‑4 | Prevent division by 0 | 1 |
| LOW‑5 | ecrecover may return empty address | 1 |
| LOW‑6 | Use of ecrecover is susceptible to signature malleability | 1 |
Total: 7 contexts over 6 issues
| Issue | Contexts | |
|---|---|---|
| NC‑1 | Critical Changes Should Use Two-step Procedure | 4 |
| NC‑2 | Use a more recent version of Solidity | 10 |
| NC‑3 | Missing event for critical parameter change | 1 |
| NC‑4 | Implementation contract may not be initialized | 5 |
| NC‑5 | Large multiples of ten should use scientific notation | 1 |
| NC‑6 | Use of Block.Timestamp | 5 |
| NC‑7 | Use bytes.concat() | 2 |
| NC‑8 | Use delete to Clear Variables | 2 |
| NC‑9 | Use Underscores for Number Literals | 1 |
| NC‑10 | No full coverage of in scope files | 1 |
Total: 41 contexts over 10 issues
_safeMint instead of _mintAccording to openzepplin's ERC721, the use of _mint is discouraged, use _safeMint whenever possible.
https://docs.openzeppelin.com/contracts/3.x/api/token/erc721#ERC721-_mint-address-uint256-
25: _mint(to, amount);
Use _safeMint whenever possible instead of _mint
The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.
9: import {Ownable2Step} from "openzeppelin-contracts/access/Ownable2Step.sol";
Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.
constructorSome parameters of constructors are not checked for invalid values.
51: address _oracleSigner 51: address _quoteCurrency
Validate the parameters.
On several locations in the code precautions are not being taken for not dividing by 0, this will revert the code.
These functions can be called with 0 value in the input, this value is not checked for being bigger than 0, that means in some scenarios this can potentially trigger a division by zero.
558: return maxLoanUnderlying / cachedTarget
Recommend making sure division by 0 won’t occur by checking the variables beforehand and handling this edge case.
ecrecover may return empty addressThere is a common issue that ecrecover returns empty (0x0) address when the signature is invalid. function recoverAddrImpl should check that before returning the result of ecrecover.
While unlikely to occur due to the value of oracleSigner being set in the constructor, it is still best practice to check for address(0x0) if oracleSigner was accidentally set as address(0x0) in addition that there is no address(0x0) checks in the constructor.
64: function underwritePriceForCollateral(ERC721 asset, PriceKind priceKind, OracleInfo memory oracleInfo) public returns (uint256) { address signerAddress = ecrecover( keccak256( abi.encodePacked( "\x19Ethereum Signed Message:\n32", // EIP-712 structured-data hash keccak256( abi.encode( keccak256("Message(bytes32 id,bytes payload,uint256 timestamp)"), oracleInfo.message.id, keccak256(oracleInfo.message.payload), oracleInfo.message.timestamp ) ) ) ), oracleInfo.sig.v, oracleInfo.sig.r, oracleInfo.sig.s );
See the solution here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.4.0/contracts/cryptography/ECDSA.sol#L68
ecrecover is susceptible to signature malleabilityThe built-in EVM precompile ecrecover is susceptible to signature malleability, which could lead to replay attacks.
References: https://swcregistry.io/docs/SWC-117, https://swcregistry.io/docs/SWC-121, and https://medium.com/cryptronics/signature-replay-vulnerabilities-in-smart-contracts-3b6f7596df57.
While this is not immediately exploitable, this may become a vulnerability if used elsewhere.
64: function underwritePriceForCollateral(ERC721 asset, PriceKind priceKind, OracleInfo memory oracleInfo) public returns (uint256) { address signerAddress = ecrecover( keccak256( abi.encodePacked( "\x19Ethereum Signed Message:\n32", // EIP-712 structured-data hash keccak256( abi.encode( keccak256("Message(bytes32 id,bytes payload,uint256 timestamp)"), oracleInfo.message.id, keccak256(oracleInfo.message.payload), oracleInfo.message.timestamp ) ) ) ), oracleInfo.sig.v, oracleInfo.sig.r, oracleInfo.sig.s );
Consider using OpenZeppelin’s ECDSA library (which prevents this malleability) instead of the built-in function. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L138-L149
The critical procedures should be two step process.
See similar findings in previous Code4rena contests for reference: https://code4rena.com/reports/2022-06-illuminate/#2-critical-changes-should-use-two-step-procedure
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)
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
<a href="https://blog.soliditylang.org/2021/04/21/solidity-0.8.4-release-announcement/">0.8.4</a>: bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)
<a href="https://blog.soliditylang.org/2022/02/16/solidity-0.8.12-release-announcement/">0.8.12</a>: string.concat() instead of abi.encodePacked(<str>,<str>)
<a href="https://blog.soliditylang.org/2022/03/16/solidity-0.8.13-release-announcement/">0.8.13</a>: Ability to use using for with a list of free functions
<a href="https://blog.soliditylang.org/2022/05/18/solidity-0.8.14-release-announcement/">0.8.14</a>:
ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases. Override Checker: Allow changing data location for parameters only when overriding external functions.
<a href="https://blog.soliditylang.org/2022/06/15/solidity-0.8.15-release-announcement/">0.8.15</a>:
Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays. Yul Optimizer: Keep all memory side-effects of inline assembly blocks.
<a href="https://blog.soliditylang.org/2022/08/08/solidity-0.8.16-release-announcement/">0.8.16</a>:
Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.
<a href="https://blog.soliditylang.org/2022/09/08/solidity-0.8.17-release-announcement/">0.8.17</a>:
Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
Consider updating to a more recent solidity version.
When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.
360: function setLiquidationsLocked(bool locked) external override onlyOwner {
OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5
67: constructor( string memory name, string memory symbol, uint256 _maxLTV, uint256 indexMarkRatioMax, uint256 indexMarkRatioMin, ERC20 underlying, address oracleSigner ) NFTEDAStarterIncentive(1e17) UniswapOracleFundingRateController(underlying, new PaprToken(name, symbol), indexMarkRatioMax, indexMarkRatioMin) ReservoirOracleUnderwriter(oracleSigner, address(underlying))
18: constructor(string memory name, string memory symbol) ERC20(string.concat("papr ", name), string.concat("papr", symbol), 18)
51: constructor(address _oracleSigner, address _quoteCurrency)
34: constructor(ERC20 _underlying, ERC20 _papr, uint256 _targetMarkRatioMax, uint256 _targetMarkRatioMin)
33: constructor(uint256 _auctionCreatorDiscountPercentWad)
Use (e.g. 1e6) rather than decimal literals (e.g. 100000), for better code readability.
92: address _pool = UniswapHelpers.deployAndInitPool(address(underlying), address(papr), 10000, initSqrtRatio);
Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts. References: SWC ID: 116
321: if (block.timestamp - info.latestAuctionStartTime < liquidationAuctionMinSpacing) {
394: if (_lastUpdated == block.timestamp) {
46: if (_lastUpdated == block.timestamp) {
64: if (_lastUpdated == block.timestamp) {
73: if (_lastUpdated == block.timestamp) {
Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.
Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.
bytes.concat()Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))
70: abi.encodePacked( "/x19Ethereum Signed Message:/n32", // EIP-712 structured-data hash keccak256( abi.encode( keccak256("Message(bytes32 id,bytes payload,uint256 timestamp)
37: abi.encodePacked( hex"ff", factory, keccak256(abi.encode(key.token0, key.token1, key.fee)
Use bytes.concat() and upgrade to at least Solidity version 0.8.4 if required.
delete to Clear Variablesdelete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.
The delete key better conveys the intention and is also more idiomatic. Consider replacing assignments of zero with delete statements.
526: _vaultInfo[auction.nftOwner][auction.auctionAssetContract].latestAuctionStartTime = 0;
58: secondAgos[0] = 0;
92: address _pool = UniswapHelpers.deployAndInitPool(address(underlying), address(papr), 10000, initSqrtRatio);
It is recommended to have full coverage on all in scope files of the project.
As stated in the scope of the project:
| SLOC | Coverage | |
|---|---|---|
| Total (over 14 files): | 1043 | 83.33% |
#0 - c4-judge
2022-12-25T08:52:33Z
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
394.4543 USDC - $394.45
| Issue | Contexts | Estimated Gas Saved | |
|---|---|---|---|
| GAS‑1 | <x> += <y> Costs More Gas Than <x> = <x> + <y> For State Variables | 2 | - |
| GAS‑2 | abi.encode() is less efficient than abi.encodepacked() | 7 | 700 |
| GAS‑3 | Multiplication/division By Two Should Use Bit Shifting | 1 | - |
| GAS‑4 | Public Functions To External | 7 | - |
| GAS‑5 | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 16 | - |
| GAS‑6 | Optimize names to save gas | 5 | 110 |
| GAS‑7 | internal functions only called once can be inlined to save gas | 22 | - |
| GAS‑8 | Setting the constructor to payable | 5 | 65 |
| GAS‑9 | Functions guaranteed to revert when called by normal users can be marked payable | 8 | 168 |
| GAS‑10 | Using unchecked blocks to save gas | 1 | 136 |
| GAS‑11 | Use solidity version 0.8.17 to gain some gas boost | 10 | 880 |
| GAS‑12 | State variables only set in the constructor should be declared immutable | 2 | 2000 |
Total: 86 contexts over 12 issues
<x> += <y> Costs More Gas Than <x> = <x> + <y> For State Variables326: info.count -= 1;
419: _vaultInfo[account][collateral.addr].count += 1;
abi.encode() is less efficient than abi.encodepacked()See for more information: https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison
197: abi.encode(msg.sender, collateralAsset, oracleInfo)
222: abi.encode(msg.sender)
509: abi.encode(account, collateralAsset, oracleInfo)
74: abi.encode(
40: keccak256(abi.encode(key.token0, key.token1, key.fee)),
36: return uint256(keccak256(abi.encode(auction)));
<x> * 2 is equivalent to <x> << 1 and <x> / 2 is the same as <x> >> 1. The MUL and DIV opcodes cost 5 gas, whereas SHL and SHR only cost 3 gas
111: return TickMath.getSqrtRatioAtTick(TickMath.getTickAtSqrtRatio(uint160((token1ONE << 96) / token0ONE)) / 2);
The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.
function updateTarget() public override returns (uint256 nTarget) {
function newTarget() public view override returns (uint256) {
function mark() public view returns (uint256) {
function auctionCurrentPrice(INFTEDA.Auction calldata auction) public view virtual returns (uint256) {
function auctionID(INFTEDA.Auction memory auction) public pure virtual returns (uint256) {
function auctionStartTime(uint256 id) public view virtual returns (uint256);
function auctionStartTime(uint256 id) public view virtual override returns (uint256) {
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
83: uint160 initSqrtRatio;
436: uint16 newCount;
23: uint8 v;
29: uint128 internal _target;
30: int56 internal _lastCumulativeTick;
31: uint48 internal _lastUpdated;
32: int24 internal _lastTwapTick;
21: uint16 count;
23: uint40 latestAuctionStartTime;
25: uint200 debt;
37: uint160 sqrtPriceLimitX96;
16: uint160 sqrtRatioX96 = TickMath.getSqrtRatioAtTick(tick);
42: int56 delta = endTick - startTick;
57: uint32[] memory secondAgos = new uint32[](1);
14: uint24 fee;
12: uint96 startTime;
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.
See more <a href="https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92">here</a>
File: PaprToken.sol
https://github.com/with-backed/papr/tree/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprToken.sol
File: ReservoirOracleUnderwriter.sol
File: UniswapOracleFundingRateController.sol
File: NFTEDA.sol
File: NFTEDAStarterIncentive.sol
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 Gauge.sol contract will be the most used; A lower method ID may be given.
internal functions only called once can be inlined to save gas424: function _removeCollateral
493: function _increaseDebtAndSell
519: function _purchaseNFTAndUpdateVaultIfNeeded
532: function _handleExcess
95: function _init
111: function _setPool
122: function _setFundingPeriod
156: function _multiplier
56: function latestCumulativeTick
22: function getPoolKey
31: function computeAddress
31: function swap
69: function deployAndInitPool
82: function poolCurrentTick
92: function poolsHaveSameTokens
100: function isUniswapPool
110: function oneToOneSqrtRatio
101: function _setAuctionStartTime
43: function _setAuctionStartTime
48: function _clearAuctionState
72: function _setCreatorDiscount
10: function currentPrice
constructor to payableSaves ~13 gas per instance
67: constructor( string memory name, string memory symbol, uint256 _maxLTV, uint256 indexMarkRatioMax, uint256 indexMarkRatioMin, ERC20 underlying, address oracleSigner ) NFTEDAStarterIncentive(1e17) UniswapOracleFundingRateController(underlying, new PaprToken(name, symbol), indexMarkRatioMax, indexMarkRatioMin) ReservoirOracleUnderwriter(oracleSigner, address(underlying))
18: constructor(string memory name, string memory symbol) ERC20(string.concat("papr ", name), string.concat("papr", symbol), 18)
51: constructor(address _oracleSigner, address _quoteCurrency)
34: constructor(ERC20 _underlying, ERC20 _papr, uint256 _targetMarkRatioMax, uint256 _targetMarkRatioMin)
33: constructor(uint256 _auctionCreatorDiscountPercentWad)
payableIf a function modifier or require such as onlyOwner/onlyX 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.
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) external override onlyOwner {
382: function sendPaprFromAuctionFees(address to, uint256 amount) external override onlyOwner {
386: function burnPaprFromAuctionFees(uint256 amount) external override onlyOwner {
24: function mint(address to, uint256 amount) external onlyController {
28: function burn(address account, uint256 amount) external onlyController {
Functions guaranteed to revert when called by normal users can be marked payable.
unchecked blocks to save gasSolidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block
42: int56 delta = endTick - startTick;
Upgrade to the latest solidity version 0.8.17 to get additional gas savings.
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
pragma solidity >=0.8.0;
constructor should be declared immutableAvoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).
26: uint256 public auctionCreatorDiscountPercentWad; 27: uint256 internal _pricePercentAfterDiscount;
#0 - c4-judge
2022-12-25T08:54:52Z
trust1995 marked the issue as grade-a
#1 - trust1995
2022-12-25T18:10:07Z
A very close second to the chosen Gas report.