Skip to content

Commit 82f372c

Browse files
alex-mystenalex-mysten
committed
add u64 overflow prevention
1 parent ee1280f commit 82f372c

File tree

11 files changed

+245
-47
lines changed

11 files changed

+245
-47
lines changed

‎Cargo.lock‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -440,7 +440,7 @@ msim-macros = { git = "https://github.com/MystenLabs/mysten-sim.git", rev = "427
440440
multiaddr = "0.17.0"
441441
nexlint = { git = "https://github.com/nextest-rs/nexlint.git", rev = "7ce56bd591242a57660ed05f14ca2483c37d895b" }
442442
nexlint-lints = { git = "https://github.com/nextest-rs/nexlint.git", rev = "7ce56bd591242a57660ed05f14ca2483c37d895b" }
443-
nonempty = "0.9.0"
443+
nonempty = { version = "0.9.0", features = ["serialize"] }
444444
nonzero_ext = "0.3.0"
445445
notify = "6.1.1"
446446
ntest = "0.9.0"

‎crates/sui-adapter-transactional-tests/tests/address_balances/large_balance_transfer.move‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
// Copyright (c) Mysten Labs, Inc.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
// Test demonstrating arithmetic overflow protection when withdrawing from address balances.
5+
// Creates two independent Supply objects, mints 18446744073709551614 (u64::MAX - 1) from each,
6+
// sends both amounts to address A, then attempts to withdraw both amounts in a single PTB.
7+
// The withdraw operation fails with arithmetic error because the total exceeds u64::MAX.
8+
9+
//# init --addresses test=0x0 --accounts A B --enable-accumulators --simulator
10+
11+
//# publish --sender A
12+
module test::large_balance {
13+
use sui::balance::{Self, Supply};
14+
15+
public struct MARKER has drop {}
16+
17+
public struct SupplyHolder has key, store {
18+
id: UID,
19+
supply: Supply<MARKER>,
20+
}
21+
22+
public fun create_holder(ctx: &mut TxContext): SupplyHolder {
23+
SupplyHolder {
24+
id: object::new(ctx),
25+
supply: balance::create_supply(MARKER {}),
26+
}
27+
}
28+
29+
public fun send_large_balance(holder: &mut SupplyHolder, recipient: address, amount: u64) {
30+
let balance = holder.supply.increase_supply(amount);
31+
balance::send_funds(balance, recipient);
32+
}
33+
}
34+
35+
//# programmable --sender A --inputs @A
36+
//> 0: test::large_balance::create_holder();
37+
//> 1: test::large_balance::create_holder();
38+
//> TransferObjects([Result(0), Result(1)], Input(0))
39+
40+
//# run test::large_balance::send_large_balance --args object(2,0) @A 18446744073709551614 --sender A
41+
42+
//# create-checkpoint
43+
44+
//# run test::large_balance::send_large_balance --args object(2,1) @A 18446744073709551614 --sender A
45+
46+
//# create-checkpoint
47+
48+
//# programmable --sender A --inputs withdraw<sui::balance::Balance<test::large_balance::MARKER>>(18446744073709551614) withdraw<sui::balance::Balance<test::large_balance::MARKER>>(18446744073709551614) @B
49+
//> 0: sui::balance::redeem_funds<test::large_balance::MARKER>(Input(0));
50+
//> 1: sui::balance::redeem_funds<test::large_balance::MARKER>(Input(1));
51+
//> 2: sui::balance::join<test::large_balance::MARKER>(Result(0), Result(1));
52+
//> 3: sui::balance::send_funds<test::large_balance::MARKER>(Result(0), Input(2));

‎crates/sui-adapter-transactional-tests/tests/address_balances/large_balance_transfer.snap‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
---
2+
source: external-crates/move/crates/move-transactional-test-runner/src/framework.rs
3+
assertion_line: 817
4+
---
5+
processed 8 tasks
6+
7+
init:
8+
A: object(0,0), B: object(0,1)
9+
10+
task 1, lines 11-33:
11+
//# publish --sender A
12+
created: object(1,0)
13+
mutated: object(0,0)
14+
gas summary: computation_cost: 1000000, storage_cost: 7311200, storage_rebate: 0, non_refundable_storage_fee: 0
15+
16+
task 2, lines 35-38:
17+
//# programmable --sender A --inputs @A
18+
//> 0: test::large_balance::create_holder();
19+
//> 1: test::large_balance::create_holder();
20+
//> TransferObjects([Result(0), Result(1)], Input(0))
21+
created: object(2,0), object(2,1)
22+
mutated: object(0,0)
23+
gas summary: computation_cost: 1000000, storage_cost: 3876000, storage_rebate: 978120, non_refundable_storage_fee: 9880
24+
25+
task 3, line 40:
26+
//# run test::large_balance::send_large_balance --args object(2,0) @A 18446744073709551614 --sender A
27+
mutated: object(0,0), object(2,0)
28+
unchanged_shared: 0x0000000000000000000000000000000000000000000000000000000000000403
29+
accumulators_written: (object(3,0), A, sui::balance::Balance<test::large_balance::MARKER>, Merge)
30+
gas summary: computation_cost: 1000000, storage_cost: 2432000, storage_rebate: 2407680, non_refundable_storage_fee: 24320
31+
32+
task 4, line 42:
33+
//# create-checkpoint
34+
Checkpoint created: 1
35+
36+
task 5, line 44:
37+
//# run test::large_balance::send_large_balance --args object(2,1) @A 18446744073709551614 --sender A
38+
mutated: object(0,0), object(2,1)
39+
unchanged_shared: 0x0000000000000000000000000000000000000000000000000000000000000403
40+
accumulators_written: (object(3,0), A, sui::balance::Balance<test::large_balance::MARKER>, Merge)
41+
gas summary: computation_cost: 1000000, storage_cost: 2432000, storage_rebate: 2407680, non_refundable_storage_fee: 24320
42+
43+
task 6, line 46:
44+
//# create-checkpoint
45+
Checkpoint created: 2
46+
47+
task 7, lines 48-52:
48+
//# programmable --sender A --inputs withdraw<sui::balance::Balance<test::large_balance::MARKER>>(18446744073709551614) withdraw<sui::balance::Balance<test::large_balance::MARKER>>(18446744073709551614) @B
49+
//> 0: sui::balance::redeem_funds<test::large_balance::MARKER>(Input(0));
50+
//> 1: sui::balance::redeem_funds<test::large_balance::MARKER>(Input(1));
51+
//> 2: sui::balance::join<test::large_balance::MARKER>(Result(0), Result(1));
52+
//> 3: sui::balance::send_funds<test::large_balance::MARKER>(Result(0), Input(2));
53+
Error: Transaction Effects Status: Move Primitive Runtime Error. Location: sui::funds_accumulator::withdraw_from_accumulator_address (function index 9) at offset 0. Arithmetic error, stack overflow, max value depth, etc.
54+
Debug of error: MovePrimitiveRuntimeError(MoveLocationOpt(Some(MoveLocation { module: ModuleId { address: sui, name: Identifier("funds_accumulator") }, function: 9, instruction: 0, function_name: Some("withdraw_from_accumulator_address") }))) at command Some(1)

‎crates/sui-adapter-transactional-tests/tests/address_balances/[email protected]‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
---
2+
source: external-crates/move/crates/move-transactional-test-runner/src/framework.rs
3+
assertion_line: 811
4+
---
5+
processed 8 tasks
6+
7+
init:
8+
A: object(0,0), B: object(0,1)
9+
10+
task 1, lines 11-33:
11+
//# publish --sender A
12+
created: object(1,0)
13+
mutated: object(0,0)
14+
gas summary: computation_cost: 1000000, storage_cost: 7311200, storage_rebate: 0, non_refundable_storage_fee: 0
15+
16+
task 2, lines 35-38:
17+
//# programmable --sender A --inputs @A
18+
//> 0: test::large_balance::create_holder();
19+
//> 1: test::large_balance::create_holder();
20+
//> TransferObjects([Result(0), Result(1)], Input(0))
21+
created: object(2,0), object(2,1)
22+
mutated: object(0,0)
23+
gas summary: computation_cost: 1000000, storage_cost: 3876000, storage_rebate: 978120, non_refundable_storage_fee: 9880
24+
25+
task 3, line 40:
26+
//# run test::large_balance::send_large_balance --args object(2,0) @A 18446744073709551614 --sender A
27+
mutated: object(0,0), object(2,0)
28+
unchanged_shared: 0x0000000000000000000000000000000000000000000000000000000000000403
29+
accumulators_written: (object(3,0), A, sui::balance::Balance<test::large_balance::MARKER>, Merge)
30+
gas summary: computation_cost: 1000000, storage_cost: 2432000, storage_rebate: 2407680, non_refundable_storage_fee: 24320
31+
32+
task 4, line 42:
33+
//# create-checkpoint
34+
Checkpoint created: 1
35+
36+
task 5, line 44:
37+
//# run test::large_balance::send_large_balance --args object(2,1) @A 18446744073709551614 --sender A
38+
mutated: object(0,0), object(2,1)
39+
unchanged_shared: 0x0000000000000000000000000000000000000000000000000000000000000403
40+
accumulators_written: (object(3,0), A, sui::balance::Balance<test::large_balance::MARKER>, Merge)
41+
gas summary: computation_cost: 1000000, storage_cost: 2432000, storage_rebate: 2407680, non_refundable_storage_fee: 24320
42+
43+
task 6, line 46:
44+
//# create-checkpoint
45+
Checkpoint created: 2
46+
47+
task 7, lines 48-52:
48+
//# programmable --sender A --inputs withdraw<sui::balance::Balance<test::large_balance::MARKER>>(18446744073709551614) withdraw<sui::balance::Balance<test::large_balance::MARKER>>(18446744073709551614) @B
49+
//> 0: sui::balance::redeem_funds<test::large_balance::MARKER>(Input(0));
50+
//> 1: sui::balance::redeem_funds<test::large_balance::MARKER>(Input(1));
51+
//> 2: sui::balance::join<test::large_balance::MARKER>(Result(0), Result(1));
52+
//> 3: sui::balance::send_funds<test::large_balance::MARKER>(Result(0), Input(2));
53+
Error: Transaction Effects Status: Move Primitive Runtime Error. Location: sui::funds_accumulator::withdraw_from_accumulator_address (function index 9) at offset 0. Arithmetic error, stack overflow, max value depth, etc.
54+
Debug of error: MovePrimitiveRuntimeError(MoveLocationOpt(Some(MoveLocation { module: ModuleId { address: sui, name: Identifier("funds_accumulator") }, function: 9, instruction: 0, function_name: Some("withdraw_from_accumulator_address") }))) at command Some(1)

‎crates/sui-json-rpc-types/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ serde_json.workspace = true
1515
serde_with.workspace = true
1616
colored.workspace = true
1717
itertools.workspace = true
18+
nonempty.workspace = true
1819
tracing.workspace = true
1920
bcs.workspace = true
2021
sui-protocol-config.workspace = true

‎crates/sui-json-rpc-types/src/sui_transaction.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ use move_core_types::annotated_value::MoveTypeLayout;
2020
use move_core_types::identifier::{IdentStr, Identifier};
2121
use move_core_types::language_storage::{ModuleId, StructTag, TypeTag};
2222
use mysten_metrics::monitored_scope;
23+
use nonempty::NonEmpty;
2324
use sui_json::{SuiJsonValue, primitive_type};
2425
use sui_types::SUI_FRAMEWORK_ADDRESS;
2526
use sui_types::accumulator_event::AccumulatorEvent;
@@ -822,7 +823,8 @@ impl From<AccumulatorOperation> for SuiAccumulatorOperation {
822823
pub enum SuiAccumulatorValue {
823824
Integer(u64),
824825
IntegerTuple(u64, u64),
825-
EventDigest(Vec<(u64 /* event index in the transaction */, Digest)>),
826+
#[schemars(with = "Vec<(u64, Digest)>")]
827+
EventDigest(NonEmpty<(u64 /* event index in the transaction */, Digest)>),
826828
}
827829

828830
impl From<AccumulatorValue> for SuiAccumulatorValue {

‎crates/sui-types/src/effects/object_change.rs‎

Lines changed: 25 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,12 @@ use crate::{
77
object::{Object, Owner},
88
};
99
use move_core_types::language_storage::TypeTag;
10+
use nonempty::NonEmpty;
1011
use serde::{Deserialize, Serialize};
1112

13+
#[cfg(test)]
14+
use nonempty::nonempty;
15+
1216
use super::IDOperation;
1317

1418
#[derive(Eq, PartialEq, Clone, Debug, Serialize, Deserialize)]
@@ -87,7 +91,7 @@ pub enum AccumulatorOperation {
8791
pub enum AccumulatorValue {
8892
Integer(u64),
8993
IntegerTuple(u64, u64),
90-
EventDigest(Vec<(u64 /* event index in the transaction */, Digest)>),
94+
EventDigest(NonEmpty<(u64 /* event index in the transaction */, Digest)>),
9195
}
9296

9397
/// Accumulator objects are named by an address (can be an account address or a UID)
@@ -149,34 +153,36 @@ impl AccumulatorWriteV1 {
149153
AccumulatorOperation::Split => (merge, split + v as u128),
150154
}
151155
} else {
152-
mysten_common::debug_fatal!(
156+
mysten_common::fatal!(
153157
"mismatched accumulator value types for same object"
154158
);
155-
(merge, split)
156159
}
157160
});
158161
let (amount, operation) = if merge_amount >= split_amount {
159162
(merge_amount - split_amount, AccumulatorOperation::Merge)
160163
} else {
161164
(split_amount - merge_amount, AccumulatorOperation::Split)
162165
};
163-
let amount_u64 = amount
164-
.try_into()
165-
.expect("accumulator value overflow: merged amount exceeds u64::MAX");
166-
(AccumulatorValue::Integer(amount_u64), operation)
166+
match amount.try_into() {
167+
Ok(amount_u64) => (AccumulatorValue::Integer(amount_u64), operation),
168+
Err(_) => {
169+
mysten_common::fatal!(
170+
"accumulator value overflow: merged amount {} exceeds u64::MAX",
171+
amount
172+
);
173+
}
174+
}
167175
}
168176
AccumulatorValue::IntegerTuple(_, _) => {
169177
todo!("IntegerTuple netting-out logic not yet implemented")
170178
}
171-
AccumulatorValue::EventDigest(_) => {
172-
let mut event_digests = Vec::new();
173-
for write in writes {
174-
if let AccumulatorValue::EventDigest(digests) = write.value {
175-
event_digests.extend(digests);
179+
AccumulatorValue::EventDigest(first_digests) => {
180+
let mut event_digests = first_digests.clone();
181+
for write in &writes[1..] {
182+
if let AccumulatorValue::EventDigest(digests) = &write.value {
183+
event_digests.extend(digests.iter().copied());
176184
} else {
177-
mysten_common::debug_fatal!(
178-
"mismatched accumulator value types for same object"
179-
);
185+
mysten_common::fatal!("mismatched accumulator value types for same object");
180186
}
181187
}
182188
(
@@ -365,7 +371,7 @@ mod tests {
365371
let write = AccumulatorWriteV1 {
366372
address: addr.clone(),
367373
operation: AccumulatorOperation::Merge,
368-
value: AccumulatorValue::EventDigest(vec![(0, digest1), (1, digest2)]),
374+
value: AccumulatorValue::EventDigest(nonempty![(0, digest1), (1, digest2)]),
369375
};
370376

371377
let result = AccumulatorWriteV1::merge(vec![write.clone()]);
@@ -384,17 +390,17 @@ mod tests {
384390
AccumulatorWriteV1 {
385391
address: addr.clone(),
386392
operation: AccumulatorOperation::Merge,
387-
value: AccumulatorValue::EventDigest(vec![(0, digest1), (1, digest2)]),
393+
value: AccumulatorValue::EventDigest(nonempty![(0, digest1), (1, digest2)]),
388394
},
389395
AccumulatorWriteV1 {
390396
address: addr.clone(),
391397
operation: AccumulatorOperation::Merge,
392-
value: AccumulatorValue::EventDigest(vec![(2, digest3)]),
398+
value: AccumulatorValue::EventDigest(nonempty![(2, digest3)]),
393399
},
394400
AccumulatorWriteV1 {
395401
address: addr.clone(),
396402
operation: AccumulatorOperation::Merge,
397-
value: AccumulatorValue::EventDigest(vec![(3, digest4)]),
403+
value: AccumulatorValue::EventDigest(nonempty![(3, digest4)]),
398404
},
399405
];
400406

@@ -411,31 +417,6 @@ mod tests {
411417
}
412418
}
413419

414-
#[test]
415-
fn test_merge_event_digests_empty_list() {
416-
let addr = test_accumulator_address();
417-
let writes = vec![
418-
AccumulatorWriteV1 {
419-
address: addr.clone(),
420-
operation: AccumulatorOperation::Merge,
421-
value: AccumulatorValue::EventDigest(vec![]),
422-
},
423-
AccumulatorWriteV1 {
424-
address: addr.clone(),
425-
operation: AccumulatorOperation::Merge,
426-
value: AccumulatorValue::EventDigest(vec![]),
427-
},
428-
];
429-
430-
let result = AccumulatorWriteV1::merge(writes);
431-
assert_eq!(result.operation, AccumulatorOperation::Merge);
432-
if let AccumulatorValue::EventDigest(digests) = result.value {
433-
assert_eq!(digests.len(), 0);
434-
} else {
435-
panic!("Expected EventDigest value");
436-
}
437-
}
438-
439420
#[test]
440421
#[should_panic(expected = "All writes must have the same accumulator address")]
441422
fn test_merge_mismatched_addresses() {

‎sui-execution/latest/sui-adapter/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ move-trace-format.workspace = true
2727
move-vm-config.workspace = true
2828
mysten-common.workspace = true
2929
mysten-metrics.workspace = true
30+
nonempty.workspace = true
3031

3132
move-bytecode-verifier = { path = "../../../external-crates/move/crates/move-bytecode-verifier" }
3233
move-vm-runtime = { path = "../../../external-crates/move/crates/move-vm-runtime" }

‎sui-execution/latest/sui-adapter/src/programmable_transactions/context.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ mod checked {
4545
};
4646
use move_vm_types::loaded_data::runtime_types::Type;
4747
use mysten_common::debug_fatal;
48+
use nonempty::nonempty;
4849
use std::{
4950
borrow::Borrow,
5051
cell::RefCell,
@@ -1551,7 +1552,7 @@ mod checked {
15511552
);
15521553
};
15531554
let digest = event.digest();
1554-
AccumulatorValue::EventDigest(vec![(event_idx, digest)])
1555+
AccumulatorValue::EventDigest(nonempty![(event_idx, digest)])
15551556
}
15561557
};
15571558

0 commit comments

Comments
 (0)