Fix errors found in the TBRv3 Solana Audit #105

feli-xlabs · 2025-01-02T16:22:33Z

Allow to create accounts by hand even if they're squatted.
Refactor the program ownership transfer so that we only need the new owner's signature when accepting the transfer.
Check both the chain ID and the peer address in case of an inbound transfer

…he peer address

… so that when the new owner accepts the transfer, there is no need for the previous owner's signature.

…nts)

nonergodic

Only comments/nits, nothing that's really important.

nonergodic · 2025-01-11T08:31:33Z

sdk/solana/tbrv3/token-bridge-relayer.ts

+    const ixs = [];
+
+    ixs.push(this.registerPeer(signer, chain, peerAddress));
+    if (config.maxGasDropoffMicroToken !== 0) {
+      ixs.push(this.updateMaxGasDropoff(signer, chain, config.maxGasDropoffMicroToken));
+    }
+    ixs.push(this.updateBaseFee(signer, chain, config.relayerFeeMicroUsd));
+    ixs.push(this.setPauseForOutboundTransfers(signer, chain, config.pausedOutboundTransfers));
+
+    return Promise.all(ixs);


Suggested change

const ixs = [];

ixs.push(this.registerPeer(signer, chain, peerAddress));

if (config.maxGasDropoffMicroToken !== 0) {

ixs.push(this.updateMaxGasDropoff(signer, chain, config.maxGasDropoffMicroToken));

}

ixs.push(this.updateBaseFee(signer, chain, config.relayerFeeMicroUsd));

ixs.push(this.setPauseForOutboundTransfers(signer, chain, config.pausedOutboundTransfers));

return Promise.all(ixs);

return Promise.all([

this.registerPeer(signer, chain, peerAddress),

...(config.maxGasDropoffMicroToken !== 0

? [this.updateMaxGasDropoff(signer, chain, config.maxGasDropoffMicroToken))]

: []

),

this.updateBaseFee(signer, chain, config.relayerFeeMicroUsd),

this.setPauseForOutboundTransfers(signer, chain, config.pausedOutboundTransfers)

]);

nit/style suggestion originally mentioned on slack

nonergodic · 2025-01-11T08:36:15Z

solana/programs/token-bridge-relayer/src/processor/inbound.rs

@@ -144,6 +144,8 @@ pub fn complete_transfer(
    } = *ctx.accounts.vaa.message().data();
    let gas_dropoff_amount = denormalize_dropoff_to_lamports(gas_dropoff_amount);

+    let _x = ctx.accounts.vaa.message();


this seems like a leftover

nonergodic · 2025-01-11T08:37:59Z

solana/programs/token-bridge-relayer/src/processor/outbound.rs

@@ -116,7 +116,7 @@ pub struct OutboundTransfer<'info> {
    /// CHECK: Token Bridge emitter.
    pub token_bridge_emitter: UncheckedAccount<'info>,

-    /// CHECK: Token Bridge sequence.
+    /// CHECK: Token Bridge sequence. Mutable.


Seems redundant and also not in line with e.g. the fee_recipient comment above, which is also mutable but isn't commented as such either.

nonergodic · 2025-01-11T08:42:27Z

solana/programs/token-bridge-relayer/src/utils.rs

+    let total_fees_mwei = (|| {
+        let evm_transaction_fee_mwei = config
+            .evm_transaction_gas
+            .checked_mul(u64::from(oracle_evm_prices.gas_price))?;
+        let evm_tx_size_fee_mwei = config
+            .evm_transaction_size
+            .checked_mul(u64::from(oracle_evm_prices.price_per_byte))?;
+        let dropoff_mwei = u64::from(dropoff_amount_micro).checked_mul(MWEI_PER_MICRO_ETH)?;
+
+        evm_transaction_fee_mwei
+            .checked_add(evm_tx_size_fee_mwei)?
+            .checked_add(dropoff_mwei)
+    })()
+    .ok_or(TokenBridgeRelayerError::Overflow)?;

    // μusd = Mwei * μusd/Token / Mwei/Token + μusd)
    let total_fees_micro_usd = u64::try_from(
        u128::from(total_fees_mwei) * u128::from(oracle_evm_prices.gas_token_price) / MWEI_PER_ETH,
    )
-    .expect("Overflow")
-        + u64::from(chain_config.relayer_fee_micro_usd);
+    .map_err(|_| TokenBridgeRelayerError::Overflow)?
+    .checked_add(u64::from(chain_config.relayer_fee_micro_usd))
+    .ok_or(TokenBridgeRelayerError::Overflow)?;

    // lamports/SOL * μusd / μusd/SOL
-    Ok((LAMPORTS_PER_SOL * total_fees_micro_usd) / oracle_config.sol_price)
+    let fee = total_fees_micro_usd
+        .checked_mul(LAMPORTS_PER_SOL)
+        .map(|sol| sol / oracle_config.sol_price)
+        .ok_or(TokenBridgeRelayerError::Overflow)?;
+
+    Ok(fee)


I see where the motivation for the custom type with a single, simple overflow check at the end that you mentioned came from. Only a mother can love this code.

nonergodic · 2025-01-11T08:50:34Z

solana/programs/token-bridge-relayer/src/lib.rs

@@ -18,6 +18,9 @@ pub mod constant {

    #[constant]
    pub const SEED_PREFIX_TEMPORARY: &[u8] = b"tmp";
+
+    #[constant]
+    pub const SEED_PREFIX_UPGRADE_LOCK: &[u8] = b"upgrade lock";


Suggested change

pub const SEED_PREFIX_UPGRADE_LOCK: &[u8] = b"upgrade lock";

pub const SEED_PREFIX_UPGRADE_LOCK: &[u8] = b"upgrade_lock";

I think we canonically use underscore instead of a plain space in seed prefixes

nonergodic · 2025-01-11T09:08:25Z

solana/tests/token-bridge-relayer-tests.ts

+        userTokenAccount: tokenAccount.publicKey,
+        transferredAmount,
+        gasDropoffAmount,
+        maxFeeLamports: 100_000_000n, // 0.1SOL max


This should probably use LAMPORT_PER_SOL/10 rather than use a comment (and same for the other values)

nonergodic · 2025-01-11T09:11:02Z

solana/tests/utils/helpers.ts

+type Tuple<T, N extends number, R extends T[] = []> = R['length'] extends N
+  ? R
+  : Tuple<T, N, [T, ...R]>;


This should (probably) use the corresponding type from the SDKv2 instead (it's very marginal though).

nonergodic · 2025-01-11T09:13:44Z

solana/tests/utils/helpers.ts

+    several: <N extends number>(amount: N): Tuple<Keypair, N> =>
+      Array.from({ length: amount }).map(Keypair.generate) as Tuple<Keypair, N>,


Same considerations as above.

nonergodic · 2025-01-11T09:15:12Z

solana/tests/utils/helpers.ts

+    several: <N extends number>(
+      amount: number,
+      ethereum?: 'ethereum',
+    ): Tuple<UniversalAddress, N> =>
+      Array.from({ length: amount }).map(() => this.universalAddress.generate(ethereum)) as Tuple<
+        UniversalAddress,
+        N
+      >,


Same considerations as above.

nonergodic · 2025-01-11T09:16:16Z

solana/tests/utils/helpers.ts

+    several: <N extends number>(amount: number): Tuple<PublicKey, N> =>
+      Array.from({ length: amount }).map(PublicKey.unique) as Tuple<PublicKey, N>,


Any reason not to just make this an optional parameter of generate? also, N should be const here and amount seems like a suboptimal choice of name since it is more associated with tokens. count would likely be better.

feli-xlabs added 8 commits January 8, 2025 10:42

Small refactoring

f51d775

Fix DoS when accounts created by hand was credited first

3bb8d9b

During an inbound transfer, checks the chain ID too, in addition to t…

e75a7bc

…he peer address

During an ownership transfer, the program authority is given to a PDA…

8b1b240

… so that when the new owner accepts the transfer, there is no need for the previous owner's signature.

Remove unnecessary mut flags

06cd14d

Transfers fail if the dropoff exceeds the maximum

127c774

Refactoring, clean-up

af7e07c

Overflow checks

1fb1484

feli-xlabs force-pushed the audit-solana branch from 0886d29 to 1fb1484 Compare January 8, 2025 09:53

feli-xlabs added 3 commits January 8, 2025 10:57

Use init_if_needed for the badge in an ownership transfer

b1508cc

Split registerPeer in 2 (one for the first peer, one for the subseque…

def5826

…nts)

Enable Anchor checks

02360e2

feli-xlabs marked this pull request as ready for review January 9, 2025 18:53

nonergodic approved these changes Jan 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix errors found in the TBRv3 Solana Audit #105

Fix errors found in the TBRv3 Solana Audit #105

feli-xlabs commented Jan 2, 2025

nonergodic left a comment

nonergodic Jan 11, 2025

nonergodic Jan 11, 2025

nonergodic Jan 11, 2025

nonergodic Jan 11, 2025

nonergodic Jan 11, 2025

nonergodic Jan 11, 2025

nonergodic Jan 11, 2025

nonergodic Jan 11, 2025

nonergodic Jan 11, 2025

nonergodic Jan 11, 2025

	pub const SEED_PREFIX_UPGRADE_LOCK: &[u8] = b"upgrade lock";
	pub const SEED_PREFIX_UPGRADE_LOCK: &[u8] = b"upgrade_lock";

		several: <N extends number>(amount: N): Tuple<Keypair, N> =>
		Array.from({ length: amount }).map(Keypair.generate) as Tuple<Keypair, N>,

		several: <N extends number>(amount: number): Tuple<PublicKey, N> =>
		Array.from({ length: amount }).map(PublicKey.unique) as Tuple<PublicKey, N>,

Fix errors found in the TBRv3 Solana Audit #105

Are you sure you want to change the base?

Fix errors found in the TBRv3 Solana Audit #105

Conversation

feli-xlabs commented Jan 2, 2025

nonergodic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment