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.

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

Solana/sdk set evm cost initialize #116

Merged
merged 8 commits into from
Feb 5, 2025
Merged

Conversation

scnale
Copy link
Collaborator

@scnale scnale commented Feb 4, 2025

Adds EVM cost update instruction into the initialize Solana sdk function.

After writing this, I think embedding this into the initialize instruction would be simpler 😅

@scnale scnale requested a review from a team February 4, 2025 22:37
Copy link

XLabs/arbitrary-token-transfers #116

Change Summary:

  • Added EVM relay gas and transaction size configuration parameters to Solana Token Bridge Relayer initialization process
  • Updated initialization method to include additional configuration and validation steps
  • Modified interfaces and configuration files across different network environments (localnet, mainnet, testnet)
  • Enhanced program initialization with more robust signer and authority checks
  • Updated EVM constant for transaction size in smart contract

Risk Score: 5/10

  • Explanation: Moderate complexity changes with potential configuration and initialization impact, but no critical security vulnerabilities detected

Potential Vulnerabilities:
File: sdk/solana/tbrv3/token-bridge-relayer.ts:354-400

async initialize(
  signer: PublicKey,
  {
    owner,
    feeRecipient,
    admins,
  }: {
    owner: PublicKey;
    feeRecipient: PublicKey;
    admins: PublicKey[];
  },
  evmTransactionGas: bigint,
  evmTransactionSize: bigint,
): Promise<TransactionInstruction[]> {
  • Potential Authorization Bypass: The method allows multiple initialization instructions, which could potentially be exploited if not carefully managed
  • The signer validation logic seems complex and might have edge cases not fully covered

Code Smell:
File: sdk/solana/tbrv3/token-bridge-relayer.ts:354-400

if (!signer.equals(owner) && !admins.some((key) => signer.equals(key)))
  throwError(`The signer must be set as either the owner or an admin`);
  • Complex conditional logic that could be simplified
  • Multiple nested conditions make the code harder to read and maintain
  • Error handling could be more explicit and provide more context

Unintended Consequences:
File: deployment/config/*/solana-tbr-init.json

{
    "evmRelayGas": "400000",
    "evmRelayTxSize": "1211"
}
  • Hard-coded configuration values across different environments might lead to:
    1. Inflexible deployment scenarios
    2. Potential configuration drift between environments
    3. Manual intervention required for environment-specific tuning

Debug Log:
File: deployment/helpers/solana.ts:128

/**
 * @todo Add priority fee instruction when configured to do so.
 */
export async function ledgerSignAndSend(connection: Connection, instructions: TransactionInstruction[], signers: Keypair[]) {
  • TODO comment left in production code
  • Indicates potential incomplete implementation
  • Should be tracked in issue management system rather than in code comments

solana/tests/utils/tbr-wrapper.ts Outdated Show resolved Hide resolved
Comment on lines +368 to +369
evmTransactionGas: bigint,
evmTransactionSize: bigint,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both could be just numbers too. 🤔

The real datatype we'd want is a uint... and preferably one with units...

Copy link
Collaborator Author

@scnale scnale Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're both uint64 for some reason. I think that this is excessive for the tx size but I don't recall if this makes sense for the gas limit. I think we chose uint32 in SR, right?

}): Promise<VersionedTransactionResponse | null> {
return $.getTransaction($.sendAndConfirm(await this.client.initialize(args), this.signer));
async initialize(
...args: Tail<Parameters<SolanaTokenBridgeRelayer["initialize"]>>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice

@scnale scnale merged commit a726b62 into main Feb 5, 2025
1 check passed
@scnale scnale deleted the solana/sdk-set-evm-cost-initialize branch February 5, 2025 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants