MystenLabs / sui

Sui, a next-generation smart contract platform with high throughput, low latency, and an asset-oriented programming model powered by the Move programming language
https://sui.io
Apache License 2.0
6.28k stars 11.22k forks source link

Fix integer overflow and underflow in DeferralKey methods and add corresponding tests #20387

Open xiaodi007 opened 1 week ago

xiaodi007 commented 1 week ago

Description

This PR addresses two critical issues in the DeferralKey implementation that could lead to integer overflow and underflow, potentially causing panics or incorrect behavior. It also includes new test cases to verify the correctness of the fixes.

Issues Fixed:

  1. Prevent Integer Overflow in range_for_up_to_consensus_round

    • Issue: When consensus_round equals u64::MAX, using checked_add(1).unwrap() in DeferralKey::range_for_up_to_consensus_round causes a panic due to overflow.
    • Solution: Replace checked_add(1).unwrap() with saturating_add(1) to safely handle the maximum value without causing a panic.
  2. Prevent Integer Underflow in transaction_deferral_within_limit

    • Issue: In transaction_deferral_within_limit, when future_round is less than deferred_from_round, subtracting them causes an underflow, resulting in incorrect behavior.
    • Solution: Use checked_sub to safely perform the subtraction, and handle the None case by returning false.

Test plan

  1. Test Name: test_range_for_up_to_consensus_round_no_overflow

    Description: Verifies that DeferralKey::range_for_up_to_consensus_round does not overflow when consensus_round is u64::MAX, and that it correctly returns future_round equal to u64::MAX.

  2. Test Name: test_transaction_deferral_within_limit_underflow

    Description: Ensures that transaction_deferral_within_limit correctly handles cases where future_round is less than deferred_from_round, preventing integer underflow and returning false.


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

vercel[bot] commented 1 week ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 26, 2024 1:44am
3 Skipped Deployments | Name | Status | Preview | Comments | Updated (UTC) | | :--- | :----- | :------ | :------- | :------ | | **multisig-toolkit** | ⬜️ Ignored ([Inspect](https://vercel.com/mysten-labs/multisig-toolkit/FrUUV1XVFBW49NY7eACAwRpEYa7e)) | [Visit Preview](https://multisig-toolkit-git-fork-xiaodi007-xiaodi00-29afc8-mysten-labs.vercel.app) | | Nov 26, 2024 1:44am | | **sui-kiosk** | ⬜️ Ignored ([Inspect](https://vercel.com/mysten-labs/sui-kiosk/Ah8B2FpGqhpbTsd1Na9DxFBFbwpQ)) | [Visit Preview](https://sui-kiosk-git-fork-xiaodi007-xiaodi007-updat-617587-mysten-labs.vercel.app) | | Nov 26, 2024 1:44am | | **sui-typescript-docs** | ⬜️ Ignored ([Inspect](https://vercel.com/mysten-labs/sui-typescript-docs/8d5XcXhy5pWuuzBWCuoJoNwriy9V)) | [Visit Preview](https://sui-typescript-docs-git-fork-xiaodi007-xiaod-c10f2e-mysten-labs.vercel.app) | | Nov 26, 2024 1:44am |
bmwill commented 6 days ago

@mystenmark and/or @aschran would be a better reviewer for this change

mystenmark commented 6 days ago

@xiaodi007 Thanks for the contribution - see the comments above. I believe it is impossible for either issue to occur, but in the second case we could be more defensive.

xiaodi007 commented 6 days ago

@mystenmark @aschran I'm very grateful for your valuable feedback. I change my code to be more defensive.

Add a debug_fatal! to enforce the invariant during object creation; and adjust future_round to be at least deferred_from_round in release builds to maintain the invariant without panicking in new_for_consensus_round.

Add debug_assert!(future_round >= deferred_from_round); and use saturating_sub in transaction_deferral_within_limit.

Thanks again for your insightful feedback! I've made the updates to the PR as you suggested. I'll keep contributing actively. The updates are ready now—please take a look!

xiaodi007 commented 3 days ago

@aschran Done!