Closed c4-bot-7 closed 3 months ago
raymondfam marked the issue as insufficient quality report
raymondfam marked the issue as duplicate of #4
See #8.
hansfriese marked the issue as unsatisfactory: Out of scope
Hi @hansfriese ,
Appeal
I believe that a mistake has been made in ruling this issue Out of Scope. May you please review below and advise if I am missing something?
Per lookout's comment in 8 which references the Readme :
Common known issue per readme: Oracle is very dependent on Chainlink, stale/invalid prices fallback to a Uniswap TWAP. 2 hours staleness means it can be somewhat out of date.
However none of the three separate vulnerabilities mentioned in this report could be considered to be covered by that statement and hence Out of Scope. Note: There is also a single Oracle issue in the 4naly3er report which also doesn't relate to the three issues in the report
ETH / USD
pricefeed because the ETH / USD
pricefeed does in fact utilise a 2 hour staleness check.
However the 'other' feeds have neither a 2 hour staleness check nor any other staleness check whatsoever.If that part of the statement is the basis for it's exclusion I argue that it is an incorrect exclusion.
timestamp
values in the try
and catch
blocks for 'other' pricefeeds. In the try
block there is a check timestamp == 0
, see here, to ensure tmestamp is not 0
.
In the catch
block this check is missing, see here, raising the possibility that a 0
timestamp value rejected in the try
block would be accepted in the catch
block.Known Issues does not appear to make this finding Out of Scope.
try
and catch
blocks for 'other' pricefeeds. In the try
block there is a check chainlinkPrice <= 0
to ensure returned price is not negative or 0
, see here.
In the catch
block this check is different price == 0
, see here, meaning that a negative value rejected in the try
block could be accepted in the catch
block.Known Issues does not appear to make this finding Out of Scope.
hansfriese marked the issue as not a duplicate
hansfriese changed the severity to QA (Quality Assurance)
hansfriese marked the issue as grade-c
@hansfriese what about the lack of a staleness check for other pricefeeds? 252 which only mentions staleness has just been Sponsor Confirmed?
The First vulnerability describes the total lack of a heartbeat / staleness check for 'other' pricefeeds. Where the Readme states "2 hours staleness means it can be somewhat out of date" it is referring to the ETH / USD pricefeed because the ETH / USD pricefeed does in fact utilise a 2 hour staleness check. However the 'other' feeds have neither a 2 hour staleness check nor any other staleness check whatsoever.
Check #164 for details.
hansfriese removed the grade
This previously downgraded issue has been upgraded by hansfriese
hansfriese marked the issue as satisfactory
hansfriese marked the issue as duplicate of #164
hansfriese marked the issue as partial-50
Lines of code
https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L117-L129 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L19-L67
Vulnerability details
Summary
Inconsistent price checks in
getOraclePrice()
will lead to wrong prices being used with detrimental effects to stability of protocol .Vulnerability Details
In
getOraclePrice()
, the returned price value for "other" oracles (e.g. JPY, XAU) is checked inconsistently.Firstly, it is mentioned in the
Known Issues
regarding heartbeats:However, both the
try
block and thecatch
block are missing even any heartbeat check as it is stated in the docs that there should be here. Thetry
block does price checks inOracleCircuitBreaker()
as below:Secondly, there is a missing check in the
catch
block that the timestamp is not equal to0
, which is present inoracleCircuitBreaker()
i.e.timeStamp == 0
.Thirdly, there is an inconsistent use of relational operators, where in the
try
block there is a check for negative prices but in thecatch
block the is just a check that the value is not equal to0
.In
oracleCircuitBreaker()
we see the checkchainlinkPrice <= 0
reverts if there is a negative price returned which, given that Chainlink's prices areint
values, is relevant. However in thecatch
block, as below, we see the checkprice == 0
.Impact
Regarding the heartbeat and timestamp issues, these are esstential missing checks which protect the protocol from using stale prices.
Regarding the inconsistent application of relational operators, this means that if there is a failure in the
try
block because a negative price reverted there, that same incorrect price will be accepted and used in thecatch
block.Both of these issues will cause huge issues to the correct functioning of the protocol via incorrect prices being fetched.
Tools Used
Manual Review Foundry Testing
Recommendations
Given that different feeds have different heartbeats, create an Oracle data struct whereby their specific infomation can be stored and fetched to be used in the
oracleCircuitBreaker()
and thecatch
block.Update the price check and add the timestamp check to the
catch
block ingetOraclePrice()
as:Assessed type
Oracle