Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added safeguard against deleting last extension with disallowed pubkey, fixed ext operation prefix clashing bug and adjusted code style #16

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Added safeguard against deleting last extension with disallowed pubkey, fixed ext operation prefix clashing bug and adjusted code style #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8661ce2
Added safeguard against deleting last ext with disabled key (need tests)
Skydev0h Feb 21, 2024
4458be0
Fix prefix clashing bug and adjust code style (reported by @behrang)
Skydev0h Feb 23, 2024
51358ad
Fixed tests and added cases for deleting last ext with disallowed pubkey
Skydev0h Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions contracts/wallet_v5.fc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,20 +71,21 @@ cell verify_actions(cell c5) inline {
() dispatch_complex_request(slice cs) impure inline_ref {

;; Recurse into extended actions until we reach standard actions
while (cs~load_uint(1)) {
var is_add_ext = cs~check_and_remove_add_extension_prefix();
var is_del_ext = cs~check_and_remove_remove_extension_prefix();
while (cs~load_int(1)) {
var is_add_ext? = cs~check_and_remove_add_extension_prefix();
var is_del_ext? = is_add_ext? ? 0 : cs~check_and_remove_remove_extension_prefix();
;; Add/remove extensions
if ((is_add_ext) | (is_del_ext)) {
if (is_add_ext? | is_del_ext?) {
(int wc, int hash) = parse_std_addr(cs~load_msg_addr());
int packed_addr = pack_address((wc, hash) );

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

;; Add extension
if (is_add_ext) {
if (is_add_ext?) {
(extensions, int success?) = extensions.udict_add_builder?(256, packed_addr, begin_cell().store_int(wc,8));
throw_unless(39, success?);
} else
Expand All @@ -93,6 +94,7 @@ cell verify_actions(cell c5) inline {
{
(extensions, int success?) = extensions.udict_delete?(256, packed_addr);
throw_unless(40, success?);
throw_if(44, null?(extensions) & (stored_seqno < 0));
}

set_data(begin_cell()
Expand All @@ -101,11 +103,11 @@ cell verify_actions(cell c5) inline {
.end_cell());
}
elseif (cs~check_and_remove_set_signature_auth_allowed_prefix()) {
var allow = cs~load_uint(1);
var allow? = cs~load_int(1);
var ds = get_data().begin_parse();
var stored_seqno = ds~load_int(size::stored_seqno);
var immutable_tail = ds; ;; stored_subwallet ~ public_key ~ extensions
if (allow) {
if (allow?) {
;; allow
throw_unless(43, stored_seqno < 0);
;; Can't be disallowed with 0 because disallowing increments seqno
Expand Down Expand Up @@ -287,10 +289,10 @@ cell verify_actions(cell c5) inline {
;; - 0x6578746E "extn" authenticated by extension
;; - 0x73696E74 "sint" internal message authenticated by signature

(body, int is_extn) = check_and_remove_extn_prefix(body); ;; 0x6578746E ("extn")
(body, int is_extn?) = check_and_remove_extn_prefix(body); ;; 0x6578746E ("extn")

;; IFJMPREF because unconditionally returns inside
if (is_extn) { ;; "extn" authenticated by extension
if (is_extn?) { ;; "extn" authenticated by extension

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

slice full_body = body;
(_, int is_sint) = check_and_remove_sint_prefix(body); ;; 0x73696E74 ("sint") - sign internal
return_unless(is_sint);
(_, int is_sint?) = check_and_remove_sint_prefix(body); ;; 0x73696E74 ("sint") - sign internal
return_unless(is_sint?);

;; Process the rest of the slice just like the signed request.
process_signed_request_from_internal_message(full_body);
Expand Down
8 changes: 4 additions & 4 deletions tests/wallet-v5-extensions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,8 +372,8 @@ describe('Wallet V5 extensions auth', () => {
const receipt = await walletV5.sendInternalMessageFromExtension(sender, {
value: toNano('0.1'),
body: packActionsList([
new ActionRemoveExtension(sender.address!),
new ActionSetSignatureAuthAllowed(true)
new ActionSetSignatureAuthAllowed(true),
new ActionRemoveExtension(sender.address!)
])
});

Expand Down Expand Up @@ -461,8 +461,8 @@ describe('Wallet V5 extensions auth', () => {
const receipt = await walletV5.sendInternalMessageFromExtension(sender, {
value: toNano('0.1'),
body: packActionsList([
new ActionRemoveExtension(sender.address!),
new ActionSetSignatureAuthAllowed(true)
new ActionSetSignatureAuthAllowed(true),
new ActionRemoveExtension(sender.address!)
])
});

Expand Down
29 changes: 25 additions & 4 deletions tests/wallet-v5-external.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -800,15 +800,14 @@ describe('Wallet V5 sign auth external', () => {
expect(contract_seqno).toEqual(seqno + 1);
});

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

const actionsList = packActionsList([
new ActionAddExtension(testExtension),
new ActionSetSignatureAuthAllowed(false),
new ActionRemoveExtension(testExtension),
new ActionSetSignatureAuthAllowed(true)
new ActionSetSignatureAuthAllowed(true),
new ActionRemoveExtension(testExtension)
]);
const receipt = await walletV5.sendExternalSignedMessage(createBody(actionsList));
accountForGas(receipt.transactions);
Expand Down Expand Up @@ -856,6 +855,28 @@ describe('Wallet V5 sign auth external', () => {
expect(receiverBalanceAfter).toEqual(receiverBalanceBefore + forwardValue - fee);
});

it('Should fail removing last extension with signature auth disabled', async () => {
const testExtension = Address.parse('EQAvDfWFG0oYX19jwNDNBBL1rKNT9XfaGP9HyTb5nb2Eml6y');

const actionsList = packActionsList([
new ActionAddExtension(testExtension),
new ActionSetSignatureAuthAllowed(false),
new ActionRemoveExtension(testExtension)
]);
const receipt = await walletV5.sendExternalSignedMessage(createBody(actionsList));
accountForGas(receipt.transactions);

expect(
(
(receipt.transactions[0].description as TransactionDescriptionGeneric)
.computePhase as TransactionComputeVm
).exitCode
).toEqual(44);

const isSignatureAuthAllowed = await walletV5.getIsSignatureAuthAllowed();
expect(isSignatureAuthAllowed).toEqual(-1);
});

it('Should fail disallowing signature auth twice in tx', async () => {
const testExtension = Address.parse('EQAvDfWFG0oYX19jwNDNBBL1rKNT9XfaGP9HyTb5nb2Eml6y');

Expand Down
34 changes: 31 additions & 3 deletions tests/wallet-v5-internal.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1011,15 +1011,14 @@ describe('Wallet V5 sign auth internal', () => {
expect(contract_seqno).toEqual(seqno + 1);
});

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

const actionsList = packActionsList([
new ActionAddExtension(testExtension),
new ActionSetSignatureAuthAllowed(false),
new ActionSetSignatureAuthAllowed(true),
new ActionRemoveExtension(testExtension),
new ActionSetSignatureAuthAllowed(true)
]);

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

it('Should fail removing last extension with signature auth disabled', async () => {
const testExtension = Address.parse('EQAvDfWFG0oYX19jwNDNBBL1rKNT9XfaGP9HyTb5nb2Eml6y');

const actionsList = packActionsList([
new ActionAddExtension(testExtension),
new ActionSetSignatureAuthAllowed(false),
new ActionRemoveExtension(testExtension)
]);

const receipt = await walletV5.sendInternal(sender, {
sendMode: SendMode.PAY_GAS_SEPARATELY,
value: toNano(0.1),
body: createBody(actionsList)
});

expect(receipt.transactions.length).toEqual(2);
accountForGas(receipt.transactions);

expect(
(
(receipt.transactions[1].description as TransactionDescriptionGeneric)
.computePhase as TransactionComputeVm
).exitCode
).toEqual(44);

const isSignatureAuthAllowed = await walletV5.getIsSignatureAuthAllowed();
expect(isSignatureAuthAllowed).toEqual(-1);
});

it('Should fail disallowing signature auth twice in tx', async () => {
const testExtension = Address.parse('EQAvDfWFG0oYX19jwNDNBBL1rKNT9XfaGP9HyTb5nb2Eml6y');

Expand Down
Loading