From e5004eba640af7555ccc63ce40fdd1aeeee9b18b Mon Sep 17 00:00:00 2001 From: John Saigle Date: Fri, 22 Mar 2024 15:06:51 -0400 Subject: [PATCH 1/5] solana: Fix logic error in Inbox release process There are a few suspicious aspects of the inbox release logic: 1. `release_inbound_unlock` takes a flag that determines whether a `false` result from `try_release` should cause an error. The flag indicates that it should revert on delay. However, `try_release` can also return `false` when a transaction has not been approved. If the flag is set, the transaction will not return an error when a user tries to release a transaction that is not approved even though whether a transaction is approved has nothing to do with its expected behaviour when it is delayed 2. The custom error TransferNotApproved is defined but unused Taken together, I believe `try_release()` should revert with the TransferNotApproved error. This would disambiguate the various 'false' results and provide more intuitive behaviour when a user tries to release a transaction that is not approved. --- .../src/queue/inbox.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/solana/programs/example-native-token-transfers/src/queue/inbox.rs b/solana/programs/example-native-token-transfers/src/queue/inbox.rs index e38546c32..2218dc956 100644 --- a/solana/programs/example-native-token-transfers/src/queue/inbox.rs +++ b/solana/programs/example-native-token-transfers/src/queue/inbox.rs @@ -35,12 +35,23 @@ impl InboxItem { pub const SEED_PREFIX: &'static [u8] = b"inbox_item"; /// Attempt to release the transfer. - /// Returns true if the transfer was released, false if it was not yet time to release it. + /// If the inbox item status is [`ReleaseStatus::ReleaseAfter`], this function returns true if the current timestamp + /// is newer than the one stored in the release status. If the timestamp is in the future, + /// returns false. + /// + /// # Errors + /// + /// Returns errors when the item cannot be released, i.e. when its status is not + /// `ReleaseAfter`: + /// - returns [`NTTError::TransferNotApproved`] if [`ReleaseStatus::NotApproved`] + /// - returns [`NTTError::TransferAlreadyRedeemed`] if [`ReleaseStatus::Released`]. This is + /// important to prevent a single transfer from being redeemed multiple times, which would + /// result in minting arbitrary amounts of the token. pub fn try_release(&mut self) -> Result { let now = current_timestamp(); match self.release_status { - ReleaseStatus::NotApproved => Ok(false), + ReleaseStatus::NotApproved => Err(NTTError::TransferNotApproved.into()), ReleaseStatus::ReleaseAfter(release_timestamp) => { if release_timestamp > now { return Ok(false); From 86f5a74f87e4e5309aa24e756a44c97721ec34d1 Mon Sep 17 00:00:00 2001 From: John Saigle Date: Fri, 22 Mar 2024 17:48:11 -0400 Subject: [PATCH 2/5] solana: change comment to revert_on_delay --- .../src/instructions/release_inbound.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs b/solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs index 4d4d3d03e..dd5182d7e 100644 --- a/solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs +++ b/solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs @@ -55,8 +55,8 @@ pub struct ReleaseInboundMint<'info> { } /// Release an inbound transfer and mint the tokens to the recipient. -/// When `revert_on_error` is true, the transaction will revert if the -/// release timestamp has not been reached. When `revert_on_error` is false, the +/// When `revert_on_delay` is true, the transaction will revert if the +/// release timestamp has not been reached. When `revert_on_delay` is false, the /// transaction succeeds, but the minting is not performed. /// Setting this flag to `false` is useful when bundling this instruction /// together with [`crate::instructions::redeem`] in a transaction, so that the minting @@ -113,8 +113,8 @@ pub struct ReleaseInboundUnlock<'info> { } /// Release an inbound transfer and unlock the tokens to the recipient. -/// When `revert_on_error` is true, the transaction will revert if the -/// release timestamp has not been reached. When `revert_on_error` is false, the +/// When `revert_on_delay` is true, the transaction will revert if the +/// release timestamp has not been reached. When `revert_on_delay` is false, the /// transaction succeeds, but the unlocking is not performed. /// Setting this flag to `false` is useful when bundling this instruction /// together with [`crate::instructions::redeem`], so that the unlocking From 5eea9b3b20b08179e4b5a780fe563818681e4754 Mon Sep 17 00:00:00 2001 From: John Saigle Date: Thu, 28 Mar 2024 11:12:40 -0400 Subject: [PATCH 3/5] solana: Let redeem code return error instead of try_release --- .../src/instructions/release_inbound.rs | 30 +++++++++++++------ .../src/queue/inbox.rs | 2 +- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs b/solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs index dd5182d7e..6054662e2 100644 --- a/solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs +++ b/solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs @@ -44,7 +44,7 @@ pub struct ReleaseInbound<'info> { #[derive(AnchorDeserialize, AnchorSerialize)] pub struct ReleaseInboundArgs { - pub revert_on_delay: bool, + pub revert_when_not_ready: bool, } // Burn/mint @@ -55,8 +55,8 @@ pub struct ReleaseInboundMint<'info> { } /// Release an inbound transfer and mint the tokens to the recipient. -/// When `revert_on_delay` is true, the transaction will revert if the -/// release timestamp has not been reached. When `revert_on_delay` is false, the +/// When `revert_when_not_ready` is true, the transaction will revert if the +/// release timestamp has not been reached. When `revert_when_not_ready` is false, the /// transaction succeeds, but the minting is not performed. /// Setting this flag to `false` is useful when bundling this instruction /// together with [`crate::instructions::redeem`] in a transaction, so that the minting @@ -70,8 +70,14 @@ pub fn release_inbound_mint( let released = inbox_item.try_release()?; if !released { - if args.revert_on_delay { - return Err(NTTError::CantReleaseYet.into()); + if args.revert_when_not_ready { + match inbox_item.release_status { + ReleaseStatus::NotApproved => return Err(NTTError::TransferNotApproved.into()), + ReleaseStatus::ReleaseAfter(_) => return Err(NTTError::CantReleaseYet.into()), + // Unreachable: if released, [`InboxItem::try_release`] will return an Error immediately + // rather than Ok(bool). + ReleaseStatus::Released => return Err(NTTError::TransferAlreadyRedeemed.into()), + } } else { return Ok(()); } @@ -113,8 +119,8 @@ pub struct ReleaseInboundUnlock<'info> { } /// Release an inbound transfer and unlock the tokens to the recipient. -/// When `revert_on_delay` is true, the transaction will revert if the -/// release timestamp has not been reached. When `revert_on_delay` is false, the +/// When `revert_when_not_ready` is true, the transaction will revert if the +/// release timestamp has not been reached. When `revert_when_not_ready` is false, the /// transaction succeeds, but the unlocking is not performed. /// Setting this flag to `false` is useful when bundling this instruction /// together with [`crate::instructions::redeem`], so that the unlocking @@ -128,8 +134,14 @@ pub fn release_inbound_unlock( let released = inbox_item.try_release()?; if !released { - if args.revert_on_delay { - return Err(NTTError::CantReleaseYet.into()); + if args.revert_when_not_ready { + match inbox_item.release_status { + ReleaseStatus::NotApproved => return Err(NTTError::TransferNotApproved.into()), + ReleaseStatus::ReleaseAfter(_) => return Err(NTTError::CantReleaseYet.into()), + // Unreachable: if released, [`InboxItem::try_release`] will return an Error immediately + // rather than Ok(bool). + ReleaseStatus::Released => return Err(NTTError::TransferAlreadyRedeemed.into()), + } } else { return Ok(()); } diff --git a/solana/programs/example-native-token-transfers/src/queue/inbox.rs b/solana/programs/example-native-token-transfers/src/queue/inbox.rs index 2218dc956..acdaa2e77 100644 --- a/solana/programs/example-native-token-transfers/src/queue/inbox.rs +++ b/solana/programs/example-native-token-transfers/src/queue/inbox.rs @@ -51,7 +51,7 @@ impl InboxItem { let now = current_timestamp(); match self.release_status { - ReleaseStatus::NotApproved => Err(NTTError::TransferNotApproved.into()), + ReleaseStatus::NotApproved => Ok(false), ReleaseStatus::ReleaseAfter(release_timestamp) => { if release_timestamp > now { return Ok(false); From ef961add483b7690722343412857c0c81ee7adc2 Mon Sep 17 00:00:00 2001 From: John Saigle Date: Tue, 2 Apr 2024 09:06:07 -0400 Subject: [PATCH 4/5] solana: update arg name in tests --- solana/ts/sdk/ntt.ts | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/solana/ts/sdk/ntt.ts b/solana/ts/sdk/ntt.ts index 0f1b32f48..99cc2b23c 100644 --- a/solana/ts/sdk/ntt.ts +++ b/solana/ts/sdk/ntt.ts @@ -477,7 +477,7 @@ export class NTT { payer: PublicKey chain: ChainName | ChainId nttMessage: NttMessage - revertOnDelay: boolean + revertWhenNotReady: boolean recipient?: PublicKey config?: Config }): Promise { @@ -494,7 +494,7 @@ export class NTT { return await this.program.methods .releaseInboundMint({ - revertOnDelay: args.revertOnDelay + revertWhenNotReady: args.revertWhenNotReady }) .accounts({ common: { @@ -513,7 +513,7 @@ export class NTT { payer: Keypair chain: ChainName | ChainId nttMessage: NttMessage - revertOnDelay: boolean + revertWhenNotReady: boolean config?: Config }): Promise { if (await this.isPaused()) { @@ -536,7 +536,7 @@ export class NTT { payer: PublicKey chain: ChainName | ChainId nttMessage: NttMessage - revertOnDelay: boolean + revertWhenNotReady: boolean recipient?: PublicKey config?: Config }): Promise { @@ -553,7 +553,7 @@ export class NTT { return await this.program.methods .releaseInboundUnlock({ - revertOnDelay: args.revertOnDelay + revertWhenNotReady: args.revertWhenNotReady }) .accounts({ common: { @@ -573,7 +573,7 @@ export class NTT { payer: Keypair chain: ChainName | ChainId nttMessage: NttMessage - revertOnDelay: boolean + revertWhenNotReady: boolean config?: Config }): Promise { if (await this.isPaused()) { @@ -742,7 +742,7 @@ export class NTT { return await this.program.methods.receiveWormholeMessage().accounts({ payer: args.payer, - config: { config: this.configAccountAddress() }, + config: { config: this.configAccountAddress() }, peer: transceiverPeer, vaa: derivePostedVaaKey(this.wormholeId, Buffer.from(wormholeNTT.hash)), transceiverMessage: this.transceiverMessageAccountAddress( @@ -825,7 +825,7 @@ export class NTT { // In case the redeemed amount exceeds the remaining inbound rate limit capacity, // the transaction gets delayed. If this happens, the second instruction will not actually // be able to release the transfer yet. - // To make sure the transaction still succeeds, we set revertOnDelay to false, which will + // To make sure the transaction still succeeds, we set revertWhenNotReady to false, which will // just make the second instruction a no-op in case the transfer is delayed. const tx = new Transaction() @@ -838,8 +838,7 @@ export class NTT { nttMessage, recipient: new PublicKey(nttMessage.payload.recipientAddress.toUint8Array()), chain: chainId, - revertOnDelay: false, - config: config + revertWhenNotReady: false } if (config.mode.locking != null) { From ed933ac73235307fb4864701b3bd62c31d34d67e Mon Sep 17 00:00:00 2001 From: John Saigle Date: Thu, 9 May 2024 10:43:50 -0400 Subject: [PATCH 5/5] solana: update IDL --- solana/idl/json/dummy_transfer_hook.json | 26 +--------- .../json/example_native_token_transfers.json | 2 +- solana/idl/ts/dummy_transfer_hook.ts | 48 ------------------- .../idl/ts/example_native_token_transfers.ts | 4 +- 4 files changed, 4 insertions(+), 76 deletions(-) diff --git a/solana/idl/json/dummy_transfer_hook.json b/solana/idl/json/dummy_transfer_hook.json index 586fb2b72..ba62b9b29 100644 --- a/solana/idl/json/dummy_transfer_hook.json +++ b/solana/idl/json/dummy_transfer_hook.json @@ -30,11 +30,6 @@ "isMut": false, "isSigner": false }, - { - "name": "counter", - "isMut": true, - "isSigner": false - }, { "name": "systemProgram", "isMut": false, @@ -78,11 +73,6 @@ "docs": [ "computes and the on-chain code correctly passes on the PDA." ] - }, - { - "name": "counter", - "isMut": true, - "isSigner": false } ], "args": [ @@ -93,21 +83,7 @@ ] } ], - "accounts": [ - { - "name": "Counter", - "type": { - "kind": "struct", - "fields": [ - { - "name": "count", - "type": "u64" - } - ] - } - } - ], "metadata": { - "address": "BgabMDLaxsyB7eGMBt9L22MSk9KMrL4zY2iNe14kyFP5" + "address": "7R368vaW4Ztt8ShPuBRaaCqSTumLCVMbc1na4ajR5y4G" } } \ No newline at end of file diff --git a/solana/idl/json/example_native_token_transfers.json b/solana/idl/json/example_native_token_transfers.json index 296557228..0bb20be29 100644 --- a/solana/idl/json/example_native_token_transfers.json +++ b/solana/idl/json/example_native_token_transfers.json @@ -1439,7 +1439,7 @@ "kind": "struct", "fields": [ { - "name": "revertOnDelay", + "name": "revertWhenNotReady", "type": "bool" } ] diff --git a/solana/idl/ts/dummy_transfer_hook.ts b/solana/idl/ts/dummy_transfer_hook.ts index 4e068844b..c7af89541 100644 --- a/solana/idl/ts/dummy_transfer_hook.ts +++ b/solana/idl/ts/dummy_transfer_hook.ts @@ -30,11 +30,6 @@ export type DummyTransferHook = { "isMut": false, "isSigner": false }, - { - "name": "counter", - "isMut": true, - "isSigner": false - }, { "name": "systemProgram", "isMut": false, @@ -78,11 +73,6 @@ export type DummyTransferHook = { "docs": [ "computes and the on-chain code correctly passes on the PDA." ] - }, - { - "name": "counter", - "isMut": true, - "isSigner": false } ], "args": [ @@ -92,20 +82,6 @@ export type DummyTransferHook = { } ] } - ], - "accounts": [ - { - "name": "counter", - "type": { - "kind": "struct", - "fields": [ - { - "name": "count", - "type": "u64" - } - ] - } - } ] }; @@ -141,11 +117,6 @@ export const IDL: DummyTransferHook = { "isMut": false, "isSigner": false }, - { - "name": "counter", - "isMut": true, - "isSigner": false - }, { "name": "systemProgram", "isMut": false, @@ -189,11 +160,6 @@ export const IDL: DummyTransferHook = { "docs": [ "computes and the on-chain code correctly passes on the PDA." ] - }, - { - "name": "counter", - "isMut": true, - "isSigner": false } ], "args": [ @@ -203,19 +169,5 @@ export const IDL: DummyTransferHook = { } ] } - ], - "accounts": [ - { - "name": "counter", - "type": { - "kind": "struct", - "fields": [ - { - "name": "count", - "type": "u64" - } - ] - } - } ] }; diff --git a/solana/idl/ts/example_native_token_transfers.ts b/solana/idl/ts/example_native_token_transfers.ts index fa85dfa73..b329f976d 100644 --- a/solana/idl/ts/example_native_token_transfers.ts +++ b/solana/idl/ts/example_native_token_transfers.ts @@ -1471,7 +1471,7 @@ export type ExampleNativeTokenTransfers = { "kind": "struct", "fields": [ { - "name": "revertOnDelay", + "name": "revertWhenNotReady", "type": "bool" } ] @@ -3353,7 +3353,7 @@ export const IDL: ExampleNativeTokenTransfers = { "kind": "struct", "fields": [ { - "name": "revertOnDelay", + "name": "revertWhenNotReady", "type": "bool" } ]