Skip to content

Commit fa1b372

Browse files
oleganzaoleganza
authored
Merge pull request #16 from Skydev0h/skydev/del-ext-safeguard
Added safeguard against deleting last extension with disallowed pubkey, fixed ext operation prefix clashing bug and adjusted code style
2 parents 53cf00c + 51358ad commit fa1b372

File tree

4 files changed

+73
-22
lines changed

4 files changed

+73
-22
lines changed

‎contracts/wallet_v5.fc‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -71,20 +71,21 @@ cell verify_actions(cell c5) inline {
7171
() dispatch_complex_request(slice cs) impure inline_ref {
7272

7373
;; Recurse into extended actions until we reach standard actions
74-
while (cs~load_uint(1)) {
75-
var is_add_ext = cs~check_and_remove_add_extension_prefix();
76-
var is_del_ext = cs~check_and_remove_remove_extension_prefix();
74+
while (cs~load_int(1)) {
75+
var is_add_ext? = cs~check_and_remove_add_extension_prefix();
76+
var is_del_ext? = is_add_ext? ? 0 : cs~check_and_remove_remove_extension_prefix();
7777
;; Add/remove extensions
78-
if ((is_add_ext) | (is_del_ext)) {
78+
if (is_add_ext? | is_del_ext?) {
7979
(int wc, int hash) = parse_std_addr(cs~load_msg_addr());
8080
int packed_addr = pack_address((wc, hash) );
8181

8282
var ds = get_data().begin_parse();
8383
var data_bits = ds~load_bits(size::stored_seqno + size::stored_subwallet + size::public_key);
84+
var stored_seqno = data_bits.preload_int(size::stored_seqno);
8485
var extensions = ds.preload_dict();
8586

8687
;; Add extension
87-
if (is_add_ext) {
88+
if (is_add_ext?) {
8889
(extensions, int success?) = extensions.udict_add_builder?(256, packed_addr, begin_cell().store_int(wc,8));
8990
throw_unless(39, success?);
9091
} else
@@ -93,6 +94,7 @@ cell verify_actions(cell c5) inline {
9394
{
9495
(extensions, int success?) = extensions.udict_delete?(256, packed_addr);
9596
throw_unless(40, success?);
97+
throw_if(44, null?(extensions) & (stored_seqno < 0));
9698
}
9799

98100
set_data(begin_cell()
@@ -101,11 +103,11 @@ cell verify_actions(cell c5) inline {
101103
.end_cell());
102104
}
103105
elseif (cs~check_and_remove_set_signature_auth_allowed_prefix()) {
104-
var allow = cs~load_uint(1);
106+
var allow? = cs~load_int(1);
105107
var ds = get_data().begin_parse();
106108
var stored_seqno = ds~load_int(size::stored_seqno);
107109
var immutable_tail = ds; ;; stored_subwallet ~ public_key ~ extensions
108-
if (allow) {
110+
if (allow?) {
109111
;; allow
110112
throw_unless(43, stored_seqno < 0);
111113
;; Can't be disallowed with 0 because disallowing increments seqno
@@ -287,10 +289,10 @@ cell verify_actions(cell c5) inline {
287289
;; - 0x6578746E "extn" authenticated by extension
288290
;; - 0x73696E74 "sint" internal message authenticated by signature
289291

290-
(body, int is_extn) = check_and_remove_extn_prefix(body); ;; 0x6578746E ("extn")
292+
(body, int is_extn?) = check_and_remove_extn_prefix(body); ;; 0x6578746E ("extn")
291293

292294
;; IFJMPREF because unconditionally returns inside
293-
if (is_extn) { ;; "extn" authenticated by extension
295+
if (is_extn?) { ;; "extn" authenticated by extension
294296

295297
;; Authenticate extension by its address.
296298
int packed_sender_addr = pack_address(parse_std_addr(full_msg_slice~load_msg_addr())); ;; no PLDMSGADDR exists
@@ -310,8 +312,8 @@ cell verify_actions(cell c5) inline {
310312
}
311313

312314
slice full_body = body;
313-
(_, int is_sint) = check_and_remove_sint_prefix(body); ;; 0x73696E74 ("sint") - sign internal
314-
return_unless(is_sint);
315+
(_, int is_sint?) = check_and_remove_sint_prefix(body); ;; 0x73696E74 ("sint") - sign internal
316+
return_unless(is_sint?);
315317

316318
;; Process the rest of the slice just like the signed request.
317319
process_signed_request_from_internal_message(full_body);

‎tests/wallet-v5-extensions.spec.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -372,8 +372,8 @@ describe('Wallet V5 extensions auth', () => {
372372
const receipt = await walletV5.sendInternalMessageFromExtension(sender, {
373373
value: toNano('0.1'),
374374
body: packActionsList([
375-
new ActionRemoveExtension(sender.address!),
376-
new ActionSetSignatureAuthAllowed(true)
375+
new ActionSetSignatureAuthAllowed(true),
376+
new ActionRemoveExtension(sender.address!)
377377
])
378378
});
379379

@@ -461,8 +461,8 @@ describe('Wallet V5 extensions auth', () => {
461461
const receipt = await walletV5.sendInternalMessageFromExtension(sender, {
462462
value: toNano('0.1'),
463463
body: packActionsList([
464-
new ActionRemoveExtension(sender.address!),
465-
new ActionSetSignatureAuthAllowed(true)
464+
new ActionSetSignatureAuthAllowed(true),
465+
new ActionRemoveExtension(sender.address!)
466466
])
467467
});
468468

‎tests/wallet-v5-external.spec.ts‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -800,15 +800,14 @@ describe('Wallet V5 sign auth external', () => {
800800
expect(contract_seqno).toEqual(seqno + 1);
801801
});
802802

803-
it('Should add ext, disallow sign, remove ext, allow sign in one tx; send in other', async () => {
804-
// N.B. Test that zero extensions do not prevent re-allowing the signature authentication
803+
it('Should add ext, disallow sign, allow sign, remove ext in one tx; send in other', async () => {
805804
const testExtension = Address.parse('EQAvDfWFG0oYX19jwNDNBBL1rKNT9XfaGP9HyTb5nb2Eml6y');
806805

807806
const actionsList = packActionsList([
808807
new ActionAddExtension(testExtension),
809808
new ActionSetSignatureAuthAllowed(false),
810-
new ActionRemoveExtension(testExtension),
811-
new ActionSetSignatureAuthAllowed(true)
809+
new ActionSetSignatureAuthAllowed(true),
810+
new ActionRemoveExtension(testExtension)
812811
]);
813812
const receipt = await walletV5.sendExternalSignedMessage(createBody(actionsList));
814813
accountForGas(receipt.transactions);
@@ -856,6 +855,28 @@ describe('Wallet V5 sign auth external', () => {
856855
expect(receiverBalanceAfter).toEqual(receiverBalanceBefore + forwardValue - fee);
857856
});
858857

858+
it('Should fail removing last extension with signature auth disabled', async () => {
859+
const testExtension = Address.parse('EQAvDfWFG0oYX19jwNDNBBL1rKNT9XfaGP9HyTb5nb2Eml6y');
860+
861+
const actionsList = packActionsList([
862+
new ActionAddExtension(testExtension),
863+
new ActionSetSignatureAuthAllowed(false),
864+
new ActionRemoveExtension(testExtension)
865+
]);
866+
const receipt = await walletV5.sendExternalSignedMessage(createBody(actionsList));
867+
accountForGas(receipt.transactions);
868+
869+
expect(
870+
(
871+
(receipt.transactions[0].description as TransactionDescriptionGeneric)
872+
.computePhase as TransactionComputeVm
873+
).exitCode
874+
).toEqual(44);
875+
876+
const isSignatureAuthAllowed = await walletV5.getIsSignatureAuthAllowed();
877+
expect(isSignatureAuthAllowed).toEqual(-1);
878+
});
879+
859880
it('Should fail disallowing signature auth twice in tx', async () => {
860881
const testExtension = Address.parse('EQAvDfWFG0oYX19jwNDNBBL1rKNT9XfaGP9HyTb5nb2Eml6y');
861882

‎tests/wallet-v5-internal.spec.ts‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1011,15 +1011,14 @@ describe('Wallet V5 sign auth internal', () => {
10111011
expect(contract_seqno).toEqual(seqno + 1);
10121012
});
10131013

1014-
it('Should add ext, disallow sign, remove ext, allow sign in one tx; send in other', async () => {
1015-
// N.B. Test that zero extensions do not prevent re-allowing the signature authentication
1014+
it('Should add ext, disallow sign, allow sign, remove ext in one tx; send in other', async () => {
10161015
const testExtension = Address.parse('EQAvDfWFG0oYX19jwNDNBBL1rKNT9XfaGP9HyTb5nb2Eml6y');
10171016

10181017
const actionsList = packActionsList([
10191018
new ActionAddExtension(testExtension),
10201019
new ActionSetSignatureAuthAllowed(false),
1020+
new ActionSetSignatureAuthAllowed(true),
10211021
new ActionRemoveExtension(testExtension),
1022-
new ActionSetSignatureAuthAllowed(true)
10231022
]);
10241023

10251024
const receipt = await walletV5.sendInternal(sender, {
@@ -1078,6 +1077,35 @@ describe('Wallet V5 sign auth internal', () => {
10781077
expect(receiverBalanceAfter).toEqual(receiverBalanceBefore + forwardValue - fee);
10791078
});
10801079

1080+
it('Should fail removing last extension with signature auth disabled', async () => {
1081+
const testExtension = Address.parse('EQAvDfWFG0oYX19jwNDNBBL1rKNT9XfaGP9HyTb5nb2Eml6y');
1082+
1083+
const actionsList = packActionsList([
1084+
new ActionAddExtension(testExtension),
1085+
new ActionSetSignatureAuthAllowed(false),
1086+
new ActionRemoveExtension(testExtension)
1087+
]);
1088+
1089+
const receipt = await walletV5.sendInternal(sender, {
1090+
sendMode: SendMode.PAY_GAS_SEPARATELY,
1091+
value: toNano(0.1),
1092+
body: createBody(actionsList)
1093+
});
1094+
1095+
expect(receipt.transactions.length).toEqual(2);
1096+
accountForGas(receipt.transactions);
1097+
1098+
expect(
1099+
(
1100+
(receipt.transactions[1].description as TransactionDescriptionGeneric)
1101+
.computePhase as TransactionComputeVm
1102+
).exitCode
1103+
).toEqual(44);
1104+
1105+
const isSignatureAuthAllowed = await walletV5.getIsSignatureAuthAllowed();
1106+
expect(isSignatureAuthAllowed).toEqual(-1);
1107+
});
1108+
10811109
it('Should fail disallowing signature auth twice in tx', async () => {
10821110
const testExtension = Address.parse('EQAvDfWFG0oYX19jwNDNBBL1rKNT9XfaGP9HyTb5nb2Eml6y');
10831111

0 commit comments

Comments
 (0)