Skip to content

Commit 857c058

Browse files
authored
Fix panic safety in register_contract_with_source and register_stellar_asset_contract_v2 (#1803)
### What Reorder operations in `register_contract_with_source` and `register_stellar_asset_contract_v2` so that the auth manager is always restored before any unwrap that could panic. Use `try_invoke_contract` instead of `invoke_contract` for the `set_admin` call in `register_stellar_asset_contract_v2`. ### Why Both functions snapshot the auth manager, switch to recording mode, perform a fallible operation, then restore the original auth manager. If the operation panics (e.g. constructor argument mismatch), the restore is skipped and the auth manager is left permanently in recording mode — silently bypassing all require_auth enforcement. This is not an expected code path in general contract testing, just an additional defensive layer. Closes #1802 ### Known limitations None
1 parent 022d7c5 commit 857c058

File tree

3 files changed

+126
-11
lines changed

3 files changed

+126
-11
lines changed

soroban-sdk/src/env.rs

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1120,15 +1120,15 @@ impl Env {
11201120
self.env_impl
11211121
.switch_to_recording_auth_inherited_from_snapshot(&prev_auth_manager)
11221122
.unwrap();
1123-
self.invoke_contract::<()>(
1123+
let admin_result = self.try_invoke_contract::<(), Error>(
11241124
&token_id,
11251125
&soroban_sdk_macros::internal_symbol_short!("set_admin"),
11261126
(admin,).try_into_val(self).unwrap(),
11271127
);
11281128
self.env_impl.set_auth_manager(prev_auth_manager).unwrap();
1129+
admin_result.unwrap().unwrap();
11291130

11301131
let issuer = StellarAssetIssuer::new(self.clone(), issuer_id);
1131-
11321132
StellarAssetContract::new(token_id, issuer, asset)
11331133
}
11341134

@@ -1167,13 +1167,14 @@ impl Env {
11671167
executable: xdr::ContractExecutable,
11681168
constructor_args: Vec<Val>,
11691169
) -> Address {
1170+
let args_vec: std::vec::Vec<xdr::ScVal> =
1171+
constructor_args.iter().map(|v| v.into_val(self)).collect();
1172+
let constructor_args = args_vec.try_into().unwrap();
11701173
let prev_auth_manager = self.env_impl.snapshot_auth_manager().unwrap();
11711174
self.env_impl
11721175
.switch_to_recording_auth_inherited_from_snapshot(&prev_auth_manager)
11731176
.unwrap();
1174-
let args_vec: std::vec::Vec<xdr::ScVal> =
1175-
constructor_args.iter().map(|v| v.into_val(self)).collect();
1176-
let contract_id: Address = self
1177+
let create_result = self
11771178
.env_impl
11781179
.invoke_function(xdr::HostFunction::CreateContractV2(
11791180
xdr::CreateContractArgsV2 {
@@ -1186,16 +1187,13 @@ impl Env {
11861187
},
11871188
),
11881189
executable,
1189-
constructor_args: args_vec.try_into().unwrap(),
1190+
constructor_args,
11901191
},
1191-
))
1192-
.unwrap()
1193-
.try_into_val(self)
1194-
.unwrap();
1192+
));
11951193

11961194
self.env_impl.set_auth_manager(prev_auth_manager).unwrap();
11971195

1198-
contract_id
1196+
create_result.unwrap().try_into_val(self).unwrap()
11991197
}
12001198

12011199
/// Set authorizations and signatures in the environment which will be

soroban-sdk/src/tests/env.rs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,11 @@ enum ContractError {
4444
AnError = 1,
4545
}
4646

47+
mod constructor_contract {
48+
use crate as soroban_sdk;
49+
soroban_sdk::contractimport!(file = "../target/wasm32v1-none/release/test_constructor.wasm");
50+
}
51+
4752
#[test]
4853
fn default_and_from_snapshot_same_settings() {
4954
let env1 = Env::default();
@@ -326,3 +331,28 @@ fn test_try_as_contract_panic() {
326331
)))
327332
);
328333
}
334+
335+
#[test]
336+
fn test_register_restores_auth_before_panics() {
337+
let env = Env::default();
338+
339+
let address = env.register(Contract, ());
340+
let client = ContractClient::new(&env, &address);
341+
let user = Address::generate(&env);
342+
343+
let pre_register = client.try_need_auth(&user);
344+
assert!(pre_register.is_err());
345+
346+
// This is contrived to cause a panic inside register_contract_with_source.
347+
// Don't actually do things like this!
348+
let another_contract = env.register(Contract, ());
349+
let register_result = env.try_as_contract::<_, Error>(&another_contract, || {
350+
// This expects arguments for the constructor, but none are provided, so it will panic
351+
env.register(constructor_contract::WASM, ());
352+
});
353+
assert!(register_result.is_err());
354+
355+
let post_register = client.try_need_auth(&user);
356+
assert!(post_register.is_err());
357+
assert_eq!(pre_register, post_register);
358+
}
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
{
2+
"generators": {
3+
"address": 4,
4+
"nonce": 0,
5+
"mux_id": 0
6+
},
7+
"auth": [
8+
[],
9+
[],
10+
[],
11+
[],
12+
[]
13+
],
14+
"ledger": {
15+
"protocol_version": 25,
16+
"sequence_number": 0,
17+
"timestamp": 0,
18+
"network_id": "0000000000000000000000000000000000000000000000000000000000000000",
19+
"base_reserve": 0,
20+
"min_persistent_entry_ttl": 4096,
21+
"min_temp_entry_ttl": 16,
22+
"max_entry_ttl": 6312000,
23+
"ledger_entries": [
24+
{
25+
"entry": {
26+
"last_modified_ledger_seq": 0,
27+
"data": {
28+
"contract_data": {
29+
"ext": "v0",
30+
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
31+
"key": "ledger_key_contract_instance",
32+
"durability": "persistent",
33+
"val": {
34+
"contract_instance": {
35+
"executable": {
36+
"wasm": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
37+
},
38+
"storage": null
39+
}
40+
}
41+
}
42+
},
43+
"ext": "v0"
44+
},
45+
"live_until": 4095
46+
},
47+
{
48+
"entry": {
49+
"last_modified_ledger_seq": 0,
50+
"data": {
51+
"contract_data": {
52+
"ext": "v0",
53+
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHK3M",
54+
"key": "ledger_key_contract_instance",
55+
"durability": "persistent",
56+
"val": {
57+
"contract_instance": {
58+
"executable": {
59+
"wasm": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
60+
},
61+
"storage": null
62+
}
63+
}
64+
}
65+
},
66+
"ext": "v0"
67+
},
68+
"live_until": 4095
69+
},
70+
{
71+
"entry": {
72+
"last_modified_ledger_seq": 0,
73+
"data": {
74+
"contract_code": {
75+
"ext": "v0",
76+
"hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
77+
"code": ""
78+
}
79+
},
80+
"ext": "v0"
81+
},
82+
"live_until": 4095
83+
}
84+
]
85+
},
86+
"events": []
87+
}

0 commit comments

Comments
 (0)