search

code-423n4 / 2021-09-wildcredit-findings

0 stars 0 forks source link

Incorrect implementation of chainlink oracle #117

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

tensors

Vulnerability details

The protocol doesn't implement the chainlink ETH oracle correctly.

Many user functions in LendingPair.sol use _currentTokenValues() which computes data based off of the chainlink eth oracle (via tokenPrice() which uses EthPrice()).

In a correct implementation using the oracle, a user would call such a function, a request from the WildCredit protocol would be sent to a chainlink node for data, while an unfinished call is pushed into some processing stack awaiting the response from the chainlink node. The node would fulfill the request within a few blocks, and call a fulfillRequest() method that should be implemented in the WildCredit protocol which includes up to date price data. This fulfillRequest() should use the data provided from the node to correctly determine prices and update the state of the protocol.

An example of a proper chainlink integration:

https://polygonscan.com/address/0xFB694dbAA0f78A8b07bE4A1a92AE12D2845138fB#code https://polygonscan.com/address/0xB28D7CeA60DB2da4d3Afd19fA8B8f14a7487745e#code

Notice how every function using the oracle has a callback.

Compare this to what the current protocol is doing: A user calls a function that uses oracle data. The protocol assumes (or just hopes) that the oracle data is fresh enough, and uses data from it to execute the function.

This assumption is a very weak one as you are relying on other protocols to update to chainlink oracle for you. Experimentally, I've found that the ETH price oracle without manual updates lags about 15min-1h. This is plenty of time for large arbitrage oppurtunities to appear, losing funds from the protocol.

Recommendations: Either, rewrite parts of the code using a proper integration or use a uniswap oracle for ETH price.

talegift commented 2 years ago

I disagree that this is how Chainlink price feeds are meant to be used. Our implementation is correct and it's how many other protocols are using it as well. Most feeds have sponsors paying for the oracle freshness.

We'll fix the possibility of an outdated feed by removing dependence on Chainlink completely.

But if we didn't, a better solution would be to check the freshness of the price feed as described in #116

I suggest to either lower severity to 0-1 or marking as invalid as this report is recommending to adopt a non-standard way of using Chainlink price feeds.

ghoul-sol commented 2 years ago

Proposed solution by the warden is valid however impractical for price discovery. Lots of protocols are using chainlink in red-only format because the price is needed immediately and each chainlink request is very expensive. The risk is well known. Making invalid.