RFX Contracts - RFX Exchange

When a position is updated, the updateFundingAndBorrowingState function is triggered to adjust the CUMULATIVE_BORROWING_FACTOR. This factor is updated based on the latest borrowing rate (i.e.: borrowing factor per second) and the elapsed time since the last update. The borrowing rate itself depends on the percentage of the pool's liquidity that is currently borrowed: the higher the borrowed percentage, the higher the rate. By updating the CUMULATIVE_BORROWING_FACTOR before making any state changes that could influence the rate, the system ensures that borrowing fees are calculated accurately.

However, this process is not consistently applied throughout the code. When users withdraw or deposit funds, they alter the borrowing rate: withdrawals increase it, while deposits decrease it. Since the CUMULATIVE_BORROWING_FACTOR is not updated during a deposit or withdrawal, the next time it is updated (e.g.: when increasing / decreasing a position), the borrowing fees will be miscalculated because the rate will use the new value instead of the actual value that was present during the elapsed time. The described situation can result in excessive fees being charged if a withdrawal occurs, or insufficient fees being charged if a deposit occurs. The affected functions are the following:

• ExecuteDepositUtils.executeDeposit

• ExecuteWithdrawalUtils.executeWithdrawal

It is worth noting that large deposits or withdrawals can significantly amplify this fee inaccuracy. Excessive fees can unexpectedly push near-liquidateable positions into liquidation. Furthermore, this incremental rise in borrowing fees creates arbitrage opportunities, allowing attackers to exploit the inaccurate fee adjustments through well-timed orders and deposits.

Update the funding and borrowing state in the executeDeposit and executeWithdrawal functions before modifying the pool amount.

SOLVED: The RFX team solved the issue when refactoring the codebase to v2.1.

When creating a deposit, withdrawal, or order, the user's funds are transferred into a vault. A key referencing the requested operation is attached to the corresponding event upon creation. Once this event is detected, a keeper is triggered. The order keeper can then execute the requested operation by providing the necessary request key and oracle prices.

While prices are validated to ensure they are appropriate at the time of execution, they are not validated to ensure they fall within an acceptable range compared to the time of request creation. As a result, if there is a significant time gap between the request creation and execution, the operation may be executed with inappropriate prices. This scenario can potentially harm both users and disrupt the market's balance.

Code Location

Let's take the example of deposits: In order to execute a deposit, an order keeper calls the executeDeposit function within the exchange/DepositHandler contract:

function executeDeposit(
    bytes32 key,
    OracleUtils.SetPricesParams calldata oracleParams
) external payable
    globalNonReentrant
    onlyOrderKeeper
    withOraclePrices(oracle, dataStore, eventEmitter, oracleParams)
{
...
}

The OracleModule.withOraclePrices modifier is implemented as follows: it sets the oracle prices using the provided prices before executing the calling function, and subsequently clears all the prices:

modifier withOraclePrices(
    Oracle oracle,
    DataStore dataStore,
    EventEmitter eventEmitter,
    OracleUtils.SetPricesParams memory params
) {
    oracle.setPrices{value: msg.value}(dataStore, eventEmitter, params);
    _;
    oracle.clearAllPrices();
}

The Oracle.setPrices function has the below implementation:

function setPrices(
    DataStore dataStore,
    EventEmitter eventEmitter,
    OracleUtils.SetPricesParams memory params
) external payable onlyController {
    if (tokensWithPrices.length() != 0) {
        revert Errors.NonEmptyTokensWithPrices(tokensWithPrices.length());
    }

    _setPricesFromRealtimeFeeds(dataStore, eventEmitter, params);
}

The Oracle._setPricesFromRealtimeFeeds function has the below implementation:

function _setPricesFromRealtimeFeeds(
    DataStore dataStore,
    EventEmitter eventEmitter,
    OracleUtils.SetPricesParams memory params
) internal returns (OracleUtils.RealtimeFeedReport[] memory) {
    OracleUtils.RealtimeFeedReport[] memory reports = _validateRealtimeFeeds(
        dataStore,
        params.realtimeFeedTokens,
        params.realtimeFeedData
    );
    ....
}

The Oracle._validateRealtimeFeeds function has the below implementation:

function _validateRealtimeFeeds(
        DataStore dataStore,
        address[] memory realtimeFeedTokens,
        bytes[] memory realtimeFeedData
    ) internal returns (OracleUtils.RealtimeFeedReport[] memory) {
        ...
        uint256 dataCounter;
        for (uint256 i; i < realtimeFeedTokens.length; i++) {
            ...
            uint fee = pyth.getUpdateFee(feedData);

            /// @dev Reverts if the transferred fee is not sufficient or the updateData is invalid or there is
            /// no update for any of the given `priceIds` within the given time range
            uint64 minPublishTime = uint64(Chain.currentTimestamp() - maxPriceAge);
            uint64 maxPublishTime = uint64(Chain.currentTimestamp() + maxPriceAge);
            PythStructs.PriceFeed[] memory priceFeeds = pyth.parsePriceFeedUpdates{value: fee}(feedData, feedIds, minPublishTime, maxPublishTime);
            ...
        }
        ...
    }

The highlighted part will ensure that the prices provided to pyth.parsePriceFeedUpdates must fall within the specified minPublishTime and maxPublishTime. These timestamps are derived from the current block timestamp, thereby enforcing that the prices are within an acceptable range relative to the time of execution.

Going back to the exchange/DepositHandler.executeDeposit function, it later calls the _executeDeposit internal function, which calls the ExecuteDepositUtils.executeDepositfunction, which also fails to confirm that the provided prices align with an acceptable timeframe relative to the deposit creation time:

function executeDeposit(ExecuteDepositParams memory params, Deposit.Props memory deposit) external {
        // 63/64 gas is forwarded to external calls, reduce the startingGas to account for this
        params.startingGas -= gasleft() / 63;

        DepositStoreUtils.remove(params.dataStore, params.key, deposit.account());

        ExecuteDepositCache memory cache;

        if (deposit.account() == address(0)) {
            revert Errors.EmptyDeposit();
        }

        Market.Props memory market = MarketUtils.getEnabledMarket(params.dataStore, deposit.market());

        _validateFirstDeposit(params, deposit, market);

        MarketUtils.MarketPrices memory prices = MarketUtils.getMarketPrices(params.oracle, market);

        MarketUtils.distributePositionImpactPool(
            params.dataStore,
            params.eventEmitter,
            market.marketToken
        );
        ...

Consider validating that the oracle prices provided during the execution of the operation fall within an acceptable range relative to the prices at the time the operation was requested.

SOLVED: The RFX team solved the issue when refactoring the codebase to v2.1.

The _executeDeposit function in the ExecuteDepositUtils library calls the MarketUtils.validatePoolAmountForDeposit function if priceImpactUsd is greater than zero. However, if the pool's long token is near its deposit cap while the short token is not, depositing short tokens can lead to a positive price impact. This price impact might cause the long token's deposit cap to be exceeded and, as a consequence, the operation could unnecessarily revert.

In such scenarios, it is preferable to allow the pool to be rebalanced even if it results in surpassing the deposit cap for the long token.

Code Location

A call to the MarketUtils.validatePoolAmountForDeposit function when priceImpactUsd is greater than zero could cause an unnecessary revert:

if (_params.priceImpactUsd > 0) {
  // when there is a positive price impact factor,
  // tokens from the swap impact pool are used to mint additional market tokens for the user
  // for example, if 50,000 USDC is deposited and there is a positive price impact
  // an additional 0.005 ETH may be used to mint market tokens
  // the swap impact pool is decreased by the used amount
  //
  // priceImpactUsd is calculated based on pricing assuming only depositAmount of tokenIn
  // was added to the pool
  // since impactAmount of tokenOut is added to the pool here, the calculation of
  // the price impact would not be entirely accurate
  //
  // it is possible that the addition of the positive impact amount of tokens into the pool
  // could increase the imbalance of the pool, for most cases this should not be a significant
  // change compared to the improvement of balance from the actual deposit
  int256 positiveImpactAmount = MarketUtils.applySwapImpactWithCap(
    params.dataStore,
    params.eventEmitter,
    _params.market.marketToken,
    _params.tokenOut,
    _params.tokenOutPrice,
    _params.priceImpactUsd
  );

  // calculate the usd amount using positiveImpactAmount since it may
  // be capped by the max available amount in the impact pool
  // use tokenOutPrice.max to get the USD value since the positiveImpactAmount
  // was calculated using a USD value divided by tokenOutPrice.max
  //
  // for the initial deposit, the pool value and token supply would be zero
  // so the market token price is treated as 1 USD
  //
  // it is possible for the pool value to be more than zero and the token supply
  // to be zero, in that case, the market token price is also treated as 1 USD
  mintAmount += MarketUtils.usdToMarketTokenAmount(
    positiveImpactAmount.toUint256() * _params.tokenOutPrice.max,
    poolValue,
    marketTokensSupply
  );

  // deposit the token out, that was withdrawn from the impact pool, to mint market tokens
  MarketUtils.applyDeltaToPoolAmount(
    params.dataStore,
    params.eventEmitter,
    _params.market,
    _params.tokenOut,
    positiveImpactAmount
  );

  MarketUtils.validatePoolAmountForDeposit(
    params.dataStore,
    _params.market,
    _params.tokenOut
  );

  MarketUtils.validatePoolAmount(
    params.dataStore,
    _params.market,
    _params.tokenOut
  );
}

It is recommended to remove the call to the MarketUtils.validatePoolAmountForDeposit function if priceImpactUsd is greater than zero.

SOLVED: The RFX team solved the issue when refactoring the codebase to v2.1.

The Oracle contract relies on the Pyth Network for querying price feeds using pyth.parsePriceFeedUpdates, which necessitates a pre-calculated fee. However, the current payment logic has several issues:

1. The _validateRealtimeFeeds function does not validate whether the caller has provided sufficient ETH to cover the fees when calculating the fee. This oversight allows the caller to utilize contract funds and bypass the payment requirement.

   
   
   

2. The validateRealtimeFeeds function, which incurs fees by calling _validateRealtimeFeeds, lacks the payable modifier. Consequently, it does not allow callers to attach the necessary funds to cover fees. If the contract has insufficient funds, this function becomes temporarily disabled. Since the Oracle contract lacks a payable fallback or receive function, funds can only be transferred by either self-destructing from another contract to transfer funds or by attaching a large amount of ETH when calling setPrices, the only available payable function.

Code Location

The _validateRealtimeFeeds function does not validate whether the caller has provided sufficient ETH to cover the fees:

function _validateRealtimeFeeds(
        DataStore dataStore,
        address[] memory realtimeFeedTokens,
        bytes[] memory realtimeFeedData
    ) internal returns (OracleUtils.RealtimeFeedReport[] memory) {
        ...
        for (uint256 i; i < realtimeFeedTokens.length; i++) {
            ...
            uint fee = pyth.getUpdateFee(feedData);

            /// @dev Reverts if the transferred fee is not sufficient or the updateData is invalid or there is
            /// no update for any of the given `priceIds` within the given time range
            uint64 minPublishTime = uint64(Chain.currentTimestamp() - maxPriceAge);
            uint64 maxPublishTime = uint64(Chain.currentTimestamp() + maxPriceAge);
            PythStructs.PriceFeed[] memory priceFeeds = pyth.parsePriceFeedUpdates{value: fee}(feedData, feedIds, minPublishTime, maxPublishTime);
            ...
        }
        ...
    }

The validateRealtimeFeeds function calls the _validateRealtimeFeedsfunction. However, it lacks the payable modifier:

function validateRealtimeFeeds(
    DataStore dataStore,
    address[] memory realtimeFeedTokens,
    bytes[] memory realtimeFeedData
) external onlyController returns (OracleUtils.RealtimeFeedReport[] memory) {
    return _validateRealtimeFeeds(dataStore, realtimeFeedTokens, realtimeFeedData);
}

It is recommended to implement the following changes:

• Validate that the caller has attached sufficient ETH to cover the fee and refund any excess amount within _validateRealtimeFeeds.

• Add the payable keyword to the validateRealtimeFeeds function to allow callers to attach ETH for covering the fees.

SOLVED: The RFX team solved the issue when refactoring the codebase to v2.1.

In the _validateRealtimeFeeds function of the Oracle contract, price feeds are queried for each token using the respective feed ID to generate real-time feed reports, of type OracleUtils.RealtimeFeedReport, containing price details.

However, currently, the feedId field within each generated report is not being set. Consequently, each report lacks the necessary feed ID information that was used to query the underlying price details. This omission can lead to ambiguity or incorrect interpretation of the price data retrieved.

Code Location

Below is the definition of the OracleUtils.RealtimeFeedReport structure:

struct RealtimeFeedReport {
    // The feed ID the report has data for
    bytes32 feedId;
    // Min price
    uint256 minPrice;
    // Max price
    uint256 maxPrice;
    // Price exponent
    int32 expo;
    // Unix timestamp describing when the price was published
    uint publishTime;
}

In the _validateRealtimeFeeds function, the feedId field is not set for each report:

OracleUtils.RealtimeFeedReport memory report;
    if(baseFeedId != bytes32(0)) {
        report = OracleUtils.getQuoteBaseReport(priceFeeds[0].price, priceFeeds[1].price);
    } else {
        report.publishTime = priceFeeds[0].price.publishTime;
        report.expo = priceFeeds[0].price.expo;
        report.minPrice = uint256(uint64(priceFeeds[0].price.price) - priceFeeds[0].price.conf);
        report.maxPrice = uint256(uint64(priceFeeds[0].price.price) + priceFeeds[0].price.conf);
    }
    if (report.publishTime + maxPriceAge < Chain.currentTimestamp()) {
        revert Errors.RealtimeMaxPriceAgeExceeded(token, report.publishTime, Chain.currentTimestamp());
    }

    reports[i] = report;

Consider setting each report's feedId:

report.feedId = feedId;
reports[i] = report; 

SOLVED: The RFX team solved the issue when refactoring the codebase to v2.1.

The _revokeRole function in the RoleStore contract does not remove roleKey from the role set when there are no more members (i.e.: roleMembers[roleKey].length() is 0). As a consequence, the number of roles stored in the contract calculated in the getRoleCount function could be distorted from the current value.

Code Location

The _revokeRole function does not remove roleKey from role set when there are no more members:

function _revokeRole(address account, bytes32 roleKey) internal {
  roleMembers[roleKey].remove(account);
  roleCache[account][roleKey] = false;

  if (roleMembers[roleKey].length() == 0) {
    if (roleKey == Role.ROLE_ADMIN) {
      revert Errors.ThereMustBeAtLeastOneRoleAdmin();
    }
    if (roleKey == Role.TIMELOCK_MULTISIG) {
      revert Errors.ThereMustBeAtLeastOneTimelockMultiSig();
    }
  }
}

It is recommended to remove keys from the role set when there are no more members.

SOLVED: The RFX team solved the issue in the specified commit id.

The _autoTopUpSubaccount function in the SubaccountRouter contract verifies if the balance of wrapped native tokens for the account is sufficient before further processing. However, this validation is carried out before capping the top-up amount to the amount of native tokens used, which could result in some subaccounts not being correctly topped up.

Code Location

The _autoTopUpSubaccount function verifies the balance of wrapped native tokens for the account before capping the top-up amount:

function _autoTopUpSubaccount(address account, address subaccount, uint256 startingGas, uint256 executionFee) internal {
  uint256 amount = SubaccountUtils.getSubaccountAutoTopUpAmount(dataStore, account, subaccount);
  if (amount == 0) {
    return;
  }

  IERC20 wnt = IERC20(dataStore.getAddress(Keys.WNT));

  if (wnt.allowance(account, address(router)) < amount) { return; }
  if (wnt.balanceOf(account) < amount) { return; }

  // cap the top up amount to the amount of native tokens used
  uint256 nativeTokensUsed = (startingGas - gasleft()) * tx.gasprice + executionFee;
  if (nativeTokensUsed < amount) { amount = nativeTokensUsed; }

  router.pluginTransfer(
    address(wnt), // token
    account, // account
    address(this), // receiver
    amount // amount
  );

  TokenUtils.withdrawAndSendNativeToken(
    dataStore,
    address(wnt),
    subaccount,
    amount
  );

  EventUtils.EventLogData memory eventData;

  eventData.addressItems.initItems(2);
  eventData.addressItems.setItem(0, "account", account);
  eventData.addressItems.setItem(1, "subaccount", subaccount);

  eventData.uintItems.initItems(1);
  eventData.uintItems.setItem(0, "amount", amount);

  eventEmitter.emitEventLog2(
    "SubaccountAutoTopUp",
    Cast.toBytes32(account),
    Cast.toBytes32(subaccount),
    eventData
  );
}

It is recommended to verify the balance of wrapped native tokens for the account after capping the top-up amount.

SOLVED: The RFX team solved the issue in the specified commit id.

When executing market orders, the processOrder function within each order type manages order execution and returns event log data. This data can later be utilized within the afterOrderExecution function inside the order’s callback contract.

However, when executing order increases via IncreaseOrderUtils.processOrder, the function returns empty event log data, thereby preventing the callback from utilizing the execution results.

Code Location

The IncreaseOrderUtils.processOrder function returns an empty event log data after processing each order:

function processOrder(BaseOrderUtils.ExecuteOrderParams memory params) external returns (EventUtils.EventLogData memory) {
    ...

    EventUtils.EventLogData memory eventData;
    return eventData;
}

It is recommended to include the collateral token address and the corresponding increment amount used to increase the user’s position in the event log data. These parameters are returned from the SwapUtils.swap call:

(address collateralToken, uint256 collateralIncrementAmount) = SwapUtils.swap(SwapUtils.SwapParams(
...

SOLVED: The RFX team has solved this issue by including the collateral token address and the corresponding increment amount used to increase the user’s position in the event log data.

Some functions in the codebase do not include a zero address check for their parameters. If one of those parameters is mistakenly set to zero, it could affect the correct operation of the protocol. The affected functions are the following:

• DataStore.setAddress

• BaseOrderHandler.constructor

• DepositHandler.constructor

• FeeHandler.constructor

• MarketFactory.constructor

• OracleStore.constructor

• OracleStore.addSigner

• RoleModule.constructor

• SubaccountRouter.constructor

It is recommended to add a zero address check in the functions mentioned above.

ACKNOWLEDGED: The RFX team acknowledged this issue.

In the SwapUtils._swap function, there is a risk that the declared cache variable might not be utilized if one of the subsequent validations fails. This could lead to unnecessary gas consumption.

Code Location

The cache variable declaration might remain unused if one of the subsequent validations fails:

function _swap(SwapParams memory params, _SwapParams memory _params) internal returns (address, uint256) {
    SwapCache memory cache;

    if (_params.tokenIn != _params.market.longToken && _params.tokenIn != _params.market.shortToken) {
        revert Errors.InvalidTokenIn(_params.tokenIn, _params.market.marketToken);
    }

    MarketUtils.validateSwapMarket(params.dataStore, _params.market);

    cache.tokenOut = MarketUtils.getOppositeToken(_params.tokenIn, _params.market);

Consider declaring the cache variable immediately after the MarketUtils.validateSwapMarket check to ensure it is used in the intended context.

SOLVED: The RFX team has solved this issue by implementing the recommended change.

Some functions in the codebase are duplicated (i.e.: they contain the same code logic), which could make it harder to apply future changes in the common logic of the functions and can even introduce potential bugs. The affected functions are the following:

• DataStore.applyDeltaToUint function has duplicated code with DataStore.incrementUint.

• DataStore.applyDeltaToInt function has duplicated code with DataStore.incrementInt.

This situation is not security-related, but mentioned in the report as part of the DRY (Don't Repeat Yourself) principle used as a best practice in software development to improve the maintainability of code during all phases of its lifecycle.

It is recommended to update the codebase to call only one version of the duplicated functions and remove the other ones to avoid potential mistakes.

SOLVED: The RFX team solved the issue in the specified commit id.

In the ExecuteDepositUtils._executeDeposit function, the deposit logic is segregated based on the price impact factor. The initial if clause manages scenarios with a positive price impact, whereas the subsequent if clause handles those with a negative price impact. Given that the first if clause checks params.priceImpactUsd > 0, it logically follows that the second if clause checks params.priceImpactUsd < 0. However, since the conditions are mutually exclusive, the second if clause will never evaluate to true if the first condition is satisfied.

Code Location

The ExecuteDepositUtils._executeDeposit function contains an inefficient if clause:

if (_params.priceImpactUsd > 0) {
    ...
}

if (_params.priceImpactUsd < 0) {
    ...
}

It is recommended to use the else if construct as follows: else if (_params.priceImpactUsd < 0) to avoid redundant checks for a negative price impact after confirming a positive one.

SOLVED: The RFX team has solved this issue by implementing the recommended change.

Within the existing for loops, the following optimization principles can be applied to reduce gas:

Increment loop counter in an unchecked block:
In Solidity, most for loops use a uint256 counter variable that starts at 0 and increments by 1. Checking for over/underflow in these increments is unnecessary because the variable will exhaust gas long before reaching the maximum capacity of uint256.

Use ++i instead of i++:
With i++, the value of i (its old value) is returned before incrementing i to its new value. This results in two values being stored on the stack, regardless of whether you intend to use both. In contrast, ++i evaluates the increment operation first, updating i to its incremented value, and then returns this new value. This approach requires only one item to be stored on the stack.

Cache array lengths outside of loop:
When the length of an array is not cached outside of a loop, the Solidity compiler reads the length of the array during each iteration. For storage arrays, this leads to an additional SLOAD operation, incurring 100 extra gas for each iteration except the first. For memory arrays, an extra MLOAD operation is performed, adding 3 additional gas for each iteration except the first. Identifying loops where the length of a storage array is accessed in the loop condition without being modified inside the loop can highlight optimization opportunities.

The optimizations mentioned above can be implemented in the following cases:

• contracts/fee/FeeHandler.sol#L38

• contracts/oracle/OracleModule.sol#L56

• contracts/oracle/Oracle.sol#L316, L407

• contracts/swap/SwapUtils.sol#L136, L165

It is recommended to implement the adequate for loop optimizations in the following way:

Change:

for (uint256 i; i < array.length; i++) {..}

To:

length = array.length
for (uint256 i; i < length;) {
  ...
  unchecked{++i;} 
}

ACKNOWLEDGED: The RFX team acknowledged this issue.

Every Solidity file specifies in the header a version number of the format pragma solidity (^)0.8.*. The caret ( ^ ) before the version number implies an unlocked pragma, meaning that the compiler will use the specified version and above, hence the term "unlocked".

Contracts should be deployed using the same compiler version and flags that were used during their development and testing phases. Locking the pragma ensures consistency and prevents unintended deployment with a different pragma version. Using an outdated pragma version can introduce bugs that may adversely affect the contract system.

Code Location

All contracts within the codebase use an unlocked pragma:

pragma solidity ^0.8.0;

Consider locking the pragma version in the smart contracts. It is not recommended to use a floating pragma in production.
For example: pragma solidity 0.8.21;

ACKNOWLEDGED: The RFX team acknowledged this issue.

Within the codebase, the following components remain unused:

• Errors.CouldNotSendNativeToken error message

• Errors.HasRealtimeFeedId error message

• Errors.InvalidRealtimeFeedId error message

• Errors.InvalidRealtimeBidAsk error message

• Errors.InvalidRealtimeBlockHash error message

• Errors.InvalidRealtimePrices error message

• Errors.MaxPriceAgeExceeded error message

• Errors.MinOracleSigners error message

• Errors.MaxOracleSigners error message

• Errors.BlockNumbersNotSorted error message

• Errors.MinPricesNotSorted error message

• Errors.MaxPricesNotSorted error message

• Errors.InvalidFeedPrice error message

• Errors.PriceFeedNotUpdated error message

• Errors.MaxSignerIndex error message

• Errors.InvalidOraclePrice error message

• Errors.InvalidSignerMinMaxPrice error message

• Errors.InvalidMedianMinMaxPrice error message

• Errors.EmptyPriceFeed error message

• Errors.UnsupportedOracleBlockNumberType error message

• oracleParams argument in the BaseOrderHandler._getExecuteOrderParams function

• Oracle.ValidatedPrice struct

• Oracle.SetPricesCache struct

• Oracle.getStablePrice function

• Oracle.getPriceFeedMultiplier function

• Oracle._getSalt function

These elements are present within the codebase but are not currently utilized.

It is recommended to remove unused components in the codebase to save gas.

PARTIALLY SOLVED: The RFX team solved most of the issue when refactoring the codebase to v2.1. The only remaining unused component is the Errors.BlockNumbersNotSorted error message.

The following issues were identified in the NatSpec documentation:

OracleUtils.SetPricesParams struct:

• The compactedOracleBlockNumbers parameter is outdated and no longer exists.

• The realtimeFeedTokens and realtimeFeedData parameters lack NatSpec documentation.

OracleUtils.validateSigner function:

• The minOracleBlockNumber and maxOracleBlockNumber parameters are no longer used but are still documented.

ExecuteDepositUtils.ExecuteDepositParams struct:

• The oracleBlockNumbers parameter is deprecated and requires removal from NatSpec documentation.

• No NatSpec documentation exists for the depositVault parameter.

SwapUtils.SwapCache struct:

• The priceImpactUsd and priceImpactAmount parameters are undocumented.

Withdrawal.Addresses struct:

• The longTokenSwapPath and shortTokenSwapPath parameters lack NatSpec documentation.

Withdrawal.Flags struct:

• The shouldUnwrapNativeToken parameter's NatSpec comment is incomplete and needs revision:"whether to unwrap the native token when".

ExecuteWithdrawalParams struct:

• The minOracleBlockNumbers and maxOracleBlockNumbers parameters are outdated and should be removed from NatSpec documentation.

ExecuteWithdrawalUtils.executeWithdrawal function:

• The withdrawal parameter lacks NatSpec documentation.

Order.Numbers struct:

• The decreasePositionSwapType parameter is not documented within the NatSpec comments.

Market.Props struct:

• The data parameter is documented in NatSpec comments but does not exist and should be removed.

Consider updating the NatSpec comments by removing outdated parameters, documenting the missing ones, and completing unfinished comments.

PARTIALLY SOLVED: The RFX team has partially solved this issue when refactoring the codebase to v2.1. However, there are still some NatSpec comments that are either missing or incorrect:

SwapUtils.SwapCache struct:

• The priceImpactUsd and priceImpactAmount parameters are undocumented.

Withdrawal.Addresses struct:

• The longTokenSwapPath and shortTokenSwapPath parameters lack NatSpec documentation.

Withdrawal.Flags struct:

• The shouldUnwrapNativeToken parameter's NatSpec comment is incomplete and needs revision:"whether to unwrap the native token when".

ExecuteWithdrawalUtils.executeWithdrawal function:

• The withdrawal parameter lacks NatSpec documentation.

Order.Numbers struct:

• The decreasePositionSwapType parameter is not documented within the NatSpec comments.

Market.Props struct:

• The data parameter is documented in NatSpec comments but does not exist and should be removed.

Within the NatSpec documentation for the Oracle.getPriceFeedMultiplier function, the word "multiplier" was misspelled as "multipler":

// @return the price feed multipler

It is recommended to fix the present typo.

SOLVED: The RFX team solved the issue when refactoring the codebase to v2.1.

Incorporating comments into the codebase is essential for elucidating key aspects and facilitating comprehension of the developer's intentions by others. Nevertheless, several errors were identified within these comments:

In the OracleUtils contract, the RealtimeFeedReport structure’s comments incorrectly referenced bid and ask parameters, which have since been deprecated and renamed to minPrice and maxPrice respectively.

// bid: min price, highest buy price
// ask: max price, lowest sell price
struct RealtimeFeedReport {
    // The feed ID the report has data for
    bytes32 feedId;
    // Min price
    uint256 minPrice;
    // Max price
    uint256 maxPrice;
    // Price exponent
    int32 expo;
    // Unix timestamp describing when the price was published
    uint publishTime;
    }

In the Oracle contract, it was stated that the setPrices function checked the number of signers and then called _setPrices and _setPricesFromPriceFeeds to set tokens prices. However, this logic no longer applies in the current implementation.

// bits contain the index of each signer in the oracleStore. The function checks that the number
// of signers is greater than or equal to the minimum number of signers required, and that
// the signer indices are unique and within the maximum signer index. The function then calls
// _setPrices and _setPricesFromPriceFeeds to set the prices of the tokens.

Below is the implementation of the setPrices function:

function setPrices(
        DataStore dataStore,
        EventEmitter eventEmitter,
        OracleUtils.SetPricesParams memory params
    ) external payable onlyController {
        if (tokensWithPrices.length() != 0) {
            revert Errors.NonEmptyTokensWithPrices(tokensWithPrices.length());
        }

        _setPricesFromRealtimeFeeds(dataStore, eventEmitter, params);
    }

Furthermore, examples within the comments of the Oracle.setPrices function contained incorrect exponent values, leading to inaccurate prices:

In the comment below, (2 ^ 64) should have been (2 ^ 32) instead:

// USDC prices maximum value: `(2 ^ 64) / (10 ^ 6) => 4,294,967,296 / (10 ^ 6) => 4294.967296`.

In the comment below, (10 ^ 3) should have been (10 ^ 4) instead:

// Price would be stored as `1 * (10 ^ -26) * (10 ^ 30) => 1 * (10 ^ 3)`.

In the comment below, (2 ^ 64) should have been (2 ^ 32) instead:

// DG prices maximum value: `(2 ^ 64) / (10 ^ 11) => 4,294,967,296 / (10 ^ 11) => 0.04294967296`.

It is recommended to remove deprecated comments and correct any comments that contain errors.

SOLVED: The RFX team solved the issue when refactoring the codebase to v2.1.

Within the codebase, the following unused imports were identified:

oracle/OracleUtils.sol:

• import "../utils/Printer.sol";

oracle/Oracle.sol:

• import "@openzeppelin/contracts/utils/math/SafeCast.sol";

• import "@openzeppelin/contracts/utils/Strings.sol";

• import "./IPriceFeed.sol";

• import "./IRealtimeFeedVerifier.sol";

• import "../utils/Bits.sol";

• import "../utils/Array.sol";

• import "hardhat/console.sol";

utils/Precision.sol:

• import "@openzeppelin/contracts/utils/math/SafeMath.sol";

deposit/ExecuteDepositUtils.sol:

• import "../error/ErrorUtils.sol";

swap/SwapUtils.sol:

• import "../token/TokenUtils.sol";

withdrawal/ExecuteWithdrawalUtils.sol:

• import "../adl/AdlUtils.sol";

• import "../nonce/NonceUtils.sol";

• import "../oracle/OracleUtils.sol";

• import "../utils/AccountUtils.sol";

order/SwapOrderUtils.sol:

• import "../order/OrderStoreUtils.sol";

order/DecreaseOrderUtils.sol:

• import "../order/OrderStoreUtils.sol";

order/IncreaseOrderUtils.sol:

• import "../order/OrderStoreUtils.sol";

• import "../callback/CallbackUtils.sol";

Consider removing the unused import statements.

PARTIALLY SOLVED: The RFX team has partially solved this issue by removing most of the unused import statements. However, there is still one unused import that remains in the order/DecreaseOrderUtils.sol contract:

• import "../order/OrderStoreUtils.sol";