juancito - `StableOracleDAI` calculates `getPriceUSD` with inverted base/rate tokens for Chainlink price
sherlock-admin opened this issue · 7 comments
juancito
high
StableOracleDAI
calculates getPriceUSD
with inverted base/rate tokens for Chainlink price
Summary
StableOracleDAI::getPriceUSD()
calculates the average price between the Uniswap pool price for a pair and the Chainlink feed as part of its result.
The problem is that it uses WETH/DAI
as the base/rate tokens for the pool, and DAI/ETH
for the Chainlink feed, which is the opposite.
This will incur in a huge price difference that will impact on the amount of USSD tokens being minted, while requesting the price from this oracle.
Vulnerability Detail
In StableOracleDAI::getPrice()
the price
from the Chainlink feed priceFeedDAIETH
returns the price as DAI/ETH.
This can be checked on Etherscan and the Chainlink Feeds Page.
Also note the comment on the code is misleading, as it is refering to another pair:
chainlink price data is 8 decimals for WETH/USD
/// constructor
24: priceFeedDAIETH = AggregatorV3Interface(
25: 0x773616E4d11A78F511299002da57A0a94577F1f4
26: );
/// getPrice()
46: // chainlink price data is 8 decimals for WETH/USD, so multiply by 10 decimals to get 18 decimal fractional
47: //(uint80 roundID, int256 price, uint256 startedAt, uint256 timeStamp, uint80 answeredInRound) = priceFeedDAIETH.latestRoundData();
48: (, int256 price, , , ) = priceFeedDAIETH.latestRoundData();
On the other hand, the price coming from the Uniswap pool DAIWethPrice
returns the price as WETH/DAI
.
Note that the relation WETH/DAI is given by the orders of the token addresses passed as arguments, being the first the base token, and the second the quote token.
Also note that the variable name DAIWethPrice
is misleading as well as the base/rate are the opposite (although this doesn't affect the code).
uint256 DAIWethPrice = DAIEthOracle.quoteSpecificPoolsWithTimePeriod(
1000000000000000000, // 1 Eth
0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2, // WETH (base token) // @audit
0x6B175474E89094C44Da98b954EedeAC495271d0F, // DAI (quote token) // @audit
pools, // DAI/WETH pool uni v3
600 // period
);
Finally, both values are used to calculate an average price of in ((DAIWethPrice + uint256(price) * 1e10) / 2)
.
But as seen, one has price in DAI/ETH
and the other one in WETH/DAI
, which leads to an incorrect result.
return
(wethPriceUSD * 1e18) /
((DAIWethPrice + uint256(price) * 1e10) / 2);
The average will be lower in this case, and the resulting price higher.
This will be used by USSD::mintForToken()
for calculating the amount of tokens to mint for the user, and thus giving them much more than they should.
Also worth mentioning that USSDRebalancer::rebalance()
also relies on the result of this price calculation and will make it perform trades with incorrect values.
Impact
Users will receive far more USSD tokens than they should when they call mintForToken()
, ruining the token value.
When performed the USSDRebalancer::rebalance()
, all the calculations will be broken for the DAI oracle, leading to incorrect pool trades due to the error in getPrice()
Code Snippet
- https://github.com/sherlock-audit/2023-05-USSD/blob/main/ussd-contracts/contracts/oracles/StableOracleDAI.sol#L46C28-L48
- https://github.com/sherlock-audit/2023-05-USSD/blob/main/ussd-contracts/contracts/oracles/StableOracleDAI.sol#L36-L42
- https://github.com/sherlock-audit/2023-05-USSD/blob/main/ussd-contracts/contracts/oracles/StableOracleDAI.sol#L50C15-L52
Tool used
Manual Review
Recommendation
Calculate the inverse of the price
returned by the Chainlink feed so that it can be averaged with the pool price, making sure that both use the correct WETH/DAI
and ETH/DAI
base/rate tokens.
Escalate for 10 USDC
This is not a duplicate of #909.
It tells about using DAI/ETH instead of ETH/DAI on Chainlink. And #909 tells about completely different issue with oracles
You've created a valid escalation for 10 USDC!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Escalate for 10 USDC
Agree with the previous comment.
This is an independent High impact finding. It is not a duplicate of #909, and hasn't been exposed by other findings selected for report.
It's main point is explained on the Summary:
The problem is that it uses WETH/DAI as the base/rate tokens for the pool, and DAI/ETH for the Chainlink feed, which is the opposite.
A more detailed explanation and recommendation to fix it is included on the rest of the report.
Possible duplicates:
Escalate for 10 USDC
Agree with the previous comment.
This is an independent High impact finding. It is not a duplicate of #909, and hasn't been exposed by other findings selected for report.
It's main point is explained on the Summary:
The problem is that it uses WETH/DAI as the base/rate tokens for the pool, and DAI/ETH for the Chainlink feed, which is the opposite.
A more detailed explanation and recommendation to fix it is included on the rest of the report.
Possible duplicates:
You've created a valid escalation for 10 USDC!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Result:
High
Has duplicates
This is a valid separate issue.
Escalations have been resolved successfully!
Escalation status:
- T1MOH593: accepted
- 0xJuancito: accepted