Papr contest - RaymondFam's results

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Here is a contract instance with missing NatSpec in its entirety:

File: PaprToken.sol

Here is a contract instance partially missing NatSpec:

File: PaprController.sol

234:    function uniswapV3SwapCallback(int256 amount0Delta, int256 amount1Delta, bytes calldata _data) external {

All internal view and non-view functions

Avoid using abi.encodePacked() when dealing with dynamic types in passing result to keccak256()

As denoted in Solidity's documentation:

"If you use keccak256(abi.encodePacked(a, b)) and both a and b are dynamic types, it is easy to craft collisions in the hash value by moving parts of a into b and vice-versa. More specifically, abi.encodePacked("a", "bc") == abi.encodePacked("ab", "c"). If you use abi.encodePacked for signatures, authentication or data integrity, make sure to always use the same types and check that at most one of them is dynamic. Unless there is a compelling reason, abi.encode should be preferred."

Consider using abi.encode() that will pad item to 32 bytes to prevent hash collision. If there is only one argument to abi.encodePacked(), it can be cast to bytes32() instead. And, if all arguments are strings and/or bytes, bytes.concat() could be used instead.

Here is an instance entailed:

File: ReservoirOracleUnderwriter.sol#L69-L82

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

underwritePriceForCollateral() does not check if ecrecover return value is 0

In ReservoirOracleUnderwriter.sol, underwritePriceForCollateral() calls the Solidity ecrecover function directly to verify the given signatures. The return value of ecrecover may be 0, which means the signature is invalid, but the check can be bypassed when signer is 0.

File: ReservoirOracleUnderwriter.sol#L67-L90

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

        if (signerAddress != oracleSigner) {
            revert IncorrectOracleSigner();
        }

Consider using the recover function from OpenZeppelin's ECDSA library for signature verification.

Alternatively, the affected code line may be refactored as follows:

-        if (signerAddress != oracleSigner) {
+        if (signerAddress != oracleSigner || signerAddress == address(0)) {
            revert IncorrectOracleSigner();
        }

Events associated with setter functions

Consider having events associated with setter functions emit both the new and old values instead of just the new value.

Here are some of the instances entailed:

File: UniswapOracleFundingRateController.sol

117:        emit SetPool(_pool);

128:        emit SetFundingPeriod(_fundingPeriod);

papr over PaprToken(address(papr))

papr in PaprController.sol is already an ERC20 instance inherited from UniswapOracleFundingRateController.sol. As such casting papr into an address type and then reinitializing PaprToken(address(papr)) is unnecessary.

Here are the instances entailed:

File: PaprController.sol

387:        PaprToken(address(papr)).burn(address(this), amount);

476:        PaprToken(address(papr)).mint(mintTo, amount);

483:        PaprToken(address(papr)).burn(burnFrom, amount);

540:        PaprToken(address(papr)).burn(address(this), fee);

Immutable variables should be parameterized in the constructor

Consider assigning immutable variables in the constructor via parameter inputs instead of directly assigning them with literal values. If the latter approach is preferred, consider declaring them as constants.

Here are some of the instances entailed:

File: PaprController.sol#L41-L54

    uint256 public immutable override liquidationAuctionMinSpacing = 2 days;

    /// @inheritdoc IPaprController
    uint256 public immutable override perPeriodAuctionDecayWAD = 0.7e18;

    /// @inheritdoc IPaprController
    uint256 public immutable override auctionDecayPeriod = 1 days;

    /// @inheritdoc IPaprController
    uint256 public immutable override auctionStartPriceMultiplier = 3;

    /// @inheritdoc IPaprController
    /// @dev Set to 10%
    uint256 public immutable override liquidationPenaltyBips = 1000;

Constants should be defined rather than using magic numbers

Consider defining magic numbers to constants, serving to improve code readability and maintainability.

Here are some of the instances entailed:

File: PaprController.sol

87:            initSqrtRatio = UniswapHelpers.oneToOneSqrtRatio(underlyingONE, 10 ** 18);

89:            initSqrtRatio = UniswapHelpers.oneToOneSqrtRatio(10 ** 18, underlyingONE);

92:        address _pool = UniswapHelpers.deployAndInitPool(address(underlying), address(papr), 10000, initSqrtRatio);

Use delete to clear variables

delete 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 or a boolean to false. 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.

For instance, the a = 0 instance below may be refactored as follows:

File: PaprController.sol#L526

-            _vaultInfo[auction.nftOwner][auction.auctionAssetContract].latestAuctionStartTime = 0;
+            delete _vaultInfo[auction.nftOwner][auction.auctionAssetContract].latestAuctionStartTime ;

Solmate's contract existence check

As denoted by File: SafeTransferLib.sol#L9:

/// @dev Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.

As such, the following token transfers associated with Solmate's SafeTransferLib.sol, are performing low-level calls without confirming contract’s existence that could return success even though no function call was executed:

File: PaprController.sol

202:            underlying.transfer(params.swapFeeTo, fee);
203:            underlying.transfer(proceedsTo, amountOut - fee);

226:            underlying.transfer(params.swapFeeTo, amountIn * params.swapFeeBips / BIPS_ONE);

253:            underlying.safeTransferFrom(payer, msg.sender, amountToPay);

383:        papr.safeTransferFrom(address(this), to, amount);

514:            underlying.transfer(params.swapFeeTo, fee);
515:            underlying.transfer(proceedsTo, amountOut - fee);

546:            papr.transfer(auction.nftOwner, totalOwed - debtCached);

Minimization of truncation

The number of divisions in an equation should be reduced to minimize truncation frequency, serving to achieve higher precision. And, where deemed fit, comment the code line with the original multiple division arithmetic operation for clarity reason.

For instance, the code line below with two division may be refactored as follows:

File: UniswapHelpers.sol#L111

-        return TickMath.getSqrtRatioAtTick(TickMath.getTickAtSqrtRatio(uint160((token1ONE << 96) / token0ONE)) / 2);
+        return TickMath.getSqrtRatioAtTick(TickMath.getTickAtSqrtRatio(uint160((token1ONE << 96) / (token0ONE * 2));

Non-fungible nature of NFTs

It could be impractical at some point to price all NFTs of a collection with the same floor value considering some of them are going to be worth more than the others.

A typical scenario would be the NFT under collateral appreciated in price significantly due to multi-factorial reasons, and debt > max would not allow the user to redeem it particularly when this constituted the only token in the mapping count. When this happened, a technically savvy user could probably rescue it via an atomic flash loan transaction, but most of the time, the user would be helpless in this type of situation.

Consider implementing a rescue plan catering to this group of users where possible, as according to the whitepaper the protocol seems to be reaching out to the higher end collections of the NFTs that could be prone to this deadlock situation.

Liquidator hesitancy to purchase NFT

With NFT Exponential Dutch Auction (NFTEDA) still in progress, the 10% starter discount does not seem to be pressing enough for the liquidator to purchase the NFT with the auction he started. And if he had waited too long, someone else could have bought it and he wasted the effort calling startLiquidationAuction(). With competition and front running so vehement out there, consider offering more incentives to the liquidator to take part more eagerly. For instance, a little time window for him to match the auction price when someone has put in a reduced offer to secure the NFT.

Private function with embedded modifier reduces contract size

Consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that saves more gas on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.

For instance, the modifier instance below may be refactored as follows:

File: PaprToken.sol#L11-L16

+    function _onlyController() private view {
+        if (msg.sender != controller) {
+            revert ControllerOnly();
+        }
+    }

    modifier onlyController() {
-        if (msg.sender != controller) {
-            revert ControllerOnly();
-        }
+        _onlyController();
        _;
    }

Use of named returns for local variables saves gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable.

For instance, the code block below may be refactored as follows:

File: NFTEDAStarterIncentive.sol#L38-L40

-    function auctionStartTime(uint256 id) public view virtual override returns (uint256) {
+    function auctionStartTime(uint256 id) public view virtual override returns (uint256 _auctionStartTime) {
-        return auctionState[id].startTime;
+        _auctionStartTime = auctionState[id].startTime;
    }

Division by 2

A division by 2 can be calculated by shifting one to the right since the div opcode uses 5 gas while SHR opcode uses 3 gas. Additionally, Solidity's division operation also includes a division-by-0 prevention by pass using shifting.

Consider using >>1 by having the code instance below refactored as follows:

File: UniswapHelpers.sol#L111

-        return TickMath.getSqrtRatioAtTick(TickMath.getTickAtSqrtRatio(uint160((token1ONE << 96) / token0ONE)) / 2);
+        return TickMath.getSqrtRatioAtTick(TickMath.getTickAtSqrtRatio(uint160((token1ONE << 96) / token0ONE)) >> 1);

|| costs less gas than its equivalent &&

Rule of thumb: (x && y) is (!(!x || !y))

Even with the 10k Optimizer enabled: ||, OR conditions cost less than their equivalent &&, AND conditions.

As an example, the code line instance below may be refactored as follows:

File: UniswapOracleFundingRateController.sol#L112

-        if (pool != address(0) && !UniswapHelpers.poolsHaveSameTokens(pool, _pool)) revert PoolTokensDoNotMatch();
+        if (!(pool == address(0) || UniswapHelpers.poolsHaveSameTokens(pool, _pool))) revert PoolTokensDoNotMatch();

Use storage instead of memory for structs/arrays

A storage pointer is cheaper since copying a state struct in memory would incur as many SLOADs and MSTOREs as there are slots. In another words, this causes all fields of the struct/array to be read from storage, incurring a Gcoldsload (2100 gas) for each field of the struct/array, and then further incurring an additional MLOAD rather than a cheap stack read. As such, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables will be much cheaper, involving only Gcoldsload for all associated field reads. Read the whole struct/array into a memory variable only when it is being returned by the function, passed into a function that requires memory, or if the array/struct is being read from another memory array/struct.

Here are some of the instances entailed:

File: PaprController.sol

        IPaprController.OnERC721ReceivedArgs memory request = abi.decode(data, (IPaprController.OnERC721ReceivedArgs));

        IPaprController.Collateral memory collateral = IPaprController.Collateral(ERC721(msg.sender), _id);

Avoid late checks

Checks should be as early as possible to avoid wasting more gas than is needed when a function reverts.

For instance, the if block below should come before transferring the NFT collateral to sendTo:

File: PaprController.sol#L444-L451

-        collateral.addr.safeTransferFrom(address(this), sendTo, collateral.id);

       uint256 debt = _vaultInfo[msg.sender][collateral.addr].debt;
       uint256 max = _maxDebt(oraclePrice * newCount, cachedTarget);

       if (debt > max) {
           revert IPaprController.ExceedsMaxDebt(debt, max);
       }

+        collateral.addr.safeTransferFrom(address(this), sendTo, collateral.id);

Ternary over if ... else

Using ternary operator instead of the if else statement saves gas.

For instance, the code block below may be refactored as follows:

File: PaprController.sol#L283-L288

 excess > 0
     ? {
         remaining = _handleExcess(excess, neededToSaveVault, debtCached, auction);
     }
     : {
         _reduceDebt(auction.nftOwner, auction.auctionAssetContract, address(this), price);
         remaining = debtCached - price;
     }

Unneeded if blocks

block.timestamp ticks down or increments every second. The likelihood of _lastUpdated == block.timestamp is almost zero unless this condition is checked when _lastUpdated has been assigned block.timestamp in the same connected function calls, which is not seen throughout the protocol's code bases. As such, consider removing them to save gas both on deployment and function calls.

Here are the instances entailed:

File: UniswapOracleFundingRateController.sol#L46-L48

 -       if (_lastUpdated == block.timestamp) {
 -           return _target;
 -       }

File: UniswapOracleFundingRateController.sol#L64-L66

-        if (_lastUpdated == block.timestamp) {
-            return _target;
-        }

File: UniswapOracleFundingRateController.sol#L73-L75

-        if (_lastUpdated == block.timestamp) {
-            return _mark(_lastTwapTick);
-        }

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, consider replacing >= with the strict counterpart > in the following inequality instance:

File: PaprController.sol#L473

-        if (newDebt >= 1 << 200) revert IPaprController.DebtAmountExceedsUint200();
+        if (newDebt > 1 << 200 - 1) revert IPaprController.DebtAmountExceedsUint200();

+= and -= cost more gas

+= and -= generally cost 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop.

For example, the += instance below may be refactored as follows:

File: PaprController.sol#L419

-        _vaultInfo[account][collateral.addr].count += 1;
+        _vaultInfo[account][collateral.addr].count = _vaultInfo[account][collateral.addr].count + 1;

Similarly, the -= instance below may be refactored as follows:

File: PaprController.sol#L326

-        info.count -= 1;
+        info.count = info.count - 1;

Function order affects gas consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.17",
settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Please visit the following site for further information:

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

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Payable access control functions costs less gas

Consider marking functions with access control as payable. This will save 20 gas on each call by their respective permissible callers for not needing to have the compiler check for msg.value.

For instance, the code line below may be refactored as follows:

File: PaprController.sol#L350

-    function setPool(address _pool) external override onlyOwner {
+    function setPool(address _pool) external payable override onlyOwner {

Unchecked SafeMath saves gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops. When no arithmetic overflow/underflow is going to happen, unchecked { ... } to use the previous wrapping behavior further saves gas.

For instance, the following code block may be refactored as follows:

File: PaprController.sol#L419

+ unchecked{
        _vaultInfo[account][collateral.addr].count += 1;
+ }