Coinshift: Chainlink Integration

Description

Prices can only be used from max 1 day old, due to several checks:

The functions getPrice(), getPriceInfo(), getPriceInfoscalls() all call the following functions:

• _readRound()
• _validateAndNormalize()
• _chainlinkIsFrozen()

The function _chainlinkIsFrozen() checks if the price has been updated < MAX_PRICE_AGE (e.g. 1 day). This means only prices upto 1 day ago can be retrieved.

Additionally there are checks in IUSPCHub::processSubscriptions() and IUSPCHub::_processSingleRedemption() which also only allow timestamp upto MAX_PRICE_AGE ago.

Recommendation

If older prices are ever relevant (possibly in non standard situations where the contracts have been paused for a day), then consider updating the checks or have a workaround.

Coinshift

Acknowledged. This is intentional behavior, staleness policy is by design. The Hub also independently enforces MAX_PRICE_AGE at processing time. Both contracts have the 24 hour window.

Description

In function _checkDeviation() the calculation of currentResponse.roundId - 1 isn't correct if the underlying aggregater has changed. In that situation high bits of roundId are updated and the lower bits part of roundId will be set to 1.

Recommendation

This situation can be detected by the check uint64(previousRoundId) == 0.

The most straightforward solution is to revert, because finding the highest roundId of the previous aggregater is difficult.

Consider changing the code to:

-if (currentResponse.roundId <= 1 || ...) return;+if ( uint64(currentResponse.roundId) <= 1 || ... ) return; // strip the high bits

Description

The value of answeredInRound is deprecated, see the chainlink documentation. Some oracles might return a value of 0, which will result in a revert during the check.

Recommendation

Consider ignoring answeredInRound.

Description

The uptime of the L2 isn't taken into account in the Pricer contract. The project indicated that the current plans are to deploy on ethereum mainnet.

Recommendation

If the protocol will be deployed on a L2, doublecheck the usefulness of the l2-sequencer-feeds.

Coinshift

Acknowledged. We will be deploying on eth mainnet. L2 sequencer uptime checks are not applicable.

Description

Several functions, events and errors are not exposed in IPricer.sol. This makes interfacing with them more difficult.

Recommendation

Consider add the function definitions to IPricer.sol and moving the errors to IPricer.sol.

Description

Several underlying init functions are not called from initialize(). The functions are currenlty empty, but that might be changed in the future.

Recommendation

Consider also calling:

• __ERC165_init()
• __Context_init()

Coinshift

Achknowledged. These are currently empty no ops. We'll review init chains when upgrading oz dependencies.

Description

Deposits and redemptions are processed with a Chainlink round id(priceId) chosen to match the time of the operation.

If that round fails validation (e.g. DeviationTooHigh, StalePriceData, PriceNotFound), it is not possible to use it for that operation:

• The operation can not be processed with the deposit-time priceId.
• The only way to process it is for the admin to call processSubscriptions / processRedemptions with a different priceId — i.e. a different Chainlink round, which corresponds to a different time than when the deposit (or redemption) actually occurred.

So when the correct round for the deposit/redemption time is unusable, the admin is forced to price the user at another moment in time, which can be seen as a temporal mismatch or fairness concern (user is minted/redeemed at a price from a time other than their operation time).

Recommendation

• Document that if the round at deposit/redemption time cannot be used (e.g. due to deviation or staleness), the operation can only be processed by using another round.

Coinshift

Acknowledged. This is Intentional behavior. The system should halt rather than process at stale prices. Adding an admin override increases attack surface for an extreme scenario.

The admin gathers roundIds off-chain, if any roundId fails validation the batch reverts and admin resubmits with valid roundIds.

Description

The function setChainlinkFeed() doesn't emit all changes.

Recommendation

Consider also emitting the old and new decimals.

Coinshift

Acknowledged. Feed decimals are queryable on chain from the feed contract directly. Not worth changing the event signature for data that is already accessible.

Description

Impractical values can be set, which will limit any changes to the prices.

• maxPriceDeviationBps can be set to 0
• minAllowedPrice can be set to the same values as maxAllowedPrice

Recommendation

Consider disallowing to set impractical values.

Conshift

Acknowledged. It is on a trust based system, admin is trusted with role gated access.

Description

When rapid price changes occur and the oracle is frequently updated the price difference between two updates can be small enough to not trigger deviations. Also when the price flattens out the prices difference will certainly be low between oracle updates.

Recommendation

Decide what actions to take during and direct after rapid price changes. Consider taking the time between oracle updates into account.

Coinshift

Acknowledged. This behaviour is expected, time weighting will add significant complexity.

Description

The try/catch in function _getCurrentChainlinkResponse() directly returns. This means the transaction will directly revert and fallbackDecimals will never be used.

Recommendation

Doublecheck the intended action. If this is intentional then consider removing fallbackDecimals.

Description

Function _getRoundChainlinkResponse() doesn't check decimals(), while function _getCurrentChainlinkResponse() does perform this check. This is inconsistent.

Recommendation

If you want anticipate a change in decimals() then it should be implemented consistently.

And to be fully consistent the following check, which is also done in setChainlinkFeed(), could also be added:

+   if (decimals > TARGET_DECIMALS) revert UnsupportedFeedDecimals();

Description

The functions _getCurrentChainlinkResponse() and _getRoundChainlinkResponse() leave the value of response.success in their default value, which is false. This values is interpreted by the caller of the function to know the function has failed.

However this behaviour might not be obvious for readers of the code.

Recommendation

Consider adding a comment:

-catch { ... }+catch { ... } // leave response.success == false

Description

The checks in _checkDeviation() and _isValidDeviationReference() are very similar. Because the checks are not trivial, having it in two places makes maintenance of the code more difficult.

Recommendation

Consider combining the checks. The check for _chainlinkIsFrozen() could also be added.

Coinshift

Acknowledged. It is doing different error handling:

• _validateAndNormalize() reverts on bad data
• _isValidDeviationReference() returns false (skip)

Combining them into one method with a revert-or-skip flag reduces clarity without improving safety.

Description

_disableInitializers() call in the constructor can be removed as the same code exists inside the constructor of AccessControlledUUPSUpgradeable parent contract.

Recommendation

Remove redundant functionality.

Description

The check for decimals == TARGET_DECIMALS is very unlikely to trigger in practice because decimals usually are 8 or 18, which is smaller than TARGET_DECIMALS.

Without this statement, the code will still function correctly and some gas will be saved.

Recommendation

Consider removing the statement:

- if (decimals == TARGET_DECIMALS) return rawPrice;

Coinshift

Acknowledged. We will keep the check, as removing the defensive guard doesn't save significant gas.

Description

In the for loop, the value of _priceIds.length is calculated for every cycle. This can be optimized.

Recommendation

Consider caching the value of _priceIds.length.

Description

There are a few checks that prevent overflows during multiplications. Solidity will also catch these types of errors so the checks aren't really necessary. Removing them will save some gas.

Recommendation

Consider removing the checks.

Coinshift

Acknowledged. Custom errors are intentionally used because they are clearer than a raw panic for debugging and monitoring.

Description

Function _checkPriceBounds() every time checks minPrice == 0 || maxPrice < minPrice. This checks can be done one time during configuration which will save same gas.

Recommendation

Consider moving this check to setBoundsCheckEnabled().

Note: the check is already present in setMaxPriceDeviationBps()

function _checkPriceBounds(...) ... {    ...-   if (minPrice == 0 || maxPrice < minPrice) revert InvalidPriceBounds();    ...}function setBoundsCheckEnabled(...) ... {+   if (minPrice == 0 || maxPrice < minPrice) revert InvalidPriceBounds(); // have to be set correctly first    ...}

Coinshift

Acknowledged. It protects against the case where bounds are set and then are somehow corrupted then enabled. Gas costs are negligible in this case.