ParaSpace contest - RaymondFam's results

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/misc/NFTFloorOracle.sol#L330-L333

Vulnerability details

Impact

In NFTFloorOracle.sol, the second condition in the if block of _removeFeeder() could revert, preventing the owner from removing a feeder. This could end up making the protocol forever stuck with unwanted feeder(s) who continued to retain its/their UPDATER_ROLE.

Proof of Concept

File: NFTFloorOracle.sol#L330-L333

330:        uint8 feederIndex = feederPositionMap[_feeder].index;
331:        if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {
332:            feeders[feederIndex] = feeders[feeders.length - 1];
333:            feeders.pop();

The above code block looks functional. Nevertheless, the issue would begin after the owner has successfully removed the first unwanted feeder.

Supposing there were 5 elements in the public array, feeders:

feeders = [A, B, C, D, E]

The owner decided to first remove feeders[2]. This would have feeders[4] replace feeders[2] and the size of feeders reduced by 1.

After a while, the owner wanted to remove feeder E. On line 330, feederIndex would be assigned 4 since feederPositionMap[E].index has not been changed. Consequently, the second if condition on line 331 would revert (instead of returning false to bypass the if block) simply because feeders[4] no longer existed.

As an illustration, foo() in the contract below would readily make the transaction revert to the initial state if an out of bound index > 4 were passed in as an argument.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.10;

contract OutOfBound {
    uint256[] public feeders = [1,2,3,4,5];
    function foo(uint256 index) public view returns(uint256) {
        return feeders[index];
    }
}

Tools Used

Manual inspection and Remix

Recommended Mitigation Steps

Consider having the affected code block refactored as follows by updating feederPositionMap[feeders[feederIndex]].index with the matching index to feeders:

        uint8 feederIndex = feederPositionMap[_feeder].index;
        if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {
            feeders[feederIndex] = feeders[feeders.length - 1];
            feeders.pop();
            feederPositionMap[feeders[feederIndex]].index = feederIndex;

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/misc/NFTFloorOracle.sol#L429

Vulnerability details

Impact

In NFTFloorOracle.sol, _combine() returns a validated medianPrice on line 429 after having validPriceList sorted in ascending order.

It will return a correct median as along as the array entails an odd number of valid prices. However, if there were an even number of valid prices, the median was supposed to be the average of the two middle values according to the median formula below:

Median Formula

The impact could be significant in edge cases and affect all function calls dependent on the finalized twap.

Proof of Concept

Let's assume there are four valid ether prices each of which is 1.5 times more than the previous one:

validPriceList = [1000, 1500, 2250, 3375]

Instead of returning (1500 + 2250) / 2 = 1875, 2250 is returned, incurring a 20% increment or 120 price deviation.

Tools Used

Manual Inspection

Recommended Mitigation Steps

Consider having line 429 refactored as follows:

        if (validNum % 2 != 0) {
            return (true, validPriceList[validNum / 2]);
        }
        else
            return (true, (validPriceList[validNum / 2] + validPriceList[(validNum / 2) - 1]) / 2); 

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/misc/ParaSpaceOracle.sol#L128

Vulnerability details

Impact

The NatSpec of ParaSpaceOracle.sol on line 18 is denoted as follows:

 * - Use of Chainlink Aggregators as first source of price

On line 128, the first source of price is retrieved using the highly discouraged and unreliable latestAnswer() featured by one of Chainlink's deployed oracles on etherscan. The impact could be significant in corrupting the lending/borrowing protocol that is heavily dependent on asset prices.

The deprecated latestAnswer() is delineated in Chainlink's associated:

1. NatSpec:

  /**
   * @notice Reads the current answer from aggregator delegated to.
   *
   * @dev #[deprecated] Use latestRoundData instead. This does not error if no
   * answer has been reached, it will simply return 0. Either wait to point to
   * an already answered Aggregator or use the recommended latestRoundData
   * instead which includes better verification information.
   */

2. Documentation:

Proof of Concept

File: ParaSpaceOracle.sol#L127-L132

127:        if (address(source) != address(0)) {
128:            price = uint256(source.latestAnswer());
129:        }
130:        if (price == 0 && address(_fallbackOracle) != address(0)) {
131:            price = _fallbackOracle.getAssetPrice(asset);
132:        }

Although the protocol did circumvent a possible glitch of zero price on lines 130 - 131 via the _fallbackOracle measure, price on line 128 could also end up assigned an erroneous/invalid value by the aggregators in an unfinished round.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider using latestRoundData() on top of specifically paying attention to the return parameters as exemplified in the code block below:

- price = uint256(source.latestAnswer());
+ (uint80 roundID, int256 price, /* uint80 startedAt */, uint80 timeStamp, uint80 answerInRound) = source.latestRoundData();
+ require(answerInRound >= roundID, "Stale price feed");
+ require(timestamp != 0, "Incomplete round of feed");
+ require(price > 0, "Zero price feed"); 
+ price = uint256(price);

Note: The following function definition will also need to be added to IEACAggregatorProxy.sol:

    function latestRoundData()
        external
        view
        returns (
            uint80 roundID,
            int256 price,
            uint256 startedAt,
            uint256 timeStamp,
            uint80 answerInRound
        );

Open TODOs

Open TODOs can point to architecture or programming issues that still need to be resolved. Consider resolving them before deploying.

Here are the instances entailed:

File: LooksRareAdapter.sol#L59

            makerAsk.price, // TODO: take minPercentageToAsk into accoun

File: UniswapV3OracleWrapper.sol#L238

        // TODO using bit shifting for the 2^96

File: MarketplaceLogic.sol#L442

        // TODO: support PToken

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 are the contract instances lacking NatSpec mostly in its entirety:

File: LooksRareAdapter.sol

File: SeaportAdapter.sol

File: X2Y2Adapter.sol

Here are the contract instances partially lacking NatSpec:

File: ParaSpaceOracle.sol

File: UniswapV3OracleWrapper.sol

No Storage Gap for Upgradeable Contracts

Consider adding a storage gap at the end of an upgradeable contract, just in case it would entail some child contracts in the future. This would ensure no shifting down of storage in the inheritance chain.

Here are the contract instances entailed:

File: NFTFloorOracle.sol

uint256[50] private __gap;

Unneeded Conditional Check

The first conditional check in the if block below is always going to return true since feederIndex can be any unsigned integer from zero onward.

File: NFTFloorOracle.sol#L331

        if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {

Consider removing it by having the code block refactored as follows:

        if (feeders[feederIndex] == _feeder) {

Merging Locally Declared Variable

Separately declaring an uninitialized local variable is unnecessary considering it may directly be assigned its supposed value.

Here is the instance entailed:

File: NFTFloorOracle.sol#L360-L365

360:        uint256 priceDeviation;
...
365:        priceDeviation = _twap > _priorTwap

The above two code lines may be merged into line 365 by getting priceDeviation directly assigned from the ternary block as follows:

365:        uint256 priceDeviation = _twap > _priorTwap

revokeRole() for _removeFeeder()

In NFTFloorOracle.sol, the modifier, onlyRole(DEFAULT_ADMIN_ROLE), has not been included as a function visibility for removeFeeder() because a similar check will be executed when externally calling revokeRole() in _removeFeeder().

Consider moving line 336, revokeRole(UPDATER_ROLE, _feeder);, to the beginning line of the function code block so that it would revert as early as possible whenever removeFeeder(), an external function, was invoked by a non-Admin.

Unneeded State Variable

In NFTFloorOracle.sol, a public state array, assets, has been declared to house all NFT contracts. Unlike its counterpart, feeders, no array trimming is implemented like it is seen in lines 332 - 333. Simply deleting it on line 301 turns the element to zero address by leaving a hole in the array, which would not be conducive if the ever growing assets were to be looked up. But since it is not practically referenced within or without the contract, e.g. in a for loop like feeders is, it may be deemed obsolete. If need be, the asset can be more meaningfully retrieved via the associated mappings, assetPriceMap and assetFeederMap.

In conjunction with this recommendation, the following associated code lines may also be removed from the contract:

35:    // index in asset list
36:    uint8 index;

283:        assets.push(_asset);
284:        assetFeederMap[_asset].index = uint8(assets.length - 1);

300:        uint8 assetIndex = assetFeederMap[_asset].index;
301:        delete assets[assetIndex];

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 the instances entailed:

File: ParaSpaceOracle.sol

109:     function _setFallbackOracle(address fallbackOracle) internal {

Missing Zero Address and Zero Value Checks

Zero address and zero value checks should be implemented at the constructor to avoid human error(s) that could result in non-functional calls associated with them particularly when the incidents involve immutable variables that could lead to contract redeployment at its worst.

In the instances entailed below, the address arrays may have the sanity checks correspondingly carried out in the for loop of _setAssetsSources():

File: ParaSpaceOracle.sol#L50-L55

        IPoolAddressesProvider provider,
        address[] memory assets,
        address[] memory sources,
        address fallbackOracle,
        address baseCurrency,
        uint256 baseCurrencyUnit

All other instances entailed:

File: UniswapV3OracleWrapper.sol#L24-L26

        address _factory,
        address _manager,
        address _addressProvider

Two-step Transfer of Ownership

In PoolAddressesProvider.sol, the contract inherits from Ownable.sol where transferOwnership() is invoked at the constructor allowing the deployer to transfer ownership to the owner specified in the constructor parameter. This function checks the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable. If the nominated EOA account is not a valid account, it is entirely possible the contract deployer may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier.

Consider implementing a two step process where the deployer nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This will ensure the new owner is going to be fully aware of the ownership assigned/transferred other than having the above mistake avoided.

Typo Mistakes

File: AuctionLogic.sol#L34

    @ tsatr
     * @notice Function to tsatr auction on an ERC721 of a position if its ERC721 Health Factor drops below 1.

Lines Too Long

Lines in source code are typically limited to 80 characters, but it’s reasonable to stretch beyond this limit when need be as monitor screens theses days are comparatively larger. Considering the files will most likely reside in GitHub that will have a scroll bar automatically kick in when the length is over 164 characters, all code lines and comments should be split when/before hitting this length. Keep line width to max 120 characters for better readability where possible.

Here are the instances entailed:

File: LiquidationLogic.sol#L603

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

Here are the instances entailed:

File: MarketplaceLogic.sol#L409-L410

            price = 0;
            downpayment = 0;

Unused Constructor

Empty constructor() that is not used can be removed to save deployment gas.

Here are the instances entailed:

File: LooksRareAdapter.sol#L23

    constructor() {}

File: SeaportAdapter.sol#L23

    constructor() {}

File: X2Y2Adapter.sol#L24

    constructor() {}

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 the instances entailed:

File: LooksRareAdapter.sol

32:            OrderTypes.TakerOrder memory takerBid,
33:            OrderTypes.MakerOrder memory makerAsk

37:        OfferItem[] memory offer = new OfferItem[](1);

47:        ConsiderationItem[] memory consideration = new ConsiderationItem[](1);

File: SeaportAdapter.sol

32:            AdvancedOrder memory advancedOrder,
33:            CriteriaResolver[] memory resolvers,

66:        (AdvancedOrder[] memory advancedOrders, , ) = abi.decode(

File: X2Y2Adapter.sol

37:        IX2Y2.RunInput memory runInput = abi.decode(params, (IX2Y2.RunInput));

40:        IX2Y2.SettleDetail memory detail = runInput.details[0];
41:        IX2Y2.Order memory order = runInput.orders[detail.orderIdx];
42:        IX2Y2.OrderItem memory item = order.items[detail.itemIdx];

49:        ERC721Pair[] memory nfts = abi.decode(item.data, (ERC721Pair[]));

53:        OfferItem[] memory offer = new OfferItem[](1);

63:        ConsiderationItem[] memory consideration = new ConsiderationItem[](1);

File: NFTFloorOracle.sol

357:        PriceInformation memory assetPriceMapEntry = assetPriceMap[_asset];

410:        uint256[] memory validPriceList = new uint256[](feederSize);

414:            PriceInformation memory priceInfo = feederRegistrar.feederPrice[

File: ParaSpaceOracle.sol

196:        uint256[] memory prices = new uint256[](assets.length);

File: UniswapV3OracleWrapper.sol

101:        UinswapV3PositionData memory positionData = getOnchainPositionData(

132:        UinswapV3PositionData memory positionData = getOnchainPositionData(

157:        UinswapV3PositionData memory positionData = getOnchainPositionData(

161:        PairOracleData memory oracleData = _getOracleData(positionData);

191:        uint256[] memory prices = new uint256[](tokenIds.length);

226:        PairOracleData memory oracleData;

294:        FeeParams memory feeParams;

File: PoolAddressesProvider.sol

        DataTypes.Marketplace memory marketplace = _marketplaces[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:

LooksRareAdapter.sol#L79-L85

    value == 0
        ? {
            selector = ILooksRareExchange.matchAskWithTakerBid.selector;
        } 
        : {
            selector = ILooksRareExchange
                .matchAskWithTakerBidUsingETHAndWETH
                .selector;
        }

All other instances entailed:

File: UniswapV3OracleWrapper.sol

324:             if (positionData.currentTick >= positionData.tickLower) {

339:            if (positionData.currentTick < positionData.tickUpper) {

File: PoolAddressesProvider.sol

213:        if (

276:        if (proxyAddress == address(0)) {

312:        if (proxyAddress == address(0)) {

344:        if (proxyAddress == address(0)) {

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.

As an example, the following instance of code block can refactored as follows:

File: LooksRareAdapter.sol#L73-L94

    function matchAskWithTakerBid(
        address marketplace,
        bytes calldata params,
        uint256 value
    ) external payable override returns (bytes memory _data) {
       
        ...

        _data =
            Address.functionCallWithValue(
                marketplace,
                data,
                value,
                Errors.CALL_MARKETPLACE_FAILED
            );
    }

All other instances entailed:

File: SeaportAdapter.sol

97:    ) external payable override returns (bytes memory) {

112:        returns (bytes memory)

127:        returns (bool)

File: X2Y2Adapter.sol

92:    ) external payable override returns (bytes memory) {

File: NFTFloorOracle.sol

261:    function getFeederSize() public view returns (uint256) {

270:    function _isAssetExisted(address _asset) internal view returns (bool) {

274:    function _isFeederExisted(address _feeder) internal view returns (bool) {

354:        returns (bool)

400:        returns (bool, uint256)

File: ParaSpaceOracle.sol

119:        returns (uint256)

142:        returns (uint256)

159:        returns (uint256[] memory)

176:        returns (uint256)

194:        returns (uint256[] memory)

208:        returns (address)

214:    function getFallbackOracle() external view returns (address) {

File: UniswapV3OracleWrapper.sol

54:        returns (UinswapV3PositionData memory)

156:    function getTokenPrice(uint256 tokenId) public view returns (uint256) {

189:        returns (uint256[] memory)

206:        returns (uint256)

224:        returns (PairOracleData memory)

Merging and Filtering Off Identical Imports

In LooksRareAdapter.sol, the following two lines of imports originate from the same ConsiderationStructs.sol:

File: LooksRareAdapter.sol#L10-L11

import {ConsiderationItem} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";
import {AdvancedOrder, CriteriaResolver, Fulfillment, OfferItem, ItemType} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";

Additionally, {AdvancedOrder, CriteriaResolver, Fulfillment} are not referenced in the contract. They may be merged into one import and refactored as follows:

import {ConsiderationItem, OfferItem, ItemType} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";

All other instances entailed:

File: SeaportAdapter.sol#L10-L11

import {ConsiderationItem} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";
import {AdvancedOrder, CriteriaResolver, Fulfillment, OfferItem, ItemType} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";

With {ConsiderationItem, OfferItem, ItemType} not refrenced in the contract, the above imports may be merged and refactored as follows:

import {AdvancedOrder, CriteriaResolver, Fulfillment} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";

File: X2Y2Adapter.sol#L11-L12

import {ConsiderationItem} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";
import {AdvancedOrder, CriteriaResolver, Fulfillment, OfferItem, ItemType} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";

With {AdvancedOrder, CriteriaResolver, Fulfillment} not refrenced in the contract, the above imports may be merged and refactored as follows:

import {ConsiderationItem, OfferItem, ItemType} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";

Unused Imports

Importing source units that are not referenced in the contract increases the size of deployment. Consider removing them to save some gas.

Here are the instances entailed:

File: LooksRareAdapter.sol

7: import {SeaportInterface} from "../../dependencies/seaport/contracts/interfaces/SeaportInterface.sol";

9: import {SignatureChecker} from "../../dependencies/looksrare/contracts/libraries/SignatureChecker.sol";

13: import {IERC1271} from "../../dependencies/openzeppelin/contracts/IERC1271.sol";

15: import {PoolStorage} from "../../protocol/pool/PoolStorage.sol";

File: SeaportAdapter.sol

6: import {OrderTypes} from "../../dependencies/looksrare/contracts/libraries/OrderTypes.sol";

8: import {ILooksRareExchange} from "../../dependencies/looksrare/contracts/interfaces/ILooksRareExchange.sol";

9: import {SignatureChecker} from "../../dependencies/looksrare/contracts/libraries/SignatureChecker.sol";

10: import {ConsiderationItem} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";

13: import {IERC1271} from "../../dependencies/openzeppelin/contracts/IERC1271.sol";

15: import {PoolStorage} from "../../protocol/pool/PoolStorage.sol";

File: X2Y2Adapter.sol

6: import {OrderTypes} from "../../dependencies/looksrare/contracts/libraries/OrderTypes.sol";

7: import {SeaportInterface} from "../../dependencies/seaport/contracts/interfaces/SeaportInterface.sol";

8: import {ILooksRareExchange} from "../../dependencies/looksrare/contracts/interfaces/ILooksRareExchange.sol";

10: import {SignatureChecker} from "../../dependencies/looksrare/contracts/libraries/SignatureChecker.sol";

14: import {IERC1271} from "../../dependencies/openzeppelin/contracts/IERC1271.sol";

16: import {PoolStorage} from "../../protocol/pool/PoolStorage.sol";

Split Require Statements Using &&

Instead of using the && operator in a single require statement to check multiple conditions, using multiple require statements with 1 condition per require statement will save 3 GAS per &&.

Here are the instances entailed:

File: SeaportAdapter.sol

43:            resolvers.length == 0 && isBasicOrder(advancedOrder)

73:            advancedOrders.length == 2 &&
74:                isBasicOrder(advancedOrders[0]) &&

File: MarketplaceLogic.sol

172:            marketplaceIds.length == payloads.length &&

280:            marketplaceIds.length == payloads.length &&

|| 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 block below may be refactored as follows:

File: ParaSpaceOracle.sol#L130

        if (!(price != 0 || address(_fallbackOracle) == address(0))) {

All other instances entailed:

[File: PoolAddressesProvider.sol]https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/protocol/configuration/PoolAddressesProvider.sol)

214:            marketplace.marketplace != address(0) &&

Private Function 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: NFTFloorOracle.sol#L110-L115

    function _whenNotPaused(address _asset) private view {
        if (!hasRole(DEFAULT_ADMIN_ROLE, msg.sender)) {
            _whenNotPaused(_asset);
        }
    }

    modifier whenNotPaused(address _asset) {
        _whenNotPaused(_asset);
        _;
    }

All other modifier instances entailed:

File: NFTFloorOracle.sol#L117-L135

117:    modifier onlyWhenAssetExisted(address _asset) {

122:    modifier onlyWhenAssetNotExisted(address _asset) {

127:    modifier onlyWhenFeederExisted(address _feeder) {

132:    modifier onlyWhenFeederNotExisted(address _feeder) {

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.10",
    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.

Here are the instances entailed:

File: NFTFloorOracle.sol

141:        onlyRole(DEFAULT_ADMIN_ROLE)

150:        onlyRole(DEFAULT_ADMIN_ROLE)

160:        onlyRole(DEFAULT_ADMIN_ROLE)

177:        onlyRole(DEFAULT_ADMIN_ROLE)

185:        onlyRole(DEFAULT_ADMIN_ROLE)

197:        onlyRole(UPDATER_ROLE)

File: ParaSpaceOracle.sol

69:    ) external override onlyAssetListingOrPoolAdmins {

77:        onlyAssetListingOrPoolAdmins

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: NFTFloorOracle.sol#L370

        if (priceDeviation > config.maxPriceDeviation - 1) {

All other instances entailed:

File: UniswapV3OracleWrapper.sol#L324

            if (positionData.currentTick >= positionData.tickLower) {

Similarly, as an example, consider replacing <= with the strict counterpart < in the following inequality instance:

File: NFTFloorOracle.sol#L244

            (block.number - updatedAt) < config.expirationPeriod + 1,

All other <= instances entailed:

File: NFTFloorOracle.sol

419:                if (diffBlock <= config.expirationPeriod) {

441:        while (i <= j) {

444:            if (i <= j) {

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 just as in the for loop below as an example:

File: NFTFloorOracle.sol#L229-L231

    unchecked {
        for (uint256 i = 0; i < _assets.length; i++) {
            setPrice(_assets[i], _twaps[i]);
        }
    }

All other instances enailed:

File: NFTFloorOracle.sol

321:        for (uint256 i = 0; i < _feeders.length; i++) {

413:        for (uint256 i = 0; i < feederSize; i++) {

442:            while (arr[uint256(i)] < pivot) i++;
443:            while (pivot < arr[uint256(j)]) j--;

File: ParaSpaceOracle.sol

95:        for (uint256 i = 0; i < assets.length; i++) {

File: UniswapV3OracleWrapper.sol#L324

193:        for (uint256 index = 0; index < tokenIds.length; index++) {

210:        for (uint256 index = 0; index < tokenIds.length; index++) {

+= and -= Costs More Gas

+= generally costs 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. As an example, the following line of code could be rewritten as:

File: UniswapV3OracleWrapper.sol#L211

            sum = sum + getTokenPrice(tokenIds[index]);

All other instances entailed:

File: UniswapV3OracleWrapper.sol

        token0Amount += positionData.tokensOwed0;
        token1Amount += positionData.tokensOwed1;