bin2chen - ChainlinkFactory no convert decimals

[sherlock-admin2]

bin2chen

medium

ChainlinkFactory no convert decimals

Summary

ChainlinkFactory._parsePrices () parsing verifiedReports returns 8 decimals and is not converted to 18 decimals

Vulnerability Details

ChainlinkFactory._parsePrices () parses the verifiedReports, the code as follows:

    function _parsePrices(
        bytes32[] memory ids,
        bytes calldata data
    ) internal override returns (PriceRecord[] memory prices) {
        bytes[] memory verifiedReports = chainlink.verifyBulk{value: msg.value}(
            abi.decode(data, (bytes[])),
            abi.encode(feeTokenAddress)
        );
        if (verifiedReports.length != ids.length) revert ChainlinkFactoryInputLengthMismatchError();

        prices = new PriceRecord[](ids.length);
        for (uint256 i = 0; i < verifiedReports.length; i++) {
            (bytes32 feedId, , uint32 observationsTimestamp, , , , uint192 price) =
@>              abi.decode(verifiedReports[i], (bytes32, uint32, uint32, uint192, uint192, uint32, uint192));

            if (feedId != toUnderlyingId[ids[i]]) revert ChainlinkFactoryInvalidFeedIdError(feedId);

@>          prices[i] = PriceRecord(observationsTimestamp, Fixed18Lib.from(UFixed18.wrap(price)));
        }
    }

However, according to the chainlink documentation, the format of verifiedReports is as follows:

https://docs.chain.link/data-streams/reference/interfaces

contract StreamsUpkeep is ILogAutomation, StreamsLookupCompatibleInterface {
    struct BasicReport {
        bytes32 feedId; // The feed ID the report has data for
        uint32 validFromTimestamp; // Earliest timestamp for which price is applicable
        uint32 observationsTimestamp; // Latest timestamp for which price is applicable
        uint192 nativeFee; // Base cost to validate a transaction using the report, denominated in the chain’s native token (WETH/ETH)
        uint192 linkFee; // Base cost to validate a transaction using the report, denominated in LINK
        uint32 expiresAt; // Latest timestamp where the report can be verified onchain
 @>     int192 price; // DON consensus median price, carried to 8 decimal places
    }
...

price is 8 decimal, not 18 decimal UFixed18

and the price is int192 not uint192. In extreme cases, there may be a price of < 0

Impact

The wrong price unit may cause abnormal market calculations or an error in the MultiInvoker's order price trigger condition

Code Snippet

https://github.com/sherlock-audit/2024-02-perennial-v2-3/blob/main/perennial-v2/packages/perennial-oracle/contracts/chainlink/ChainlinkFactory.sol#L60

Tool used

Manual Review

Recommendation

1. Convert to 18 decimal

2. check that the price cannot be less than 0

   function _parsePrices(
       bytes32[] memory ids,
       bytes calldata data
   ) internal override returns (PriceRecord[] memory prices) {
       bytes[] memory verifiedReports = chainlink.verifyBulk{value: msg.value}(
           abi.decode(data, (bytes[])),
           abi.encode(feeTokenAddress)
       );
       if (verifiedReports.length != ids.length) revert ChainlinkFactoryInputLengthMismatchError();
   
       prices = new PriceRecord[](ids.length);
       for (uint256 i = 0; i < verifiedReports.length; i++) {
   -           (bytes32 feedId, , uint32 observationsTimestamp, , , , uint192 price) =
   -               abi.decode(verifiedReports[i], (bytes32, uint32, uint32, uint192, uint192, uint32, uint192));
   +           (bytes32 feedId, , uint32 observationsTimestamp, , , ,  int192 price) =
   +               abi.decode(verifiedReports[i], (bytes32, uint32, uint32, uint192, uint192, uint32, int192));
   +           price = price * 10**10; // 8 decimal to 18 decimal
   +           require(price>=0,"invalid price");
           if (feedId != toUnderlyingId[ids[i]]) revert ChainlinkFactoryInvalidFeedIdError(feedId);
   
           prices[i] = PriceRecord(observationsTimestamp, Fixed18Lib.from(UFixed18.wrap(price)));
       }
   }

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

panprog commented:

low, can be considered trusted admin action, and admin can easily do the conversion using the correct payoff function

[nevillehuang]

@panprog Could you point me to where could the admin potentially convert this?

[panprog]

@nevillehuang Each oracle has payoff function connected with it, which is applied to price returned by oracle:

https://github.com/sherlock-audit/2024-02-perennial-v2-3/blob/86ea166a2055f68c4a2c3ff0a2b6bef037476bb8/perennial-v2/packages/perennial-oracle/contracts/keeper/KeeperFactory.sol#L287-L299

So the price can be fixed by using the payoff which multiplies the price by 10**10. It's not optimal, yes, but I'm not sure it's severe enough for the medium as there is simply no impact. Even if the price is not fixed via payoff, the price will consistently be just in different decimals, meaning users can't lose funds due to this, although it can be unexpected. Then again, admin has all the tools to fix the decimals issue via payoff function.

[nevillehuang]

@panprog The price seems to be multiplied by itself, so wouldn't that be 10 8 decimals multiplied by each other twice? Are you indicating the admin would inject 10 10 into the payOff associated with the oracle?

CC: @bin2chen66 @kbrizzle @arjun-io

[panprog]

@panprog The price seems to be multiplied by itself, so wouldn't that be 10 8 decimals multiplied by each other twice? Are you indicating the admin would inject 10 10 into the payOff associated with the oracle?

Payoff can be anything, it's just that example payoff is square and sqrt, but it can easily change decimals instead of being power of price, or it can be both.

[nevillehuang]

@panprog I am unsure if such a fix would constitute this issue being invalid. @kbrizzle @arjun-io Would the workaround mentioned above affect the protocol in anyway?

[arjun-io]

We need to double check here, as the documentations we have from Chainlink (as well the feeds noted here: https://docs.chain.link/data-streams/stream-ids?network=arbitrum&page=1) list 18 decimals.

[nevillehuang]

@arjun-io @bin2chen66 Seems like there is no issue here if the above comment is true. Is it also true for Base network as well?

Edit: Seems like there is no USDC/USD oracle on base chain as of yet as seen here, so this issue seems invalid

[bin2chen66]

@nevillehuang OK.agree.thanks