Make oracle and treasury configurable through setter functions .#20
Open
Sourav-IIITBPL wants to merge 2 commits into
Open
Make oracle and treasury configurable through setter functions .#20Sourav-IIITBPL wants to merge 2 commits into
Sourav-IIITBPL wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThe changes make oracle and treasury state mutable with setter functions and emit events in StableCoin, normalize uint to uint256 types in the IPyth interface for consistency, reformat event and function declarations across multiple files, and remove minor whitespace. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/StableCoin.sol (2)
82-83: Consider indexing address parameters for better event filtering.Adding
indexedto the address parameters would enable efficient off-chain filtering by oracle or treasury address, which is useful for governance monitoring tools.♻️ Suggested improvement
- event OracleUpdated(address oldOracle, address newOracle, bytes32 oldPriceId, bytes32 newPriceId); - event TreasuryUpdated(address oldTreasury, address newTreasury); + event OracleUpdated(address indexed oldOracle, address indexed newOracle, bytes32 oldPriceId, bytes32 newPriceId); + event TreasuryUpdated(address indexed oldTreasury, address indexed newTreasury);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/StableCoin.sol` around lines 82 - 83, Update the OracleUpdated and TreasuryUpdated event declarations to make the address parameters indexed so off-chain tools can filter by oracle/treasury; specifically modify the OracleUpdated event (OracleUpdated) to mark oldOracle and newOracle as indexed and modify TreasuryUpdated (TreasuryUpdated) to mark oldTreasury and newTreasury as indexed, then run a quick compile to ensure the ABI changes are valid.
166-173: Implementation is correct; consider operational safeguards.The function logic is sound with proper validation and access control. However, note that treasury transfers are immediate with no timelock or multi-sig requirement. For production deployments, consider implementing a timelock pattern or ensuring the treasury address is a multi-sig/governance contract to prevent single-point-of-failure scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/StableCoin.sol` around lines 166 - 173, setTreasury currently allows immediate swaps of TREASURY (function setTreasury) which is correct but risky for production; add operational safeguards by requiring a timelock or multi-sig/governance-controlled address for TREASURY: either integrate a timelock mechanism (e.g., queueTreasuryChange / executeTreasuryChange flow) around setTreasury or add a runtime check that newTreasury is a known multisig/governance contract (e.g., verify support for a specific interface or require isTrustedGovernance(newTreasury)); emit TreasuryUpdated only when the queued change is executed (or after the verification), and keep existing access control (onlyTreasury) in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/StableCoin.sol`:
- Around line 154-164: The setOracle function validates newOracle but not the
price identifier; add a check in setOracle to require(newPriceId != bytes32(0),
"invalid price id") (or similar) before assigning PRICE_ID to prevent setting
PRICE_ID to zero which would break getBasePriceInPeggedAsset(), fission(), and
transmute functions; keep the rest of the function (capture oldPriceId, assign
PRICE_ID, emit OracleUpdated) unchanged so the OracleUpdated(oldPriceId,
newPriceId) still reflects the validated value.
---
Nitpick comments:
In `@src/StableCoin.sol`:
- Around line 82-83: Update the OracleUpdated and TreasuryUpdated event
declarations to make the address parameters indexed so off-chain tools can
filter by oracle/treasury; specifically modify the OracleUpdated event
(OracleUpdated) to mark oldOracle and newOracle as indexed and modify
TreasuryUpdated (TreasuryUpdated) to mark oldTreasury and newTreasury as
indexed, then run a quick compile to ensure the ABI changes are valid.
- Around line 166-173: setTreasury currently allows immediate swaps of TREASURY
(function setTreasury) which is correct but risky for production; add
operational safeguards by requiring a timelock or
multi-sig/governance-controlled address for TREASURY: either integrate a
timelock mechanism (e.g., queueTreasuryChange / executeTreasuryChange flow)
around setTreasury or add a runtime check that newTreasury is a known
multisig/governance contract (e.g., verify support for a specific interface or
require isTrustedGovernance(newTreasury)); emit TreasuryUpdated only when the
queued change is executed (or after the verification), and keep existing access
control (onlyTreasury) in place.
🪄 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: 9abf6281-3eb1-4594-aa95-557c746bf871
📒 Files selected for processing (5)
script/Deploy.s.solsrc/StableCoin.solsrc/StableCoinFactory.solsrc/interfaces/IPyth.solsrc/tokens/Tokeon.sol
💤 Files with no reviewable changes (1)
- script/Deploy.s.sol
Comment on lines
+154
to
+164
| function setOracle(address newOracle, bytes32 newPriceId) external onlyTreasury { | ||
| require(newOracle != address(0), "invalid oracle"); | ||
|
|
||
| address oldOracle = address(PYTH_ORACLE); | ||
| bytes32 oldPriceId = PRICE_ID; | ||
|
|
||
| PYTH_ORACLE = IPyth(newOracle); | ||
| PRICE_ID = newPriceId; | ||
|
|
||
| emit OracleUpdated(oldOracle, newOracle, oldPriceId, newPriceId); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing validation for newPriceId.
The function validates newOracle but not newPriceId. Setting PRICE_ID to bytes32(0) would cause oracle price lookups to fail or return invalid data in functions like getBasePriceInPeggedAsset(), fission(), and the transmute functions.
🛡️ Proposed fix to add validation
function setOracle(address newOracle, bytes32 newPriceId) external onlyTreasury {
require(newOracle != address(0), "invalid oracle");
+ require(newPriceId != bytes32(0), "invalid price id");
address oldOracle = address(PYTH_ORACLE);
bytes32 oldPriceId = PRICE_ID;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/StableCoin.sol` around lines 154 - 164, The setOracle function validates
newOracle but not the price identifier; add a check in setOracle to
require(newPriceId != bytes32(0), "invalid price id") (or similar) before
assigning PRICE_ID to prevent setting PRICE_ID to zero which would break
getBasePriceInPeggedAsset(), fission(), and transmute functions; keep the rest
of the function (capture oldPriceId, assign PRICE_ID, emit OracleUpdated)
unchanged so the OracleUpdated(oldPriceId, newPriceId) still reflects the
validated value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Currently,
PYTH_ORACLE,PRICE_ID, andTREASURYinStableCoin.solare immutable. This prevents:Changes
This prepares the system for:
Summary by CodeRabbit
New Features
setOracle()andsetTreasury()functions for configuration updates.OracleUpdatedandTreasuryUpdatedevents to track administrative changes.Chores