fix: #319 oracle quorum check & #333 reserve tracker add_currency dedup#428
Conversation
…e tracker add_currency dedup Pi-Defi-world#319 (acbu_oracle/src/lib.rs): update_rate previously allowed a single validator submission to bypass the source-count floor when sources was empty, and used only the hardcoded MIN_ORACLE_SOURCE_FEEDS constant rather than the configured min_signatures. Replace the conditional guard with an unconditional check that enforces max(min_signatures, MIN_ORACLE_SOURCE_FEEDS) sources, so a solitary malicious validator can never set a rate with fewer independent feeds than the quorum requires. Pi-Defi-world#333 (acbu_reserve_tracker/src/lib.rs): add add_currency() (admin-only) that iterates the currencies Vec before pushing and panics with DuplicateCurrency (8008) if the entry already exists, preventing double-counting of reserves in iteration-based totals. Also adds get_currencies() accessor and the backing storage key. Tests added for both fixes.
|
@hartz0 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughThe PR enforces a dynamic minimum source count in ChangesOracle dynamic quorum enforcement
Reserve tracker currency list management
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@acbu_oracle/src/lib.rs`:
- Around line 363-371: `update_rate` only checks the length of the
caller-supplied `sources` vector, so a single validator can still control the
median by submitting arbitrary prices. Update `update_rate` to require distinct
validator provenance for each entry, binding each price in `sources` to a unique
signer or authenticated source instead of trusting the raw `Vec<i128>`. Use the
existing `DATA_KEY.min_signatures`, `MIN_ORACLE_SOURCE_FEEDS`, and
`OracleError::InsufficientOracleSources` flow as the guard, but extend the
verification logic so quorum is based on independent attestations rather than
array size alone.
In `@acbu_reserve_tracker/src/lib.rs`:
- Around line 265-289: `add_currency` and `get_currencies` only manage the
tracked-currencies list and do not affect reserve totals, so the
duplicate-counting issue is still in the reserve data path. Update the reserve
logic in `update_reserve` and the reserve-sufficiency flow that reads
`get_all_reserves()` / `Map<CurrencyCode, ReserveData>` so currency registration
and reserve accounting stay in sync, and prevent `update_reserve` from
introducing currencies that were never registered (or make it register them
consistently through the same source of truth as `DATA_KEY.currencies`).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d0bebdb-bbc4-40b4-8d78-0f72c30b0f6d
📒 Files selected for processing (4)
acbu_oracle/src/lib.rsacbu_oracle/src/tests.rsacbu_reserve_tracker/src/lib.rsacbu_reserve_tracker/tests/test.rs
| let min_sigs: u32 = env | ||
| .storage() | ||
| .instance() | ||
| .get(&DATA_KEY.min_signatures) | ||
| .unwrap(); | ||
| let required = min_sigs.max(MIN_ORACLE_SOURCE_FEEDS); | ||
| if sources.len() < required { | ||
| env.panic_with_error(OracleError::InsufficientOracleSources); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how `sources` is produced/validated and whether any multi-validator
# aggregation exists, to confirm whether a single caller controls all sources.
rg -nP --type=rust -C4 '\bupdate_rate\b'
rg -nP --type=rust -C3 'sources' acbu_oracle/src
rg -nP --type=rust -C3 'min_signatures|MIN_ORACLE_SOURCE_FEEDS'Repository: Pi-Defi-world/acbu-smart-contract
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the contract file and map its structure.
git ls-files | rg '^acbu_oracle/src/lib\.rs$|^acbu_oracle/src/'
ast-grep outline acbu_oracle/src/lib.rs --view expanded
# Show the relevant region around the reported lines.
sed -n '320,430p' acbu_oracle/src/lib.rs
# Find the update_rate definition and its call sites.
rg -n "update_rate|min_signatures|MIN_ORACLE_SOURCE_FEEDS|validator\.require_auth|sources:" acbu_oracle/src -n -C 3Repository: Pi-Defi-world/acbu-smart-contract
Length of output: 16584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the oracle contract implementation and nearby code.
if [ -f acbu_oracle/src/lib.rs ]; then
echo '--- outline ---'
ast-grep outline acbu_oracle/src/lib.rs --view expanded || true
echo '--- excerpt ---'
sed -n '330,430p' acbu_oracle/src/lib.rs
fi
echo '--- call sites / related symbols ---'
rg -n -C 3 '\bupdate_rate\b|min_signatures|MIN_ORACLE_SOURCE_FEEDS|validator\.require_auth|sources:' acbu_oracle/src || true
echo '--- repo-wide oracle references ---'
rg -n -C 2 'oracle|update_rate|min_signatures|MIN_ORACLE_SOURCE_FEEDS' . || trueRepository: Pi-Defi-world/acbu-smart-contract
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the oracle contract file in the relevant region.
wc -l acbu_oracle/src/lib.rs || true
sed -n '1,220p' acbu_oracle/src/lib.rs
sed -n '220,420p' acbu_oracle/src/lib.rs
# Search for any validation that each source comes from a distinct validator or signed payload.
rg -n -C 3 'distinct|quorum|signature|signatures|validator|feed|oracle source|source.*validator|sources.*validator|require_auth|median' acbu_oracle/src || trueRepository: Pi-Defi-world/acbu-smart-contract
Length of output: 37771
Source-count floor still doesn't create quorum
update_rate still accepts a caller-supplied Vec<i128>, so one authenticated validator can pass this length check with three self-chosen prices and drive the median arbitrarily. If these are meant to be independent validator attestations, each source needs to be bound to a distinct signer/provenance; otherwise this only enforces array size, not multi-validator consensus.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@acbu_oracle/src/lib.rs` around lines 363 - 371, `update_rate` only checks the
length of the caller-supplied `sources` vector, so a single validator can still
control the median by submitting arbitrary prices. Update `update_rate` to
require distinct validator provenance for each entry, binding each price in
`sources` to a unique signer or authenticated source instead of trusting the raw
`Vec<i128>`. Use the existing `DATA_KEY.min_signatures`,
`MIN_ORACLE_SOURCE_FEEDS`, and `OracleError::InsufficientOracleSources` flow as
the guard, but extend the verification logic so quorum is based on independent
attestations rather than array size alone.
| pub fn add_currency(env: Env, currency: CurrencyCode) { | ||
| Self::check_admin(&env); | ||
| let mut currencies: Vec<CurrencyCode> = env | ||
| .storage() | ||
| .instance() | ||
| .get(&DATA_KEY.currencies) | ||
| .unwrap_or(Vec::new(&env)); | ||
| for c in currencies.iter() { | ||
| if c == currency { | ||
| env.panic_with_error(ReserveTrackerError::DuplicateCurrency); | ||
| } | ||
| } | ||
| currencies.push_back(currency); | ||
| env.storage() | ||
| .instance() | ||
| .set(&DATA_KEY.currencies, ¤cies); | ||
| } | ||
|
|
||
| /// Return the list of tracked currencies. | ||
| pub fn get_currencies(env: Env) -> Vec<CurrencyCode> { | ||
| env.storage() | ||
| .instance() | ||
| .get(&DATA_KEY.currencies) | ||
| .unwrap_or(Vec::new(&env)) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all reads/writes of the currencies storage key and any aggregation over reserves
rg -nP --type=rust 'DATA_KEY\.currencies|\.currencies\b' acbu_reserve_tracker
echo '--- consumers of get_currencies in reserve_tracker ---'
rg -nP --type=rust '\bget_currencies\s*\(' acbu_reserve_tracker
echo '--- reserve aggregation sites ---'
rg -nP --type=rust 'fn (total_reserves|is_reserve_sufficient)\b' -A 25 acbu_reserve_tracker/src/lib.rsRepository: Pi-Defi-world/acbu-smart-contract
Length of output: 2189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant contract code and tests around currency tracking and reserve updates.
FILE=acbu_reserve_tracker/src/lib.rs
TEST=acbu_reserve_tracker/tests/test.rs
echo '--- lib.rs (currency/reserve functions) ---'
sed -n '1,360p' "$FILE" | nl -ba | sed -n '1,360p'
echo '--- tests mentioning currencies/reserves ---'
rg -n -A3 -B3 'get_currencies|add_currency|update_reserve|total_reserves|is_reserve_sufficient' "$TEST"Repository: Pi-Defi-world/acbu-smart-contract
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any other consumers of the currencies list across the repo.
rg -n --type=rust '\bget_currencies\s*\(|DATA_KEY\.currencies|\.currencies\b|add_currency\s*\(' .Repository: Pi-Defi-world/acbu-smart-contract
Length of output: 2816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant contract code and tests around currency tracking and reserve updates.
FILE=acbu_reserve_tracker/src/lib.rs
TEST=acbu_reserve_tracker/tests/test.rs
echo '--- lib.rs (currency/reserve functions) ---'
sed -n '1,360p' "$FILE" | nl -ba | sed -n '1,360p'
echo '--- tests mentioning currencies/reserves ---'
rg -n -A3 -B3 'get_currencies|add_currency|update_reserve|total_reserves|is_reserve_sufficient' "$TEST"Repository: Pi-Defi-world/acbu-smart-contract
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any other consumers of the currencies list across the repo.
rg -n --type=rust '\bget_currencies\s*\(|DATA_KEY\.currencies|\.currencies\b|add_currency\s*\(' .Repository: Pi-Defi-world/acbu-smart-contract
Length of output: 2816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant functions in acbu_reserve_tracker without relying on nl.
sed -n '110,240p' acbu_reserve_tracker/src/lib.rs
echo '---'
sed -n '250,300p' acbu_reserve_tracker/src/lib.rsRepository: Pi-Defi-world/acbu-smart-contract
Length of output: 6524
currencies doesn’t affect reserve totals
add_currency only writes DATA_KEY.currencies; reserve sufficiency still uses get_all_reserves() / Map<CurrencyCode, ReserveData>, so this change doesn’t address the duplicate-counting issue in #333. update_reserve can also add currencies that were never registered, so the two stores can diverge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@acbu_reserve_tracker/src/lib.rs` around lines 265 - 289, `add_currency` and
`get_currencies` only manage the tracked-currencies list and do not affect
reserve totals, so the duplicate-counting issue is still in the reserve data
path. Update the reserve logic in `update_reserve` and the reserve-sufficiency
flow that reads `get_all_reserves()` / `Map<CurrencyCode, ReserveData>` so
currency registration and reserve accounting stay in sync, and prevent
`update_reserve` from introducing currencies that were never registered (or make
it register them consistently through the same source of truth as
`DATA_KEY.currencies`).
|
Some checks were not successful |
2 participants
closes #319
closes #333
#319 (acbu_oracle/src/lib.rs): update_rate previously allowed a single validator submission to bypass the source-count floor when sources was empty, and used only the hardcoded MIN_ORACLE_SOURCE_FEEDS constant rather than the configured min_signatures. Replace the conditional guard with an unconditional check that enforces max(min_signatures, MIN_ORACLE_SOURCE_FEEDS) sources, so a solitary malicious validator can never set a rate with fewer independent feeds than the quorum requires.
#333 (acbu_reserve_tracker/src/lib.rs): add add_currency() (admin-only) that iterates the currencies Vec before pushing and panics with DuplicateCurrency (8008) if the entry already exists, preventing double-counting of reserves in iteration-based totals. Also adds get_currencies() accessor and the backing storage key.
Tests added for both fixes.
Summary by CodeRabbit
New Features
Bug Fixes