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

Yield Fi yUSD Rate Providers #252

Merged
merged 11 commits into from
Feb 6, 2025
Merged

Yield Fi yUSD Rate Providers #252

merged 11 commits into from
Feb 6, 2025

Conversation

brunoguerios
Copy link
Member

@brunoguerios brunoguerios commented Jan 30, 2025

Close #225

@brunoguerios brunoguerios requested a review from mkflow27 January 30, 2025 16:58
@brunoguerios brunoguerios self-assigned this Jan 30, 2025
@brunoguerios brunoguerios changed the title Yield Fi yUSD Rate Provider Yield Fi yUSD Rate Providers Jan 30, 2025
@MattPereira
Copy link
Member

MattPereira commented Jan 30, 2025

@brunoguerios I think we are supposed to also add info to registry for each new rate provider.

https://github.com/balancer/code-review/pull/247/files#diff-3b890de0e38bb388421bc68f5e6a0174f3cd1d5de6a060590b9314a8cf039e63

Maybe we could eventually figure out how to rip the info from the .md review files and use a GHA to automate the registry.json on merge of PR, but challenge is existing reviews don't seem to adhere to strict pattern so parsing will be tough 🤔

- [Protocol Audits](https://docs.yield.fi/resources/audits)

## Context
This RateProvider is updated via the API3 product. It reports the exchangeRate of yUSD/USD on mainnet. It is read on mainnet and available as an Oracle on arbitrum.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must say I'm not able to fully grasp this API3 data source, but I'm assuming it's a standard and I replicated the findings registered on the ezETH rate provider review.
Can you guys please confirm this is indeed the case?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Bruno. Gerally speaking whenever we first took a look at a new oracle rate provider, they were reviewed as part of the review. After that if the rate is a "direct" result of a oracle price provider, we have marked it as usable. This includes oracle such as:

  • API3
  • Chainlink
  • Redstone
  • Chronicle

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cases like this, you can refer to the review already being done at review

- admin address: [arbitrum:0x2AAE699ed04BBbD068f67a5b3C813eBB35f2c9E8](https://arbiscan.io/address/0x2AAE699ed04BBbD068f67a5b3C813eBB35f2c9E8)
- admin type: multisig
- multisig threshold/signers: 4/8
- multisig timelock? NO
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find timelocks in the implementation, but if it's too relevant, it would be nice if you could double check.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not every system has them. They are helpful for the reader to have but not a requirement.

- admin type: multisig
- multisig threshold/signers: 4/8
- multisig timelock? NO
- trustworthy signers? NO
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we go through signers to figure this out? Is there a registry of some sort?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signers section of the review can be removed (I will also remove it from the template)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just removed it from the current review and from the template ✅

- [x] Price data is provided by an off-chain source (e.g., a Chainlink oracle, a multisig, or a network of nodes).
- source: API3.
- source address: The data is sourced from multiple so called "beacons" which are a set of nodes which provide the rate data to be aggregated by the `Api3ServerV1` [base:0x709944a48cAf83535e43471680fDA4905FB3920a](https://arbiscan.io/address/0x709944a48cAf83535e43471680fDA4905FB3920a)
- any protections? The rate is calculated as the median of all the rate data "fetched" from beacons. For more information about more protections see [api3 docs](https://docs.api3.org/reference/dapis/understand/deviations.html) and consult the `updateBeaconSetWithBeacons` & `updateOevProxyDataFeedWithSignedData` functions.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about not fully grasping API3 and replicating ezETH findings. Please confirm 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine to leave it like this.

Comment on lines 51 to 53
**Summary judgment: SAFE**

This rate provider should work well with Balancer pools. API3 updates the rate on arbitrum regularly and has various protections in place to ensure the correct values are bridged accurately.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it weird that I just flagged the mainnet rate provider as unsafe and these ones derived from that as safe 😂
I mean, I couldn't find a direct link between them, but I'm assuming L2s rates are derived from L1 rates through API3.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasonable to say. Let's wait for them to upgrade to multisigs. We need to take into account that our reviews are time-boxed. So we cannot investigate all the fine details as this is not an audit. It might be worth to share your experiences on proper time-boxing with John & Daniel as well.

"0xa65ffBa1CD05414df3fD24Bf5dc319B47450Fbf4": {
"asset": "0x1CE7D9942ff78c328A4181b9F3826fEE6D845A97",
"name": "YieldFi yUSD Rate Provider",
"summary": "unsafe",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait for them to upgrade to multisigs, then we can merge this pr. Blocked until then I suggest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note, should we start using usable/unusable in the registry as well?
If yes, maybe we should have a separate PR to make that a standard through all existing reviews and registry entries.

@brunoguerios brunoguerios requested a review from mkflow27 February 6, 2025 19:00
Copy link
Collaborator

@mkflow27 mkflow27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mkflow27 mkflow27 merged commit 2a3e3e7 into main Feb 6, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Yield Fi yUSD Rate Providers
3 participants