Closed code423n4 closed 2 years ago
Agree this is an issue, but disagree with the severity as there are no oracles used in the core flow that could allow for value to be removed from the protocol. Within the bridging flow, the only time oracles are used is within the SponsorVault
, and they are only used to provide additive value to the user.
AFAIK, the only instance where an oracle is used in SponsorVault
is in getRate()
. I don't think the issue raised actually affects the behaviour of this function unless gasTokenOracle
builds upon ConnextPriceOracle
. Tagging in @LayneHaber for more context.
Downgrading to QA
because ConnextPriceOracle.sol
is not currently used within the codebase but may be integrated again in the future.
Merging with #203.
Lines of code
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L158-L161
Vulnerability details
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L81-L97
setDirectPrice()
can be called by the admin to setassetPrices
directly, once set, it will become the primary source ingetTokenPrice()
.However, there is no timestamp assigned alongside with the price when
setDirectPrice()
, as a result, the price set by the admin can and tend to be stale.Furthermore, without a timestamp in the calldata, when the network is congested, transactions sent a while ago with stale prices can be accepted as new/fresh prices.
Recommendation
tokenPrice
should record not only the price but also the last updated time.setDirectPrice
should add a new parameter:_timestamp
:getTokenPrice()
should check for the freshness of directly set token price: