From e339fadecbb2f7283155f5fbfd0aaa234156b6e3 Mon Sep 17 00:00:00 2001 From: mkflow27 Date: Wed, 18 Sep 2024 08:22:15 +0200 Subject: [PATCH 1/3] balETH ERC4626 Rate Provider Fixes #148 --- rate-providers/TokemakRateProvider.md | 51 +++++++++++++++++++++++++++ rate-providers/registry.json | 10 ++++++ 2 files changed, 61 insertions(+) create mode 100644 rate-providers/TokemakRateProvider.md diff --git a/rate-providers/TokemakRateProvider.md b/rate-providers/TokemakRateProvider.md new file mode 100644 index 0000000..dc0235e --- /dev/null +++ b/rate-providers/TokemakRateProvider.md @@ -0,0 +1,51 @@ +# Rate Provider: `ERC4626RateProvider` + +## Details +- Reviewed by: @mkflow27 +- Checked by: @\ +- Deployed at: + - [ethereum:0xd4580a56e715F14Ed9d340Ff30147d66230d44Ba](https://etherscan.io/address/0xd4580a56e715F14Ed9d340Ff30147d66230d44Ba#readContract) +- Audit report(s): + - [Tokemak audits](https://docs.tokemak.xyz/developer-docs/security-and-audits) + +## Context +An Autopool, which use the highly composable ERC-4626 standard and can be configured with a set of destinations (pools and DEXs) to which assets may be deployed to. In this particular rate provider the Autopool's assets are distributed accross the Balancer ecosystem where the base asset is Ether. For more information see also the [tokemak app](https://app.tokemak.xyz/autopool?id=0x6dC3ce9C57b20131347FDc9089D740DAf6eB34c5). +Assets deposited into an Autopool are not subject to any lock ups or cooldown periods, meaning that users can withdraw their funds at any time. + +## Review Checklist: Bare Minimum Compatibility +Each of the items below represents an absolute requirement for the Rate Provider. If any of these is unchecked, the Rate Provider is unfit to use. + +- [x] Implements the [`IRateProvider`](https://github.com/balancer/balancer-v2-monorepo/blob/bc3b3fee6e13e01d2efe610ed8118fdb74dfc1f2/pkg/interfaces/contracts/pool-utils/IRateProvider.sol) interface. +- [x] `getRate` returns an 18-decimal fixed point number (i.e., 1 == 1e18) regardless of underlying token decimals. + +## Review Checklist: Common Findings +Each of the items below represents a common red flag found in Rate Provider contracts. + +If none of these is checked, then this might be a pretty great Rate Provider! If any of these is checked, we must thoroughly elaborate on the conditions that lead to the potential issue. Decision points are not binary; a Rate Provider can be safe despite these boxes being checked. A check simply indicates that thorough vetting is required in a specific area, and this vetting should be used to inform a holistic analysis of the Rate Provider. + +### Administrative Privileges +- [ ] The Rate Provider is upgradeable (e.g., via a proxy architecture or an `onlyOwner` function that updates the price source address). + +- [ ] Some other portion of the price pipeline is upgradeable (e.g., the token itself, an oracle, or some piece of a larger system that tracks the price). + +### Oracles +- [ ] Price data is provided by an off-chain source (e.g., a Chainlink oracle, a multisig, or a network of nodes). + +- [ ] Price data is expected to be volatile (e.g., because it represents an open market price instead of a (mostly) monotonically increasing price). + +### Common Manipulation Vectors +- [ ] The Rate Provider is susceptible to donation attacks. + +## Additional Findings +To save time, we do not bother pointing out low-severity/informational issues or gas optimizations (unless the gas usage is particularly egregious). Instead, we focus only on high- and medium-severity findings which materially impact the contract's functionality and could harm users. + +## Conclusion +**Summary judgment: SAFE** + +This rate provider should work with Balancer pools. However due to the time-boxed nature of this review and the high complexity of the underlying Tokemak Autopool product, this review could not cover the total path of how the rate is computed. The approach used to rate calculation is the common totalAssets / totalSupply approach usually used by yield-bearing vault type products. One thing to mention is that for a user, there are essentially two ways to exit an Autopool. Either, by withdrawing the ERC4626 vault's `asset` or selling balETH into the the pool where this rate provider is used. Depending on withdraw size, the rate the pool uses can differ between the rate used on `withdraw` (hint:slippage) from the Vault. For more information also see the developer comments in `AutopoolDebt.sol:withdraw()`. + +Additionally During initial Autopool deployment rates are expected to be more dynamic. For more context see the developers comments +> Right now the the balETH Autopool has done two rebalances and there is value loss associated with that... slippage and fee's swapping from WETH to wstETH/ETHx/rsETH. Since its so early in the deployment, it also hasn't performed any reward claiming and auto-compounds so it hasn't had a chance to make up lost value yet. We'd expect this on every Autopool for the first few days after the first rebalances. + + +The suggestions is to revisit this rate provider review once the pool has gained traction and conduct a more thorough review of the underlying Tokemak system. \ No newline at end of file diff --git a/rate-providers/registry.json b/rate-providers/registry.json index d963a02..ca9e545 100644 --- a/rate-providers/registry.json +++ b/rate-providers/registry.json @@ -1399,6 +1399,15 @@ "implementationReviewed": "0xd7097af27b14e204564c057c636022fae346fe60" } ] + }, + "0xd4580a56e715F14Ed9d340Ff30147d66230d44Ba": { + "asset": "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2", + "name": "ERC4626RateProvider", + "summary": "safe", + "review": "./TokemakRateProvider.md", + "warnings": [], + "factory": "", + "upgradeableComponents": [] } }, "fantom": { @@ -1561,6 +1570,7 @@ "factory": "", "upgradeableComponents": [] }, + "0x5F62fd24941B585b91EB059E0ea1a7e729357511": { "asset": "0xf0E7eC247b918311afa054E0AEdb99d74c31b809", "name": "ERC4626RateProvider", From 0125aac580848c93240487a31bb0c7a0a36d7329 Mon Sep 17 00:00:00 2001 From: mkflow27 Date: Wed, 18 Sep 2024 13:15:44 +0200 Subject: [PATCH 2/3] add note on upgradeability --- rate-providers/TokemakRateProvider.md | 1 + 1 file changed, 1 insertion(+) diff --git a/rate-providers/TokemakRateProvider.md b/rate-providers/TokemakRateProvider.md index dc0235e..8f74eb7 100644 --- a/rate-providers/TokemakRateProvider.md +++ b/rate-providers/TokemakRateProvider.md @@ -25,6 +25,7 @@ If none of these is checked, then this might be a pretty great Rate Provider! If ### Administrative Privileges - [ ] The Rate Provider is upgradeable (e.g., via a proxy architecture or an `onlyOwner` function that updates the price source address). + - comment: The system uses a minimal proxy architecture, which simply forwards all calls. While the vault is not upgradeable by the usual upgradeable-proxy pattern it needs to be noted that additional downstream components may be upgradeable but were not part of the first review. - [ ] Some other portion of the price pipeline is upgradeable (e.g., the token itself, an oracle, or some piece of a larger system that tracks the price). From ba1ef881037bc97f626c638b75cfef9be4a3ebbf Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 18 Sep 2024 18:32:46 +0200 Subject: [PATCH 3/3] Add checked by --- rate-providers/TokemakRateProvider.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rate-providers/TokemakRateProvider.md b/rate-providers/TokemakRateProvider.md index 8f74eb7..4b93689 100644 --- a/rate-providers/TokemakRateProvider.md +++ b/rate-providers/TokemakRateProvider.md @@ -2,7 +2,7 @@ ## Details - Reviewed by: @mkflow27 -- Checked by: @\ +- Checked by: @danielmkm - Deployed at: - [ethereum:0xd4580a56e715F14Ed9d340Ff30147d66230d44Ba](https://etherscan.io/address/0xd4580a56e715F14Ed9d340Ff30147d66230d44Ba#readContract) - Audit report(s):