Papr contest - Aymen0909's results

QA Report

Summary

Findings

1- Owner is unable to change auctionCreatorDiscountPercentWad value after deployment :

The value of the state variables auctionCreatorDiscountPercentWad and _pricePercentAfterDiscount is used to determine the discount in the auction price given to the auction starter as incentive, but this value can not be changed after the deployment of the PaprController contract as there is not external/public function to do so.

Even if the creator discount value was not intented to be changed in the protocol concepts, you should consider adding a setter function which will allow only the owner to update it (just in case it was needed in the future).

Risk : Low

Proof of Concept

The values of both auctionCreatorDiscountPercentWad and _pricePercentAfterDiscount are only set in the constructor of NFTEDAStarterIncentive contract :

constructor(uint256 _auctionCreatorDiscountPercentWad) {
    _setCreatorDiscount(_auctionCreatorDiscountPercentWad);
}

And _setCreatorDiscount is an internal function and there is no external/public function in the PaprController contract that calls it.

Mitigation

Consider adding an external function callable only by the onwer to allow the change of the creator discount value in the future.

2- Add a check to ensure that targetMarkRatioMax > targetMarkRatioMin :

The state variables targetMarkRatioMax and targetMarkRatioMin are immutable and their values are set in the consturctor of UniswapOracleFundingRateController, those values are used to limit the value of (target/mark) and are essentiel for the protocol working, so when setting them the constructor should check that targetMarkRatioMax is always greater than targetMarkRatioMin as there values can't be changed later on.

Risk : Low

Proof of Concept

The UniswapOracleFundingRateController constructor doesn't check the input values of both _targetMarkRatioMax and _targetMarkRatioMin variables :

constructor(ERC20 _underlying, ERC20 _papr, uint256 _targetMarkRatioMax, uint256 _targetMarkRatioMin) {
    underlying = _underlying;
    papr = _papr;

    targetMarkRatioMax = _targetMarkRatioMax;
    targetMarkRatioMin = _targetMarkRatioMin;

    _setFundingPeriod(4 weeks);
}

Mitigation

You should add a check statement to make sure that you always have targetMarkRatioMax > targetMarkRatioMin, the constructor should be modified as follow :

constructor(ERC20 _underlying, ERC20 _papr, uint256 _targetMarkRatioMax, uint256 _targetMarkRatioMin) {
    underlying = _underlying;
    papr = _papr;

    require(_targetMarkRatioMax > _targetMarkRatioMin);
    targetMarkRatioMax = _targetMarkRatioMax;
    targetMarkRatioMin = _targetMarkRatioMin;

    _setFundingPeriod(4 weeks);
}

3- setLiquidationsLocked function should emit an event :

The function setLiquidationsLocked is used to change the boolean variable liquidationsLocked which determine if auctions can be started or not, as this parameter is really critical for the protocol working this function should emit an event to allow all users to know when auctions are possible.

Risk : Low

Proof of Concept

The function setLiquidationsLocked doesn't emit an event :

File: src/PaprController.sol Line 360-362

function setLiquidationsLocked(bool locked) external override onlyOwner {
    liquidationsLocked = locked;
}

Mitigation

Emit an event in the setLiquidationsLocked function.

4- Non-library/interface files should use fixed compiler versions, not floating ones :

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.

check this source : https://swcregistry.io/docs/SWC-103

Risk : NON CRITICAL

Proof of Concept

There followings contract all use a floating solidity version pragma solidity ^0.8.17 :

File: src/PaprController.sol

File: src/PaprToken.sol

File: src/ReservoirOracleUnderwriter.sol

File: src/UniswapOracleFundingRateController.sol

File: src/NFTEDA/NFTEDA.sol

Mitigation

Lock the pragma version to the same version as used in the other contracts and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile it locally.

5- Use scientific notation :

When using multiples of 10 you shouldn't use decimal literals or exponentiation (e.g. 1000000, 10**18) but use scientific notation for better readability.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: src/PaprController.sol Line 54

uint256 public immutable override liquidationPenaltyBips = 1000;

File: src/PaprController.sol Line 87

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

File: src/PaprController.sol Line 89

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

File: src/PaprController.sol Line 92

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

Mitigation

Use scientific notation for the instances aforementioned.

Gas Optimizations

Summary

Findings

1- auctionCreatorDiscountPercentWad and _pricePercentAfterDiscount state variables should be declared immutable :

The state variables auctionCreatorDiscountPercentWad and _pricePercentAfterDiscount are only set in the constructor of NFTEDAStarterIncentive and their values can't be changed later on (there is no external/public function in PaprController contract to update them), so they should be marked immutable this will avoid a Gsset (20000 gas) in the constructor and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

There are 2 instances of this issue:

File: src/NFTEDA/extensions/NFTEDAStarterIncentive.sol Line 26-27

uint256 public auctionCreatorDiscountPercentWad;
uint256 internal _pricePercentAfterDiscount;

2- Variables inside struct should be packed to save gas :

As the solidity EVM works with 32 bytes, variables less than 32 bytes should be packed inside a struct so that they can be stored in the same slot, each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings.

There are 2 instances of this issue:

File: src/NFTEDA/interfaces/INFTEDA.sol Line 10-26

struct Auction {
    // the nft owner
    address nftOwner;
    // the nft token id
    uint256 auctionAssetID;
    // the nft contract address
    ERC721 auctionAssetContract;
    // How much the auction price will decay in each period
    // expressed as percent scaled by 1e18, i.e. 1e18 = 100%
    uint256 perPeriodDecayPercentWad;
    // the number of seconds in the period over which perPeriodDecay occurs
    uint256 secondsInPeriod;
    // the auction start price
    uint256 startPrice;
    // the payment asset and quote asset for startPrice
    ERC20 paymentAsset;
}

As the values of perPeriodDecayPercentWad and secondsInPeriod can't really overflow 2^128 (the first has a max value of 1e18 and the second is a period in seconds which can not realistically overflow) their values should be packed together and the struct should be modified as follow to save gas :

struct Auction {
    // the nft owner
    address nftOwner;
    // the nft token id
    uint256 auctionAssetID;
    // the nft contract address
    ERC721 auctionAssetContract;
    // How much the auction price will decay in each period
    // expressed as percent scaled by 1e18, i.e. 1e18 = 100%
    uint128 perPeriodDecayPercentWad;
    // the number of seconds in the period over which perPeriodDecay occurs
    uint128 secondsInPeriod;
    // the auction start price
    uint256 startPrice;
    // the payment asset and quote asset for startPrice
    ERC20 paymentAsset;
}

File: src/interfaces/IPaprController.sol Line 31-42

struct SwapParams {
    /// @dev amount of input token to swap
    uint256 amount;
    /// @dev minimum amount of output token to be received
    uint256 minOut;
    /// @dev sqrt price limit for the swap
    uint160 sqrtPriceLimitX96;
    /// @dev optional address to receive swap fees
    address swapFeeTo;
    /// @dev optional swap fee in bips
    uint256 swapFeeBips;
}

As the value of swapFeeBips can't really overflow 2^96 as it has a max value of 1e18, so it should be packed with sqrtPriceLimitX96 to save gas and the struct should be modified as follow :

struct SwapParams {
    /// @dev amount of input token to swap
    uint256 amount;
    /// @dev minimum amount of output token to be received
    uint256 minOut;
    /// @dev sqrt price limit for the swap
    uint160 sqrtPriceLimitX96;
    /// @dev optional swap fee in bips
    uint96 swapFeeBips;
    /// @dev optional address to receive swap fees
    address swapFeeTo;
}

3- The function purchaseLiquidationAuctionNFT can be optimized to save gas :

In the purchaseLiquidationAuctionNFT function there is the follwing lines of code which are used to calculate collateralValueCached :

File: src/PaprController.sol Line 270-273

uint256 collateralValueCached = underwritePriceForCollateral(
    auction.auctionAssetContract, ReservoirOracleUnderwriter.PriceKind.TWAP, oracleInfo
) * _vaultInfo[auction.nftOwner][auction.auctionAssetContract].count;
bool isLastCollateral = collateralValueCached == 0;

Those lines can be rearranged as follow to save gas at each call :

bool isLastCollateral; // by default set to false
uint256 collateralValueCached; // by default set to 0

uint16 count = _vaultInfo[auction.nftOwner][auction.auctionAssetContract].count;

if (count == 0){
    isLastCollateral = true;
    // no need to calculate collateralValueCached as it will remain equal to 0
} else {
    collateralValueCached = underwritePriceForCollateral(
        auction.auctionAssetContract, ReservoirOracleUnderwriter.PriceKind.TWAP, oracleInfo
    ) * count;
}

In this case the function will not call the underwritePriceForCollateral function when count is zero which will cost less gas for the sender.

4- Use unchecked blocks to save gas :

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

There are 4 instances of this issue:

1- File: src/PaprController.sol Line 278

uint256 neededToSaveVault = maxDebtCached > debtCached ? 0 : debtCached - maxDebtCached;

The operation debtCached - maxDebtCached cannot underflow because of the check maxDebtCached > debtCached which came before it, so the operation should be marked unchecked.

2- File: src/PaprController.sol Line 280

uint256 excess = price > neededToSaveVault ? price - neededToSaveVault : 0;

The operation price - neededToSaveVault cannot underflow because of the check price > neededToSaveVault which came before it, so the operation should be marked unchecked.

3- File: src/PaprController.sol Line 537

uint256 credit = excess - fee;

The above operation cannot underflow because of the line code which ensures that we always have fee <= excess :

File: src/PaprController.sol Line 536

uint256 fee = excess * liquidationPenaltyBips / BIPS_ONE;

So the operation should be marked unchecked.

4- File: src/PaprController.sol Line 550

remaining = debtCached - totalOwed;

The above operation cannot underflow because of the check :

File: src/PaprController.sol Line 542

if (totalOwed > debtCached) 

So the operation should be marked unchecked.

5- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

Using the addition operator instead of plus-equals saves 113 gas as explained here

There are 2 instances of this issue:

File: src/PaprController.sol Line 326

info.count -= 1;

File: src/PaprController.sol Line 419

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