From 10477b75a8e98419e6828f811c7188fd75b83286 Mon Sep 17 00:00:00 2001 From: mkflow27 <35266877+mkflow27@users.noreply.github.com> Date: Wed, 5 Jun 2024 11:07:14 +0200 Subject: [PATCH 1/5] rETH/ETH rate provider Fixes #73 --- rate-providers/rETHRateProviderGnosis.md | 64 ++++++++++++++++++++++++ rate-providers/registry.json | 9 ++++ 2 files changed, 73 insertions(+) create mode 100644 rate-providers/rETHRateProviderGnosis.md diff --git a/rate-providers/rETHRateProviderGnosis.md b/rate-providers/rETHRateProviderGnosis.md new file mode 100644 index 0000000..0c38e30 --- /dev/null +++ b/rate-providers/rETHRateProviderGnosis.md @@ -0,0 +1,64 @@ +# Rate Provider: `TollgateChronicleRateProvider` + +## Details +- Reviewed by: @mkflow27 +- Checked by: @\ +- Deployed at: + - [gnosis:0xdc90e2680094314CEaB45CE15100F6e02cEB7ceD](https://gnosisscan.io/address/0xdc90e2680094314ceab45ce15100f6e02ceb7ced#code) +- Audit report(s): + - [Chronicle Oracles audits](\) + +## Context +This rate Provider bridges the eth/reth exchange rate from Mainnet to gnosis chain. This is done via an oracle solution developed by chronicle. + +## 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 +- [x] Price data is provided by an off-chain source (e.g., a Chainlink oracle, a multisig, or a network of nodes). + - source: Chronicle protocol Oracle + - source address: [gnosis:0xE04a8f725b49c9D36C0fD3495F4a792056374847](https://gnosisscan.io/address/0xe04a8f725b49c9d36c0fd3495f4a792056374847) + - any protections? YES + - The rate data's supplied age must be greater than the timestamp of last successful update + - the rate data's age must not be greater than current time + - The rate data's integrity is verified by the supplied signature. Currently `bar` (7) signers verify the rate's integrity. For more information see `_poke` and `isAcceptableSchnorrSignatureNow` as part of the PriceFeed `Chronicle_RETH_ETH_1` contract deployed at [gnosis:0x7706A143c750aDfc2196c4Bf84e6BB012Aed1182](https://gnosisscan.io/address/0x7706a143c750adfc2196c4bf84e6bb012aed1182#code) + +- [ ] 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. + +### M-01: `owner` is an EOA and can make Rate Provider revert. +The current `owner` of the RateProvider can call `enableTollgate` which would in its current form make the RateProvider revert. +```solidity +function getRate() public view override returns (uint256) { +if (tollgateEnabled && !tolled(msg.sender)) { + revert NotTolled(msg.sender); +} + +return super.getRate(); +} +``` +Currently `tollgateEnabled` is false and the msg.sender (the pool) is not tolled, skipping the if condition. However if the tollgate was enabled, the rateProvider would revert. Currently the `owner` has this capability. + +Suggestion: The `owner` should call `transferOwnership` with the `newOwner` being a multisig. + +## Conclusion +**Summary judgment: \** + diff --git a/rate-providers/registry.json b/rate-providers/registry.json index 58284ef..793e937 100644 --- a/rate-providers/registry.json +++ b/rate-providers/registry.json @@ -1092,6 +1092,15 @@ "implementationReviewed": "0x5b522140fabeB6b6232336295581e63902e9b4ad" } ] + }, + "0xdc90e2680094314CEaB45CE15100F6e02cEB7ceD": { + "asset": "", + "name": "TollgateChronicleRateProvider", + "summary": "", + "review": "./rETHRateProviderGnosis.md", + "warnings": [], + "factory": "", + "upgradeableComponents": [] } }, "optimism": { From 356e2ac89a8f325fe35598adfd603323530a9dbb Mon Sep 17 00:00:00 2001 From: mkflow27 Date: Wed, 5 Jun 2024 16:05:25 +0200 Subject: [PATCH 2/5] review: add registry --- rate-providers/registry.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rate-providers/registry.json b/rate-providers/registry.json index 793e937..e1c59c2 100644 --- a/rate-providers/registry.json +++ b/rate-providers/registry.json @@ -1094,7 +1094,7 @@ ] }, "0xdc90e2680094314CEaB45CE15100F6e02cEB7ceD": { - "asset": "", + "asset": "0xc791240d1f2def5938e2031364ff4ed887133c3d", "name": "TollgateChronicleRateProvider", "summary": "", "review": "./rETHRateProviderGnosis.md", From e7c08578a0d1a0315805d3d27fdadad69a70c333 Mon Sep 17 00:00:00 2001 From: mkflow27 Date: Mon, 10 Jun 2024 17:08:43 +0200 Subject: [PATCH 3/5] review: add GBP & registry update --- ...is.md => TollgateChronicleRateProvider.md} | 40 +++++++++---------- rate-providers/registry.json | 11 ++++- 2 files changed, 28 insertions(+), 23 deletions(-) rename rate-providers/{rETHRateProviderGnosis.md => TollgateChronicleRateProvider.md} (51%) diff --git a/rate-providers/rETHRateProviderGnosis.md b/rate-providers/TollgateChronicleRateProvider.md similarity index 51% rename from rate-providers/rETHRateProviderGnosis.md rename to rate-providers/TollgateChronicleRateProvider.md index 0c38e30..b84bac7 100644 --- a/rate-providers/rETHRateProviderGnosis.md +++ b/rate-providers/TollgateChronicleRateProvider.md @@ -5,11 +5,12 @@ - Checked by: @\ - Deployed at: - [gnosis:0xdc90e2680094314CEaB45CE15100F6e02cEB7ceD](https://gnosisscan.io/address/0xdc90e2680094314ceab45ce15100f6e02ceb7ced#code) + - [gnosis:0x92320D3C8Fd6BE59b22eB0eEe330901Fe4617f33](https://gnosisscan.io/address/0x92320D3C8Fd6BE59b22eB0eEe330901Fe4617f33#code) - Audit report(s): - [Chronicle Oracles audits](\) ## Context -This rate Provider bridges the eth/reth exchange rate from Mainnet to gnosis chain. This is done via an oracle solution developed by chronicle. +This rate Provider bridges the eth/reth exchange rate & GBP/USD to gnosis chain. This is done via an oracle solution developed by chronicle. ## 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. @@ -29,12 +30,20 @@ If none of these is checked, then this might be a pretty great Rate Provider! If ### Oracles - [x] Price data is provided by an off-chain source (e.g., a Chainlink oracle, a multisig, or a network of nodes). - - source: Chronicle protocol Oracle - - source address: [gnosis:0xE04a8f725b49c9D36C0fD3495F4a792056374847](https://gnosisscan.io/address/0xe04a8f725b49c9d36c0fd3495f4a792056374847) - - any protections? YES - - The rate data's supplied age must be greater than the timestamp of last successful update - - the rate data's age must not be greater than current time - - The rate data's integrity is verified by the supplied signature. Currently `bar` (7) signers verify the rate's integrity. For more information see `_poke` and `isAcceptableSchnorrSignatureNow` as part of the PriceFeed `Chronicle_RETH_ETH_1` contract deployed at [gnosis:0x7706A143c750aDfc2196c4Bf84e6BB012Aed1182](https://gnosisscan.io/address/0x7706a143c750adfc2196c4bf84e6bb012aed1182#code) + - reth/eth: + - source: Chronicle protocol Oracle + - source address: [gnosis:0xE04a8f725b49c9D36C0fD3495F4a792056374847](https://gnosisscan.io/address/0xe04a8f725b49c9d36c0fd3495f4a792056374847) + - any protections? YES + - The rate data's supplied age must be greater than the timestamp of last successful update + - the rate data's age must not be greater than current time + - The rate data's integrity is verified by the supplied signature. Currently `bar` (7) signers verify the rate's integrity. For more information see `_poke` and `isAcceptableSchnorrSignatureNow` as part of the PriceFeed `Chronicle_RETH_ETH_1` contract deployed at [gnosis:0x7706A143c750aDfc2196c4Bf84e6BB012Aed1182](https://gnosisscan.io/address/0x7706a143c750adfc2196c4bf84e6bb012aed1182#code) + - GBP/USD: + - source: Chronicle protocol Oracle + - source address: [gnosis:0x0E418d54863a3fAfeC9e96a358795f0f236f5f66](https://gnosisscan.io/address/0x0E418d54863a3fAfeC9e96a358795f0f236f5f66) + - any protections? YES + - The rate data's supplied age must be greater than the timestamp of last successful update + - the rate data's age must not be greater than current time + - The rate data's integrity is verified by the supplied signature. Currently `bar` (7) signers verify the rate's integrity. For more information see `_poke` and `isAcceptableSchnorrSignatureNow` as part of the PriceFeed `Chronicle_GBP_USD_1` contract deployed at [gnosis:0x0E418d54863a3fAfeC9e96a358795f0f236f5f66](https://gnosisscan.io/address/0x0E418d54863a3fAfeC9e96a358795f0f236f5f66#code) - [ ] Price data is expected to be volatile (e.g., because it represents an open market price instead of a (mostly) monotonically increasing price). @@ -44,21 +53,8 @@ If none of these is checked, then this might be a pretty great Rate Provider! If ## 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. -### M-01: `owner` is an EOA and can make Rate Provider revert. -The current `owner` of the RateProvider can call `enableTollgate` which would in its current form make the RateProvider revert. -```solidity -function getRate() public view override returns (uint256) { -if (tollgateEnabled && !tolled(msg.sender)) { - revert NotTolled(msg.sender); -} - -return super.getRate(); -} -``` -Currently `tollgateEnabled` is false and the msg.sender (the pool) is not tolled, skipping the if condition. However if the tollgate was enabled, the rateProvider would revert. Currently the `owner` has this capability. - -Suggestion: The `owner` should call `transferOwnership` with the `newOwner` being a multisig. ## Conclusion -**Summary judgment: \** +**Summary judgment: ** +This rate provider should work well with Balancer pools. The oracle providing the rate data has various guardrails in place ensuring the integrity of the rate being provided. The `owner` of the rate provider has the capability to revert the call to `getRate`. However this potential revert scenario is guarded behind a Multisig of [1/4] for reth and [2/6] for GBP. diff --git a/rate-providers/registry.json b/rate-providers/registry.json index e1c59c2..9748229 100644 --- a/rate-providers/registry.json +++ b/rate-providers/registry.json @@ -1097,7 +1097,16 @@ "asset": "0xc791240d1f2def5938e2031364ff4ed887133c3d", "name": "TollgateChronicleRateProvider", "summary": "", - "review": "./rETHRateProviderGnosis.md", + "review": "./TollgateChronicleRateProvider.md", + "warnings": [], + "factory": "", + "upgradeableComponents": [] + }, + "0x92320D3C8Fd6BE59b22eB0eEe330901Fe4617f33": { + "asset": "", + "name": "TollgateChronicleRateProvider", + "summary": "", + "review": "./TollgateChronicleRateProvider.md", "warnings": [], "factory": "", "upgradeableComponents": [] From 778b1ac6d65860361202af3b44375b087f1aceb3 Mon Sep 17 00:00:00 2001 From: mkflow27 Date: Wed, 12 Jun 2024 10:29:32 +0200 Subject: [PATCH 4/5] review: add asset & audit --- rate-providers/TollgateChronicleRateProvider.md | 6 +++--- rate-providers/registry.json | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/rate-providers/TollgateChronicleRateProvider.md b/rate-providers/TollgateChronicleRateProvider.md index b84bac7..62f02ec 100644 --- a/rate-providers/TollgateChronicleRateProvider.md +++ b/rate-providers/TollgateChronicleRateProvider.md @@ -7,7 +7,7 @@ - [gnosis:0xdc90e2680094314CEaB45CE15100F6e02cEB7ceD](https://gnosisscan.io/address/0xdc90e2680094314ceab45ce15100f6e02ceb7ced#code) - [gnosis:0x92320D3C8Fd6BE59b22eB0eEe330901Fe4617f33](https://gnosisscan.io/address/0x92320D3C8Fd6BE59b22eB0eEe330901Fe4617f33#code) - Audit report(s): - - [Chronicle Oracles audits](\) + - [Chronicle audits](https://github.com/chronicleprotocol/scribe/tree/main/audits) ## Context This rate Provider bridges the eth/reth exchange rate & GBP/USD to gnosis chain. This is done via an oracle solution developed by chronicle. @@ -55,6 +55,6 @@ To save time, we do not bother pointing out low-severity/informational issues or ## Conclusion -**Summary judgment: ** +**Summary judgment: SAFE** -This rate provider should work well with Balancer pools. The oracle providing the rate data has various guardrails in place ensuring the integrity of the rate being provided. The `owner` of the rate provider has the capability to revert the call to `getRate`. However this potential revert scenario is guarded behind a Multisig of [1/4] for reth and [2/6] for GBP. +This rate provider should work well with Balancer pools. The oracle providing the rate data has various guardrails in place ensuring the integrity of the rate being provided. The `owner` of the rate provider has the capability to revert the call to `getRate`. However this potential revert scenario is guarded behind a Multisig of [2/6] for reth and [2/6] for GBP. diff --git a/rate-providers/registry.json b/rate-providers/registry.json index 9748229..1e85bbd 100644 --- a/rate-providers/registry.json +++ b/rate-providers/registry.json @@ -1103,7 +1103,7 @@ "upgradeableComponents": [] }, "0x92320D3C8Fd6BE59b22eB0eEe330901Fe4617f33": { - "asset": "", + "asset": "0x5Cb9073902F2035222B9749F8fB0c9BFe5527108", "name": "TollgateChronicleRateProvider", "summary": "", "review": "./TollgateChronicleRateProvider.md", From 592f7ece01c23a69a27a20cb8b9c9a3130c46f60 Mon Sep 17 00:00:00 2001 From: Daniel Date: Tue, 18 Jun 2024 09:20:49 -0600 Subject: [PATCH 5/5] Add checked by --- rate-providers/TollgateChronicleRateProvider.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rate-providers/TollgateChronicleRateProvider.md b/rate-providers/TollgateChronicleRateProvider.md index 62f02ec..aecef98 100644 --- a/rate-providers/TollgateChronicleRateProvider.md +++ b/rate-providers/TollgateChronicleRateProvider.md @@ -2,7 +2,7 @@ ## Details - Reviewed by: @mkflow27 -- Checked by: @\ +- Checked by: @danielmkm - Deployed at: - [gnosis:0xdc90e2680094314CEaB45CE15100F6e02cEB7ceD](https://gnosisscan.io/address/0xdc90e2680094314ceab45ce15100f6e02ceb7ced#code) - [gnosis:0x92320D3C8Fd6BE59b22eB0eEe330901Fe4617f33](https://gnosisscan.io/address/0x92320D3C8Fd6BE59b22eB0eEe330901Fe4617f33#code)