sherlock-audit / 2023-12-ubiquity-judging

2 stars 2 forks source link

0xadrii - Wrong Chainlink staleness check due to hardcoding price feed staleness threshold when adding a new collateral #130

Closed sherlock-admin2 closed 9 months ago

sherlock-admin2 commented 9 months ago

0xadrii

medium

Wrong Chainlink staleness check due to hardcoding price feed staleness threshold when adding a new collateral

Summary

Hardcoding price feed staleness threshold when adding a new collateral asset in the pool might lead the protocol to receive stale Chainlink prices.

Vulnerability Detail

When adding a new collateral to the Ubiquity pool, certain configurations are performed. One of the configurations consists in setting up a Chainlink price feed for that specific collateral, as well as the price feed staleness threshold (i.e maximum allowed time between Chainlink responses to consider the Chainlink answer stale):

// LibUbiquityPool.sol

function addCollateralToken(
        address collateralAddress,
        address chainLinkPriceFeedAddress,
        uint256 poolCeiling
    ) internal {
        ...

        // set price feed address
        poolStorage.collateralPriceFeedAddresses.push(
            chainLinkPriceFeedAddress
        );

        // set price feed staleness threshold in seconds
        poolStorage.collateralPriceFeedStalenessThresholds.push(1 days); 

The problem is that the price feed staleness is always hardcoded to 1 days for ****ALL collaterals.****

Usually, stablecoin-USD Chainlink price feeds have a heartbeat (a countdown timer that updates the price on-chain when it reaches 00:00) of 3600 seconds (or 1 hour)due to the need of having really accurate prices. Concretetly, the following pricefeeds will be used in Ubiquity:

Expanding the pricefeed details by toggling the “Show more details” button will unveil that both pricefeeds have a heartbeat of 3600s, which is a completely different value to the one set in the addCollateralToken() function by default.

Impact

Medium. Although the price feed configurations might be changed by using the setCollateralChainLinkPriceFeed() function, it looks like the project was not aware of the actual heartbeat times expected for the LUSD and DAI pricefeeds. This would have probably made the Ubiquity team leave the pricefeeds with the default staleness threshold of 1 days, which could have eventually lead the protocol to fetching stale prices from Chainlink

Code Snippet

https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L679

Tool used

Manual Review

Recommendation

Consider passing the staleness threshold as a parameter in the addCollateralToken() function instead of hardcoding the value to 1 days, just like it is done with other important values such as the chainLinkPriceFeedAddress or poolCeiling, and set it to a proper value considering the LUSD and DAI pricefeeds.

sherlock-admin2 commented 9 months ago

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

auditsea commented:

This issue describes about retreiving zero index by default when collateralAddress is unknown, but the function is called by admin so no need to be considered