Sonic mainnet: queue wOS oracle hotfix to remove OS/S dependency

[Git-on-my-level]

Summary

• add a new mainnet-only deploy script that rewires the wOS composite USD feed to remove the OS/S dependency
• route wOS/USD as wOS/OS * S/USD (implicit OS = S assumption)
• execute through GovernanceExecutor so updates are queued/executed via governance Safe
• preserve existing threshold parameters when rewriting the composite feed
• update Sonic mainnet config so future deployments use direct S/USD for wOS composite feed

Deploy behavior

• script: deploy/20_wos_oracle_hotfix/01_queue_remove_os_s_dependency.ts
• network guard: mainnet-only (isMainnet)
• governance: uses Safe path on mainnet, queues removeCompositeFeed + addCompositeFeed
• idempotent: exits when wOS already points to direct S/USD

Validation

• corepack yarn lint:eslint
• corepack yarn hardhat compile

[claude]

Claude finished @Git-on-my-level's task —— View job

---

Code Review Complete ✅

Reviewing PR #166: Sonic mainnet wOS oracle hotfix to remove OS/S dependency

Todo List:

• Read repository guidelines (CLAUDE.md) - No repo guidelines found
• Analyze new deploy script: deploy/20_wos_oracle_hotfix/01_queue_remove_os_s_dependency.ts
• Review config changes: config/networks/sonic_mainnet.ts
• Check code quality and best practices
• Identify potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage and validation
• Provide comprehensive feedback

---

📋 Code Review Feedback

✅ Strengths

1. Robust Governance Integration: Excellent use of GovernanceExecutor pattern for Safe multisig operations on mainnet
2. Comprehensive Safety Checks:
   • Network guard (isMainnet) prevents accidental execution on wrong networks
   • Idempotency check exits early if already configured correctly
   • Post-deployment verification ensures the change worked
   • Price sanity check validates the new feed is alive
3. Clean Error Handling: Proper validation of required dependencies and clear error messages
4. Good Separation of Concerns: Helper functions createRemoveCompositeFeedTx and createAddCompositeFeedTx improve readability

⚠️ Issues & Improvements

Security Concerns

1. Critical Oracle Assumption (Line 335): The config assumes OS = S (1:1 parity) when routing wOS/USD as wOS/OS * S/USD. This assumption should be:

   • Documented with clear comments explaining the business rationale
   • Monitored post-deployment to ensure the peg holds
   • Have a recovery plan if the assumption breaks

2. Threshold Preservation (Lines 105-112): While preserving existing thresholds is good for consistency, consider whether the threshold values are still appropriate for the new S/USD feed vs the old OS/S * S/USD composite.

Code Quality Issues

1. String Comparison Inefficiency (Lines 96, 100, 178): Using .toLowerCase() for address comparisons is inefficient. Use ethers.getAddress() for proper checksum comparison:

   // Instead of:
   if (currentFeed1.toLowerCase() !== wOSToOSConverter.toLowerCase())
   // Use:
   if (ethers.getAddress(currentFeed1) !== ethers.getAddress(wOSToOSConverter))

2. Type Safety (Lines 89-90): The as string cast could be unsafe. Consider proper type validation:

   const currentFeed1 = String(currentFeed.feed1);
   const currentFeed2 = String(currentFeed.feed2);

3. Inconsistent BigInt Handling (Line 183): Mixing BigInt() constructor with toString():

   // More consistent:
   const rewiredPrice = BigInt(priceInfo[0].toString());

Potential Bugs

1. Missing Await (Line 182): The getPriceInfo call result should be properly destructured:

   const [priceInfo, isAlive] = await wrapper.getPriceInfo(wOSAddress);
   const rewiredPrice = BigInt(priceInfo.toString());

2. Safe Transaction Ordering: The script queues removeCompositeFeed then addCompositeFeed. Ensure the Safe execution order is guaranteed, as out-of-order execution could leave the oracle in an inconsistent state.

Performance & Gas Considerations

1. Redundant Contract Calls: The script makes multiple calls to wrapper.compositeFeeds() (lines 88, 176). Consider caching the result after the update.

2. Gas Estimation: For mainnet deployment, consider adding gas estimation logging for the Safe transactions.

🧪 Test Coverage Concerns

1. Missing Unit Tests: No accompanying tests for this critical oracle migration
2. Integration Test Gap: Should include fork tests validating:
   • Price feed continues working after migration
   • Price deviation is within expected bounds
   • Threshold behavior with new feed structure
3. Edge Case Testing: Test scenarios like failed transactions, partial execution, or reverted Safe operations

🏗️ Config Change Review

File: config/networks/sonic_mainnet.ts:332-340

• ✅ Correctly updates the future config to use direct S/USD feed
• ✅ Maintains consistency with the deploy script's target state
• ✅ Proper address format and structure

📊 Deployment Safety

Excellent safety measures:

• Mainnet-only guard
• Idempotency protection
• Governance multisig requirement
• Post-deployment validation
• Price liveness verification

🔧 Recommendations

1. Add comprehensive tests covering happy path and edge cases
2. Document the OS=S assumption prominently in comments and deployment docs
3. Consider adding alerting for price deviations post-deployment
4. Fix address comparison efficiency issues
5. Add gas estimation logging for transparency
6. Consider a rollback script in case issues arise

📈 Overall Assessment

This is a well-structured, safety-conscious oracle migration with appropriate governance controls. The code follows established patterns and includes good validation. Main concerns are around testing coverage and the critical business assumption that OS = S.

Recommendation: ✅ Approve with suggested improvements - particularly adding tests and documenting the oracle assumption.

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee6d2837b9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Parse getPriceInfo result without indexing bigint

wrapper.getPriceInfo(wOSAddress) returns (price, isAlive), so after const [priceInfo, isAlive] = ... the priceInfo variable is already the price value; indexing it with priceInfo[0] produces undefined and BigInt(undefined) throws. In runs where the script executes changes directly (for example, deployer has ORACLE_MANAGER_ROLE on mainnet), this causes the post-update verification path to fail even when the feed rewrite succeeded.

Useful? React with 👍 / 👎.

[Lange1337]

we dont need remove, add overwrites completely

[claude]

Claude finished @Git-on-my-level's task —— View job

---

Claude Code finished reviewing the PR ✅

Todo List:

• Read repository guidelines (CLAUDE.md) - No repo guidelines found
• Analyze the deploy script: deploy/20_wos_oracle_hotfix/01_queue_remove_os_s_dependency.ts
• Review config changes: config/networks/sonic_mainnet.ts
• Evaluate code quality and best practices
• Identify potential bugs or issues
• Assess performance considerations
• Review security concerns
• Analyze test coverage
• Provide comprehensive feedback

---

📋 Code Review Feedback

✅ Strengths

1. Robust Governance Integration: Excellent use of GovernanceExecutor pattern for Safe multisig operations on mainnet
2. Comprehensive Safety Checks:
   • Network guard (isMainnet) prevents accidental execution on wrong networks
   • Idempotency check exits early if already configured correctly
   • Post-deployment verification ensures the change worked
   • Price sanity check validates the new feed is alive
3. Clean Error Handling: Proper validation of required dependencies and clear error messages
4. Good Separation of Concerns: Helper functions would improve readability (though not used in current implementation)

⚠️ Critical Issues Found

🐛 Bug: getPriceInfo Result Parsing (Line 154)

CRITICAL: The getPriceInfo call returns [price, isAlive], but the code incorrectly indexes the result:

const [priceInfo, isAlive] = await wrapper.getPriceInfo(wOSAddress);
const rewiredPrice = BigInt(priceInfo[0]); // ❌ priceInfo is already the price, not an array

Fix: Remove the [0] indexing:

const rewiredPrice = BigInt(priceInfo.toString());

This matches the issue identified by @chatgpt-codex-connector.

🚨 Security Concerns

1. Critical Oracle Assumption (Line 335): The config assumes OS = S (1:1 parity) when routing wOS/USD as wOS/OS * S/USD. This assumption:

   • Should be documented with clear comments explaining the business rationale
   • Needs monitoring post-deployment to ensure the peg holds
   • Requires a recovery plan if the assumption breaks

2. Unnecessary removeCompositeFeed: As @Lange1337 noted, the removeCompositeFeed call is unnecessary since addCompositeFeed completely overwrites. This adds unnecessary complexity and gas costs.

💻 Code Quality Issues

1. Inefficient Address Comparisons (Lines 85, 89): Using .toLowerCase() for address comparisons:

   // Current (inefficient):
   if (currentFeed1.toLowerCase() !== wOSToOSConverter.toLowerCase())
   if (currentFeed2.toLowerCase() === sUsdFeed.toLowerCase())
   
   // Better:
   if (ethers.getAddress(currentFeed1) !== ethers.getAddress(wOSToOSConverter))
   if (ethers.getAddress(currentFeed2) === ethers.getAddress(sUsdFeed))

2. Inconsistent Type Handling (Lines 78-79): Casting to string could be unsafe:

   // Current:
   const currentFeed1 = currentFeed.feed1 as string;
   const currentFeed2 = currentFeed.feed2 as string;
   
   // Better:
   const currentFeed1 = String(currentFeed.feed1);
   const currentFeed2 = String(currentFeed.feed2);

⚡ Performance Considerations

1. Redundant Contract Calls: Multiple calls to wrapper.compositeFeeds() (lines 77, 147). Consider caching the result after update.

2. Gas Optimization: For mainnet deployment, consider adding gas estimation logging for transparency.

🧪 Test Coverage Concerns

1. Missing Unit Tests: No accompanying tests for this critical oracle migration
2. Integration Test Gap: Should include fork tests validating:
   • Price feed continues working after migration
   • Price deviation is within expected bounds
   • Threshold behavior with new feed structure
3. Edge Case Testing: Test scenarios like failed transactions or reverted Safe operations

🏗️ Config Change Review (sonic_mainnet.ts:332-340)

✅ Correct Implementation: The config properly updates future deployments to use direct S/USD feed, maintaining consistency with the deploy script's target state.

📊 Deployment Safety Assessment

Excellent safety measures:

• ✅ Mainnet-only guard
• ✅ Idempotency protection
• ✅ Governance multisig requirement
• ✅ Post-deployment validation
• ❌ Price liveness check has parsing bug

🔧 Recommendations

1. 🚨 HIGH PRIORITY: Fix the priceInfo[0] parsing bug before deployment
2. 📝 Document the OS=S assumption prominently in comments and deployment docs
3. 🧪 Add comprehensive tests covering happy path and edge cases
4. ⚡ Consider removing unnecessary removeCompositeFeed as suggested by reviewer
5. 🔍 Fix address comparison inefficiencies
6. 📊 Add gas estimation logging for transparency
7. 🔄 Consider a rollback script in case issues arise

📈 Overall Assessment

This is a well-structured, safety-conscious oracle migration with appropriate governance controls. However, the critical parsing bug in the price validation must be fixed before deployment.

Recommendation: ⚠️ Request Changes - Fix the critical bug and address the unnecessary removeCompositeFeed operation, then this will be ready for deployment.

---

[Git-on-my-level]

Deployed to mainnet, need to commit artifacts