Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Oracle data feed is insufficiently validated. #136

Open
code423n4 opened this issue Jan 30, 2022 · 1 comment
Open

Oracle data feed is insufficiently validated. #136

code423n4 opened this issue Jan 30, 2022 · 1 comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Handle

throttle

Vulnerability details

Impact

Price can be stale and can lead to wrong quoteAmount return value

Proof of Concept

Oracle data feed is insufficiently validated. There is no check for stale price and round completeness.
Price can be stale and can lead to wrong quoteAmount return value

function _peek(
    bytes6 base,
    bytes6 quote,
    uint256 baseAmount
) private view returns (uint256 quoteAmount, uint256 updateTime) {
    ...

    (, int256 daiPrice, , , ) = DAI.latestRoundData();
    (, int256 usdcPrice, , , ) = USDC.latestRoundData();
    (, int256 usdtPrice, , , ) = USDT.latestRoundData();

    require(
        daiPrice > 0 && usdcPrice > 0 && usdtPrice > 0,
        "Chainlink pricefeed reporting 0"
    );

    ...
}

Tools Used

Manual review

Recommended Mitigation Steps

Validate data feed

function _peek(
    bytes6 base,
    bytes6 quote,
    uint256 baseAmount
) private view returns (uint256 quoteAmount, uint256 updateTime) {
    ...
    (uint80 roundID, int256 daiPrice, , uint256 timestamp, uint80 answeredInRound) = DAI.latestRoundData();
    require(daiPrice > 0, "ChainLink: DAI price <= 0");
    require(answeredInRound >= roundID, "ChainLink: Stale price");
    require(timestamp > 0, "ChainLink: Round not complete");

    (roundID, int256 usdcPrice, , timestamp, answeredInRound) = USDC.latestRoundData();
    require(usdcPrice > 0, "ChainLink: USDC price <= 0");
    require(answeredInRound >= roundID, "ChainLink: Stale USDC price");
    require(timestamp > 0, "ChainLink: USDC round not complete");

    (roundID, int256 usdtPrice, , timestamp, answeredInRound) = USDT.latestRoundData();
    require(usdtPrice > 0, "ChainLink: USDT price <= 0");
    require(answeredInRound >= roundID, "ChainLink: Stale USDT price");
    require(timestamp > 0, "ChainLink: USDT round not complete");

    ...
}
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jan 30, 2022
code423n4 added a commit that referenced this issue Jan 30, 2022
@devtooligan devtooligan added duplicate This issue or pull request already exists and removed duplicate This issue or pull request already exists labels Jan 31, 2022
@iamsahu iamsahu added the duplicate This issue or pull request already exists label Feb 1, 2022
@code-423n4 code-423n4 deleted a comment from devtooligan Feb 1, 2022
@iamsahu iamsahu removed the duplicate This issue or pull request already exists label Feb 1, 2022
@iamsahu iamsahu added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 1, 2022
@iamsahu iamsahu added the resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) label Feb 8, 2022
@GalloDaSballo
Copy link
Collaborator

When using Chainlink Price feeds it is important to ensure the price feed data was updated recently.
While getting started with chainlink requires just one line of code, it is best to add additional checks for in production environments.

I believe the finding to be valid and Medium severity to be appropriate.

The sponsor has mitigated in a subsequent PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

4 participants