Skip to content

feat: two-phase MultisigManager threshold rotation#643

Open
dot-enny wants to merge 2 commits into
Predictify-org:masterfrom
dot-enny:feat/multisig-threshold-two-phase
Open

feat: two-phase MultisigManager threshold rotation#643
dot-enny wants to merge 2 commits into
Predictify-org:masterfrom
dot-enny:feat/multisig-threshold-two-phase

Conversation

@dot-enny

Copy link
Copy Markdown

Pull Request Description

📋 Basic Information

Type of Change

Please select the type of change this PR introduces:

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected - due to signature change from set_threshold to propose/confirm)
  • 📚 Documentation update
  • 🧪 Test addition/update
  • 🔧 Refactoring (no functional changes)
  • ⚡ Performance improvement
  • 🔒 Security fix
  • 🎨 UI/UX improvement
  • 🚀 Deployment/Infrastructure change

Related Issues

Closes #632

Priority Level

  • 🔴 Critical (blocking other development)
  • 🟡 High (significant impact)
  • [ X ] 🟢 Medium (moderate impact)
  • 🔵 Low (minor improvement)

📝 Detailed Description

What does this PR do?

  1. Branch Created: Forked and checked out a new branch feat/multisig-threshold-two-phase.
  2. Implementation in admin.rs:
    • Replaced set_threshold with propose_threshold(env, admin, new_threshold).
    • Added confirm_threshold(env) which enforces a 24-hour configured delay (THRESHOLD_ROTATION_DELAY: u64 = 86400).
    • Added cancel_threshold_proposal(env, admin) to allow cancelling a pending rotation proposal.
    • Introduced a PendingThresholdUpdate struct to hold the proposal state in persistent storage.
  3. Events inside events.rs:
    • Added MultisigThresholdProposedEvent and MultisigThresholdConfirmedEvent structures.
    • Implemented emit_threshold_proposed and emit_threshold_confirmed inside EventEmitter.
  4. Test Suite in multi_admin_multisig_tests.rs:
    • Updated existing tests by substituting MultisigManager::set_threshold with a localized set_threshold_helper which executes the two-phase commit seamlessly behind the scenes (proposing, warping time by 24h, confirming).
    • Added test_threshold_confirm_before_delay to guarantee a threshold confirmation is rejected before the delay.
    • Added test_threshold_double_propose to verify that proposing a second time gracefully overrides the first.
    • Added test_threshold_cancel_propose to document and assert the correct cancellation path.
  5. Code Practices:
    • Strictly no new .unwrap() statements were introduced into contract code (admin.rs and events.rs). Tests utilize them inherently for assertions.
    • Fully committed with message "feat: two-phase MultisigManager threshold rotation".

Why is this change needed?

In the previous implementation of MultisigManager, the threshold could be rotated instantly in a single transaction. This introduced a significant security risk where a compromised admin key or accidental inputs could instantly change the threshold and lock out other administrators.

By enforcing a two-phase commit with a mandatory 24-hour delay, other admins are given a grace period to notice the proposed changes, inspect the proposal, and cancel it if it was malicious or erroneous.

How was this tested?

  • Unit & Integration Tests:
    • Updated the existing test suite to ensure compatibility with the new two-phase system.
    • Added new edge case tests in multi_admin_multisig_tests.rs:
      • test_threshold_confirm_before_delay: Assert that confirmation fails if attempted before the 24-hour delay.
      • test_threshold_double_propose: Assert that a newer proposal correctly overwrites a pending one.
      • test_threshold_cancel_propose: Assert that pending proposals can be cancelled.

Alternative Solutions Considered

  • Configurable Delay Time: Considered making the delay configurable at runtime, but decided to keep it as a constant (THRESHOLD_ROTATION_DELAY = 86400 seconds) to keep the codebase simple, clean, and highly secure.

🏗️ Smart Contract Specific

Contract Changes

Please check all that apply:

  • Core contract logic modified
  • Oracle integration changes (Pyth/Reflector)
  • New functions added
  • Existing functions modified
  • Storage structure changes
  • Events added/modified
  • Error handling improved
  • Gas optimization
  • Access control changes
  • Admin functions modified
  • Fee structure changes

Oracle Integration

  • Pyth oracle integration affected
  • Reflector oracle integration affected
  • Oracle configuration changes
  • Price feed handling modified
  • Oracle fallback mechanisms
  • Price validation logic

Market Resolution Logic

  • Hybrid resolution algorithm changed
  • Dispute mechanism modified
  • Fee structure updated
  • Voting mechanism changes
  • Community weight calculation
  • Oracle weight calculation

Security Considerations

  • Access control reviewed
  • Reentrancy protection
  • Input validation
  • Overflow/underflow protection
  • Oracle manipulation protection

🧪 Testing

Test Coverage

  • Unit tests added/updated
  • Integration tests added/updated
  • All tests passing locally
  • Manual testing completed
  • Oracle integration tested
  • Edge cases covered
  • Error conditions tested
  • Gas usage optimized
  • Cross-contract interactions tested

Test Results

# Run tests locally via:
cargo test -p predictify-hybrid
# Expected output: All tests passed.


📸 Screenshots (if applicable)
N/A

🔗 Additional Resources
Related Issue: #632
💬 Notes for Reviewers
Please pay special attention to:

The time validation logic in confirm_threshold relative to env.ledger().timestamp().
The storage cleanup logic during confirmation and cancellation.


Questions for reviewers:
Is the 24-hour lock delay (86400 seconds) optimal for our current workflows?

@drips-wave

drips-wave Bot commented Jun 26, 2026

Copy link
Copy Markdown

@dot-enny 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! 🚀

Learn more about application limits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add MultisigManager threshold-rotation safety with two-phase commit

1 participant