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

Usage of deprecated ChainLink API in EIP1271Wallet #197

Open
code423n4 opened this issue Feb 2, 2022 · 3 comments
Open

Usage of deprecated ChainLink API in EIP1271Wallet #197

code423n4 opened this issue Feb 2, 2022 · 3 comments
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

Comments

@code423n4
Copy link
Contributor

Handle

cmichel

Vulnerability details

The Chainlink API (latestAnswer) used in the EIP1271Wallet contract is deprecated:

This API is deprecated. Please see API Reference for the latest Price Feed API. Chainlink Docs

This function does not error if no answer has been reached but returns 0. Besides, the latestAnswer is reported with 18 decimals for crypto quotes but 8 decimals for FX quotes (See Chainlink FAQ for more details). A best practice is to get the decimals from the oracles instead of hard-coding them in the contract.

Recommended Mitigation Steps

Use the latestRoundData function to get the price instead. Add checks on the return data with proper revert messages if the price is stale or the round is uncomplete, for example:

(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = priceOracle.latestRoundData();
require(answeredInRound >= roundID, "...");
require(timeStamp != 0, "...");
@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 Feb 2, 2022
code423n4 added a commit that referenced this issue Feb 2, 2022
@jeffywu jeffywu added the duplicate This issue or pull request already exists label Feb 6, 2022
@jeffywu
Copy link
Collaborator

jeffywu commented Feb 6, 2022

Duplicate #178

@pauliax
Copy link
Collaborator

pauliax commented Feb 12, 2022

Valid finding. I am hesitating whether this should be low or medium but decided to leave it as a medium because the likeliness is low but the impact would be huge, and all the wardens submitted this with a medium severity. Also: "Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements."

@pauliax
Copy link
Collaborator

pauliax commented Feb 12, 2022

Making this a primary issue, because it contains the most concise description.

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
Projects
None yet
Development

No branches or pull requests

4 participants