Papr contest - 0xSmartContract's results

Summary

Low Risk Issues List

Total 3 issues

Non-Critical Issues List

Total 12 issues

[L-01] twat value may be truncated

delta and twapDuration are int56 , but truncated with int24

Context:

src/libraries/OracleLibrary.sol:
  33      // adapted from https://github.com/Uniswap/v3-periphery/blob/main/contracts/libraries/OracleLibrary.sol#L30-L40
  34:     function timeWeightedAverageTick(int56 startTick, int56 endTick, int56 twapDuration)
  35:         internal
  36:         view
  37:         returns (int24 twat)
  38:     {
  39:         require(twapDuration != 0, "BP");
  40: 
  41:         unchecked {
  42:             int56 delta = endTick - startTick;
  43: 
  44:             twat = int24(delta / twapDuration);
  45: 
  46:             // Always round to negative infinity
  47:             if (delta < 0 && (delta % (twapDuration) != 0)) {
  48:                 twat--;
  49:             }
  50: 
  51:             twat;
  52:         }
  53:     }

Proof of Concept

contract Test is DSTest {

    Contract0 c0;
    Contract1 c1;

    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();

    }
    function test() public {
        c0.timeWeightedAverageTick(887008,887227000,3);
        c1.timeWeightedAverageTick(887008,887227000,3);
    }
}


contract Contract0{

   function timeWeightedAverageTick(int56 startTick, int56 endTick, int56 twapDuration)
        external
        view
        returns (int24 twat)

    {
        require(twapDuration != 0, "BP");
        unchecked {
            int56 delta = endTick - startTick;
            twat = int24(delta / twapDuration);
            // Always round to negative infinity
            if (delta < 0 && (delta % (twapDuration) != 0)) {
                twat--;
            }
            twat;
        }
    }
}



contract Contract1{

   function timeWeightedAverageTick(int56 startTick, int56 endTick, int56 twapDuration)
        external
        view
        returns (int56 twat)
    {
        require(twapDuration != 0, "BP");
        unchecked {
            int56 delta = endTick - startTick;
            twat = int56(delta / twapDuration);
            // Always round to negative infinity
            if (delta < 0 && (delta % (twapDuration) != 0)) {
                twat--;
            }
            twat;
        }
    }
}

Running 1 test for src/test/test.sol:Test
[PASS] test() (gas: 11586)
Traces:

  [11586] Test::test() 
    ├─ [682] Contract0::timeWeightedAverageTick(887008, 887227000, 3) [staticcall]
    │   └─ ← -6543224
    ├─ [682] Contract1::timeWeightedAverageTick(887008, 887227000, 3) [staticcall]
    │   └─ ← 295446664
    └─ ← ()

[L-02] Missing Event for critical parameters change

Context:

src/PaprController.sol:
  358  
  359:     /// @inheritdoc IPaprController
  360:     function setLiquidationsLocked(bool locked) external override onlyOwner {
  361:         liquidationsLocked = locked;
  362:     }

Description: Events help non-contract tools to track changes, and events prevent users from being surprised by changes

Recommendation: Add Event-Emit

[L-03] A single point of failure

PaprController.sol#L23

Impact

The owner role has a single point of failure and onlyOwner can use critical a few functions.

owner role in the project: Owner is not behind a multisig and changes are not behind a timelock.

Even if protocol admins/developers are not malicious there is still a chance for Owner keys to be stolen. In such a case, the attacker can cause serious damage to the project due to important functions. In such a case, users who have invested in project will suffer high financial losses.

onlyOwner functions;

5 results - 1 file

src/PaprController.sol:
  349      /// @inheritdoc IPaprController
  350:     function setPool(address _pool) external override onlyOwner {
  351          _setPool(_pool);

  354      /// @inheritdoc IPaprController
  355:     function setFundingPeriod(uint256 _fundingPeriod) external override onlyOwner {

  359      /// @inheritdoc IPaprController
  360:     function setLiquidationsLocked(bool locked) external override onlyOwner {
  361          liquidationsLocked = locked;

  382:     function sendPaprFromAuctionFees(address to, uint256 amount) external override onlyOwner {
  383          papr.safeTransferFrom(address(this), to, amount);

  385  
  386:     function burnPaprFromAuctionFees(uint256 amount) external override onlyOwner {
  387          PaprToken(address(papr)).burn(address(this), amount);

This increases the risk of A single point of failure

Tools Used

Manuel Code Review

Recommended Mitigation Steps

Add a time lock to critical functions. Admin-only functions that change critical parameters should emit events and have timelocks.

Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services.

Allow only multi-signature wallets to call the function to reduce the likelihood of an attack.

https://twitter.com/danielvf/status/1572963475101556738?s=20&t=V1kvzfJlsx-D2hfnG0OmuQ

Also detail them in documentation and NatSpec comments

[N-01] Use a single file for all system-wide constants

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.

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

5 results - 4 files

src/PaprController.sol:
  29      /// @dev what 1 = 100% is in basis points (bips)
  30:     uint256 public constant BIPS_ONE = 1e4;
  31  

src/ReservoirOracleUnderwriter.sol:
  34      /// @notice the amount of time to use for the TWAP
  35:     uint256 constant TWAP_SECONDS = 30 days;
  36  
  37      /// @notice the maximum time a given signed oracle message is valid for
  38:     uint256 constant VALID_FOR = 20 minutes;
  39  

src/libraries/PoolAddress.sol:
  7  library PoolAddress {
  8:     bytes32 internal constant POOL_INIT_CODE_HASH = 0xe34f199b19b2b4f47f68442619d555527d244f78a3297ea89325f843f87b8b54;
  9  

src/libraries/UniswapHelpers.sol:
  18  
  19:     IUniswapV3Factory constant FACTORY = IUniswapV3Factory(0x1F98431c8aD98523631AE4a59f267346ea31F984);

[N-02] NatSpec comments should be increased in contracts

Context:

src/libraries/OracleLibrary.sol:
src/NFTEDA/libraries/EDAPrice.sol:

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

[N-03] For functions, follow Solidity standard naming conventions

Context:

src/libraries/OracleLibrary.sol:

  10:     function getQuoteAtTick(int24 tick, uint128 baseAmount, address baseToken, address quoteToken)
  11:         internal
 
  34:     function timeWeightedAverageTick(int56 startTick, int56 endTick, int56 twapDuration)
  35:         internal
  
  56:     function latestCumulativeTick(address pool) internal view returns (int56) {


src/libraries/PoolAddress.sol:
  22:     function getPoolKey(address tokenA, address tokenB, uint24 fee) internal pure returns (PoolKey memory) {

  31:     function computeAddress(address factory, PoolKey memory key) internal pure returns (address pool) {

Description: The above codes don't follow Solidity's standard naming convention,

internal and private functions : the mixedCase format starting with an underscore (_mixedCase starting with an underscore)

[N-04] Function writing that does not comply with the Solidity Style Guide

Context: 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

[N-05] Add a timelock to critical functions

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:

src/PaprController.sol:
  349      /// @inheritdoc IPaprController
  350:     function setPool(address _pool) external override onlyOwner {
  351          _setPool(_pool);

  354      /// @inheritdoc IPaprController
  355:     function setFundingPeriod(uint256 _fundingPeriod) external override onlyOwner {
  356          _setFundingPeriod(_fundingPeriod);

  359      /// @inheritdoc IPaprController
  360:     function setLiquidationsLocked(bool locked) external override onlyOwner {
  361          liquidationsLocked = locked;

  364      /// @inheritdoc IPaprController
  365:     function setAllowedCollateral(IPaprController.CollateralAllowedConfig[] calldata collateralConfigs)
  366          external

[N-06] Use a more recent version of Solidity

Context:

10 results - 14 files

src/interfaces/IFundingRateController.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity >=0.8.0;

src/interfaces/IPaprController.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity >=0.8.0;

src/interfaces/IUniswapOracleFundingRateController.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity >=0.8.0;

src/libraries/OracleLibrary.sol:
  1  // SPDX-License-Identifier: GPL-2.0-or-later
  2: pragma solidity >=0.8.0;

src/libraries/PoolAddress.sol:
  3  // with updates for solc 8
  4: pragma solidity >=0.8.0;

src/libraries/UniswapHelpers.sol:
  1  // SPDX-License-Identifier: GPL-2.0-or-later
  2: pragma solidity >=0.8.0;

src/NFTEDA/NFTEDA.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity >=0.8.0;

src/NFTEDA/extensions/NFTEDAStarterIncentive.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity >=0.8.0;

src/NFTEDA/interfaces/INFTEDA.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity >=0.8.0;

src/NFTEDA/libraries/EDAPrice.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity >=0.8.0;

Description: For security, it is best practice to use the latest Solidity version. For the security fix list in the versions; https://github.com/ethereum/solidity/blob/develop/Changelog.md

Recommendation: Old version of Solidity is used , newer version can be used (0.8.17)

[N-07] Insufficient coverage

Description: The test coverage rate of the project is 55%. Testing all functions is best practice in terms of security criteria.

| File                                                                                 | % Lines          | % Statements     | % Branches      | % Funcs         |
|--------------------------------------------------------------------------------------|------------------|------------------|-----------------|-----------------|
| src/NFTEDA/NFTEDA.sol                                                                | 95.65% (22/23)   | 96.15% (25/26)   | 87.50% (7/8)    | 100.00% (5/5)   |
| src/NFTEDA/extensions/NFTEDAStarterIncentive.sol                                     | 70.00% (7/10)    | 72.73% (8/11)    | 100.00% (2/2)   | 80.00% (4/5)    |
| src/NFTEDA/libraries/EDAPrice.sol                                                    | 0.00% (0/5)      | 0.00% (0/9)      | 100.00% (0/0)   | 0.00% (0/1)     |
| src/PaprController.sol                                                               | 95.33% (143/150) | 94.92% (168/177) | 80.77% (42/52)  | 100.00% (27/27) |
| src/PaprToken.sol                                                                    | 100.00% (2/2)    | 100.00% (2/2)    | 100.00% (0/0)   | 100.00% (2/2)   |
| src/ReservoirOracleUnderwriter.sol                                                   | 75.00% (9/12)    | 80.00% (12/15)   | 62.50% (5/8)    | 0.00% (0/1)     |
| src/UniswapOracleFundingRateController.sol                                           | 100.00% (49/49)  | 98.31% (58/59)   | 95.45% (21/22)  | 100.00% (12/12) |
| Total                                                                                | 58.57% (263/449) | 58.11% (308/530) | 69.83% (81/116) | 55.20% (69/125) |

Due to its capacity, test coverage is expected to be 100%.

[N-08] Floating pragma

Description: Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. https://swcregistry.io/docs/SWC-103

Floating Pragma List:

4 results - 4 files

src/PaprController.sol:
  1  //SPDX-License-Identifier: BUSL-1.1
  2: pragma solidity ^0.8.17;
  3  

src/PaprToken.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity ^0.8.17;
  3  

src/ReservoirOracleUnderwriter.sol:
  1  // SPDX-License-Identifier: MIT
  2: pragma solidity ^0.8.17;
  3  

src/UniswapOracleFundingRateController.sol:
  1  //SPDX-License-Identifier: BUSL-1.1
  2: pragma solidity ^0.8.17;

Recommendation: Lock the pragma version and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

[N-09] Include return parameters in NatSpec comments

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);

[N-10] Omissions in Events

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:

src/PaprController.sol:
  349      /// @inheritdoc IPaprController
  350:     function setPool(address _pool) external override onlyOwner {
  351          _setPool(_pool);

  354      /// @inheritdoc IPaprController
  355:     function setFundingPeriod(uint256 _fundingPeriod) external override onlyOwner {
  356          _setFundingPeriod(_fundingPeriod);

  359      /// @inheritdoc IPaprController
  360:     function setLiquidationsLocked(bool locked) external override onlyOwner {
  361          liquidationsLocked = locked;

  364      /// @inheritdoc IPaprController
  365:     function setAllowedCollateral(IPaprController.CollateralAllowedConfig[] calldata collateralConfigs)
  366          external

[N-11] Need Fuzzing test

6 results - 2 files

src/PaprController.sol:
  101              collateralArr[i].addr.transferFrom(msg.sender, address(this), collateralArr[i].id);
  102:             unchecked {
  103:                 ++i;
  104              }

  130  
  131:             unchecked {
  132:                 ++i;
  133              }

  374              emit AllowCollateral(collateralConfigs[i].collateral, collateralConfigs[i].allowed);
  375:             unchecked {
  376:                 ++i;
  377              }

  436          uint16 newCount;
  437:         unchecked {
  438:             newCount = _vaultInfo[msg.sender][collateral.addr].count - 1;
  439              _vaultInfo[msg.sender][collateral.addr].count = newCount;

src/libraries/OracleLibrary.sol:
  14      {
  15:         unchecked {
  16:             uint160 sqrtRatioX96 = TickMath.getSqrtRatioAtTick(tick);
  17  

  40  
  41:         unchecked {
  42:             int56 delta = endTick - startTick;

Description: In total 2 contracts, 6 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

[N-12] Add comments for debug to requires

src/PaprController.sol:
  245:             require(amount1Delta > 0); 

src/libraries/PoolAddress.sol:
  32:         require(key.token0 < key.token1);

Gas Optimizations List

Total 12 issues

[G-01] Add _checkController() function to modifier onlyController()

Modifier code is copied in all instances where it's used, increasing bytecode size. By doing a refractor to the internal function, one can reduce bytecode size significantly at the cost of one JUMP.

https://github.com/PolymathNetwork/polymath-core/pull/548/commits/2dc0286f4e96241eed9603534607431a8a84ba35#diff-8b6746c2c4e7c9e3fca67d62718d70e8

src/PaprToken.sol:
  11:     modifier onlyController() {
  12         if (msg.sender != controller) {
  13             revert ControllerOnly();
  14         }
  15         _;
  16     }

Recommendation Code:

src/PaprToken.sol:
11:     modifier onlyController() {
+              _checkController();
- 12         if (msg.sender != controller) {
- 13             revert ControllerOnly();
- 14         }
15         _;
16     }


+ function _checkController() internal view {
+ 12      if (msg.sender != controller) {
+ 13         revert ControllerOnly();
+ 14    }
+  }

[G-02] No need for type conversion as pool is already an address

src/libraries/UniswapHelpers.sol:
  69     function deployAndInitPool(address tokenA, address tokenB, uint24 feeTier, uint160 sqrtRatio)
  70         internal
  71         returns (address)
  72     {
  73         IUniswapV3Pool pool = IUniswapV3Pool(FACTORY.createPool(tokenA, tokenB, feeTier));
  74         pool.initialize(sqrtRatio);
  75 
- 76:         return address(pool);
+ 76:         return pool;
  77     }

[G-03] Before you deploy your contract, activate the optimizer

foundry.toml:
   1: [profile.default]
   2: src = 'src'
   3: out = 'out'
   4: libs = ['lib']
   5: solc = "0.8.17"
   6: remappings = [
   7:     '@openzeppelin/=lib/openzeppelin-contracts/',
   8:     '@uniswap/v3-core/=lib/v3-core/',
   9:     '@uniswap/v3-periphery/=lib/v3-periphery/',
  10:     '@reservoir/=lib/oracle/contracts'
  11: ]
  12: fs_permissions = [{ access = "read", path = "./"}]

For optimizing gas costs, always use Solidity optimizer. It’s a good practice to set the optimizer as high as possible just until it no longer helps reducing gas costs on function calls. This can be recommended because function calls are intended to be executed way more times than the deployment of the contract, which happens just once. But, if the project you’re working on is really sensitive about deployment cost, playing with low optimizer values just until it no longer helps reducing the deployment cost should help.

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

[G-04] Use nested if and, avoid multiple check combinations

Using nested is cheaper than using && multiple check combinations. There are more advantages, such as easier to read code and better coverage reports.

3 results - 3 files

src\PaprController.sol:
  290:         if (isLastCollateral && remaining != 0) {

src\UniswapOracleFundingRateController.sol:
  112:         if (pool != address(0) && !UniswapHelpers.poolsHaveSameTokens(pool, _pool)) revert PoolTokensDoNotMatch();

src\libraries\OracleLibrary.sol:
  47:             if (delta < 0 && (delta % (twapDuration) != 0)) {

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L290 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/UniswapOracleFundingRateController.sol#L112 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/libraries/OracleLibrary.sol#L47

Recomendation Code:

src\PaprController.sol#L290

- if (isLastCollateral && remaining != 0) {
+ if (isLastCollateral) {
+ if (remaining != 0) {

[G-05] Use assembly to write address storage values

5 results - 2 files

src\ReservoirOracleUnderwriter.sol:
  51     constructor(address _oracleSigner, address _quoteCurrency) {
  52:         oracleSigner = _oracleSigner;
  53:         quoteCurrency = _quoteCurrency;

src\UniswapOracleFundingRateController.sol:
  34     constructor(ERC20 _underlying, ERC20 _papr, uint256 _targetMarkRatioMax, uint256 _targetMarkRatioMin) {
  35:         underlying = _underlying;
  36:         papr = _papr;

  111     function _setPool(address _pool) internal {
  115:         pool = _pool;

https://github.com/code-423n4/2022-12-Stealth-Project/blob/main/maverick-v1/contracts/models/Factory.sol#L30 https://github.com/code-423n4/2022-12-Stealth-Project/blob/main/maverick-v1/contracts/models/Factory.sol#L42 https://github.com/code-423n4/2022-12-Stealth-Project/blob/main/maverick-v1/contracts/models/Pool.sol#L78-L79 https://github.com/code-423n4/2022-12-Stealth-Project/blob/main/maverick-v1/contracts/models/Position.sol#L19 https://github.com/code-423n4/2022-12-Stealth-Project/blob/main/router-v1/contracts/Router.sol#L49-L51

Recommendation Code:

contracts\PairsContract.sol:

    function _setPool(address _pool) internal {
        assembly {
            sstore(pool.slot, _pool)
         }  
     }

[G-06] Setting the constructor to payable

You 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.

Context:

5 results - 5 files

src/PaprController.sol:
  67:     constructor(

src/PaprToken.sol:
  18:     constructor(string memory name, string memory symbol)

src/ReservoirOracleUnderwriter.sol:
  51:     constructor(address _oracleSigner, address _quoteCurrency) {

src/UniswapOracleFundingRateController.sol:
  34:     constructor(ERC20 _underlying, ERC20 _papr, uint256 _targetMarkRatioMax, uint256 _targetMarkRatioMin) {

src/NFTEDA/extensions/NFTEDAStarterIncentive.sol:
  33:     constructor(uint256 _auctionCreatorDiscountPercentWad) {

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L67 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprToken.sol#L18 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/ReservoirOracleUnderwriter.sol#L51 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/UniswapOracleFundingRateController.sol#L34 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/NFTEDA/extensions/NFTEDAStarterIncentive.sol#L33

Recommendation: Set the constructor to payable

[G-07] Functions guaranteed to revert_ when callled by normal users can be marked payable

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.

5 results - 1 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 {
  382:     function sendPaprFromAuctionFees(address to, uint256 amount) external override onlyOwner {
  386:     function burnPaprFromAuctionFees(uint256 amount) external override onlyOwner {

Recommendation: Functions guaranteed to revert when called by normal users can be marked payable  (for only onlyOwner functions)

[G-08] Use abi.encodePacked() instead of abi.encode()

4 results - 3 files

src/PaprController.sol:
  197:   abi.encode(msg.sender, collateralAsset, oracleInfo)
  509:   abi.encode(account, collateralAsset, oracleInfo)

src/libraries/PoolAddress.sol:
  40:    keccak256(abi.encode(key.token0, key.token1, key.fee)),

src/NFTEDA/NFTEDA.sol:
  36:    return uint256(keccak256(abi.encode(auction)));

Recommendation Code:

src/PaprController.sol:
- 197:   abi.encode(msg.sender, collateralAsset, oracleInfo)
+ 197:   abi.encodePacked(msg.sender, collateralAsset, oracleInfo)

[G-09] ++x costs less gas compared to x += 1

1 result - 1 file

src\PaprController.sol:

  413:     function _addCollateralToVault(address account, IPaprController.Collateral memory collateral) internal { 
- 419:         _vaultInfo[account][collateral.addr].count += 1;
+ 419:         ++_vaultInfo[account][collateral.addr].count;

  325          info.latestAuctionStartTime = uint40(block.timestamp);
- 326:         info.count -= 1;
+ 326:        --info.count;

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L419 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L326

[G-10] Optimize names to save gas

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 PaprContrroller.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

PaprControllerl.sol function names can be named and sorted according to METHOD ID

Sighash   |   Function Signature
========================
c06ef4b5  =>  addCollateral(IPaprController.Collateral[])
dcf8d683  =>  removeCollateral(address,IPaprController.Collateral[],ReservoirOracleUnderwriter.OracleInfo)
32fcb12a  =>  increaseDebt(address,ERC721,uint256,ReservoirOracleUnderwriter.OracleInfo)
242910be  =>  reduceDebt(address,ERC721,uint256)
150b7a02  =>  onERC721Received(address,address,uint256,bytes)
204d3ac0  =>  increaseDebtAndSell(address,ERC721,IPaprController.SwapParams,ReservoirOracleUnderwriter.OracleInfo)
4d8da1cf  =>  buyAndReduceDebt(address,ERC721,IPaprController.SwapParams)
fa461e33  =>  uniswapV3SwapCallback(int256,int256,bytes)
5e0925a4  =>  purchaseLiquidationAuctionNFT(Auction,uint256,address,ReservoirOracleUnderwriter.OracleInfo)
53e2a6de  =>  startLiquidationAuction(address,IPaprController.Collateral,ReservoirOracleUnderwriter.OracleInfo)
4437152a  =>  setPool(address)
2a5aa292  =>  setFundingPeriod(uint256)
ccc77541  =>  setLiquidationsLocked(bool)
82f3bebf  =>  setAllowedCollateral(IPaprController.CollateralAllowedConfig[])
a6acf125  =>  sendPaprFromAuctionFees(address,uint256)
93d7cde6  =>  burnPaprFromAuctionFees(uint256)
115042d3  =>  maxDebt(uint256)
a8559e32  =>  vaultInfo(address,ERC721)
89fbd332  =>  _addCollateralToVault(address,IPaprController.Collateral)
83d294b7  =>  _removeCollateral(address,IPaprController.Collateral,uint256,uint256)
b6d4cdbe  =>  _increaseDebt(address,ERC721,address,uint256,ReservoirOracleUnderwriter.OracleInfo)
eadaa21b  =>  _reduceDebt(address,ERC721,address,uint256)
99c581aa  =>  _reduceDebtWithoutBurn(address,ERC721,uint256)
0e27a944  =>  _increaseDebtAndSell(address,address,ERC721,IPaprController.SwapParams,ReservoirOracleUnderwriter.OracleInfo)
de052790  =>  _purchaseNFTAndUpdateVaultIfNeeded(Auction,uint256,address)
d4875cc4  =>  _handleExcess(uint256,uint256,uint256,Auction)
54aa2190  =>  _maxDebt(uint256,uint256)

[G-11] Use a more recent version of solidity

10 results - 10 files

src/interfaces/IFundingRateController.sol:
  2: pragma solidity >=0.8.0;

src/interfaces/IPaprController.sol:
  2: pragma solidity >=0.8.0;

src/interfaces/IUniswapOracleFundingRateController.sol:
  2: pragma solidity >=0.8.0;

src/libraries/OracleLibrary.sol:
  2: pragma solidity >=0.8.0;

src/libraries/PoolAddress.sol:
  4: pragma solidity >=0.8.0;

src/libraries/UniswapHelpers.sol:
  2: pragma solidity >=0.8.0;

src/NFTEDA/NFTEDA.sol:
  2: pragma solidity >=0.8.0;

src/NFTEDA/extensions/NFTEDAStarterIncentive.sol:
  2: pragma solidity >=0.8.0;

src/NFTEDA/interfaces/INFTEDA.sol:
  2: pragma solidity >=0.8.0;

src/NFTEDA/libraries/EDAPrice.sol:
  2: pragma solidity >=0.8.0;

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.

[G-12] The solady Library's Ownable contract is significantly gas-optimized, which can be used

The project uses the onlyOwner authorization model I recommend using Solady's highly gas optimized contract.

https://github.com/Vectorized/solady/blob/main/src/auth/OwnableRoles.sol

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)
  368:         onlyOwner
 
  382:     function sendPaprFromAuctionFees(address to, uint256 amount) external override onlyOwner {
  
  386:     function burnPaprFromAuctionFees(uint256 amount) external override onlyOwner {