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

refactor: enhance PDA validation #40

Merged
merged 6 commits into from
Oct 8, 2024
Merged

refactor: enhance PDA validation #40

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9555f86
add PDA address to README
brewmaster012 Oct 7, 2024
46cecb2
enhance pda validation
brewmaster012 Oct 7, 2024
f156723
Update programs/protocol-contracts-solana/src/lib.rs
brewmaster012 Oct 7, 2024
64e914f
Update README.md
brewmaster012 Oct 7, 2024
b0d3184
add pda_ata validation as constraints
brewmaster012 Oct 7, 2024
4f36439
Merge branch 'main' into enhance-pda-validation
brewmaster012 Oct 8, 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
5 changes: 5 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ Mainnet-beta, testnet, devnet gateway program address:
ZETAjseVjuFsxdRxo6MmTCvqFwb3ZHUx56Co3vCmGis
```

The PDA account address (derived from seeds `b"meta"` and canonical bump) is
```
2f9SLuUNb7TNeM6gzBwT4ZjbL5ZyKzzHg1Ce9yiquEjj
```

This repository hosts the smart contracts (program)
on Solana network to support ZetaChain cross-chain
functionality. Specifically, it consists of a single
Expand Down
18 changes: 9 additions & 9 deletions programs/protocol-contracts-solana/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ declare_id!("ZETAjseVjuFsxdRxo6MmTCvqFwb3ZHUx56Co3vCmGis");

#[program]
pub mod gateway {

use super::*;

pub fn initialize(
Expand Down Expand Up @@ -338,8 +337,9 @@ pub struct Deposit<'info> {
#[account(mut)]
pub signer: Signer<'info>,

#[account(mut)]
#[account(mut, seeds = [b"meta"], bump)]
pub pda: Account<'info, Pda>,

pub system_program: Program<'info, System>,
}

Expand All @@ -348,8 +348,9 @@ pub struct DepositSplToken<'info> {
#[account(mut)]
pub signer: Signer<'info>,

#[account(mut, seeds = [b"meta"], bump)]
#[account(seeds = [b"meta"], bump)]
pub pda: Account<'info, Pda>,

pub token_program: Program<'info, Token>,

#[account(mut)]
Expand All @@ -363,7 +364,7 @@ pub struct Withdraw<'info> {
#[account(mut)]
pub signer: Signer<'info>,

#[account(mut)]
#[account(mut, seeds = [b"meta"], bump)]
pub pda: Account<'info, Pda>,
/// CHECK: to account is not read so no need to check its owners; the program neither knows nor cares who the owner is.
#[account(mut)]
Expand All @@ -378,10 +379,9 @@ pub struct WithdrawSPLToken<'info> {
#[account(mut, seeds = [b"meta"], bump)]
pub pda: Account<'info, Pda>,

#[account(mut)]
#[account(mut, token::mint = mint_account, token::authority = pda)]
pub pda_ata: Account<'info, TokenAccount>, // associated token address of PDA
Copy link
Contributor

Choose a reason for hiding this comment

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

just to double check if we can add some of these spl token constraints on these related accounts:
https://www.quicknode.com/guides/solana-development/anchor/how-to-use-constraints-in-anchor#spl-token-constraints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

b0d3184

added a few constraints. This seems to be redundant to the checks inside the withdrawSPLToken function:
as a result, there is no way to trigger the error SPLAtaAndMintAddressMismatch in the test.
For now let's leave both validation there as they do not harm.

let pda_ata = get_associated_token_address(&pda.key(), &ctx.accounts.mint_account.key());
        require!(
            pda_ata == ctx.accounts.pda_ata.to_account_info().key(),
            Errors::SPLAtaAndMintAddressMismatch
        );

Copy link
Contributor

Choose a reason for hiding this comment

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

yes i suspected we can achieve the same validation with anchor after checking link above

i am ok with leaving both validations, but also would be ok to remove ours if it cant be triggered anymore to keep programs simpler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will keep both for now.


#[account()]
pub mint_account: Account<'info, Mint>,

#[account(mut)]
Expand All @@ -392,23 +392,23 @@ pub struct WithdrawSPLToken<'info> {

#[derive(Accounts)]
pub struct UpdateTss<'info> {
#[account(mut)]
#[account(mut, seeds = [b"meta"], bump)]
pub pda: Account<'info, Pda>,
#[account(mut)]
pub signer: Signer<'info>,
}

#[derive(Accounts)]
pub struct UpdateAuthority<'info> {
#[account(mut)]
#[account(mut, seeds = [b"meta"], bump)]
pub pda: Account<'info, Pda>,
#[account(mut)]
pub signer: Signer<'info>,
}

#[derive(Accounts)]
pub struct UpdatePaused<'info> {
#[account(mut)]
#[account(mut, seeds = [b"meta"], bump)]
pub pda: Account<'info, Pda>,
#[account(mut)]
pub signer: Signer<'info>,
Expand Down
50 changes: 40 additions & 10 deletions tests/protocol-contracts-solana.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ describe("some tests", () => {
await gatewayProgram.methods.depositSplToken(new anchor.BN(1_000_000), Array.from(address)).accounts({
from: tokenAccount.address,
to: pda_ata.address,
}).rpc({commitment: 'confirmed'});
}).rpc({commitment: 'processed'});
acct = await spl.getAccount(conn, pda_ata.address);
let bal1 = acct.amount;
expect(bal1-bal0).to.be.eq(1_000_000n);
Expand Down Expand Up @@ -298,7 +298,7 @@ describe("some tests", () => {
} catch (err) {
expect(err).to.be.instanceof(anchor.AnchorError);
console.log("Error message: ", err.message);
expect(err.message).to.include("SPLAtaAndMintAddressMismatch");
expect(err.message).to.include("ConstraintTokenMint");
const account4 = await spl.getAccount(conn, pda_ata.address);
console.log("After 2nd withdraw: Account balance:", account4.amount.toString());
expect(account4.amount).to.be.eq(2_500_000n);
Expand All @@ -307,7 +307,7 @@ describe("some tests", () => {
});

it("deposit and withdraw 0.5 SOL from Gateway with ECDSA signature", async () => {
await gatewayProgram.methods.deposit(new anchor.BN(1_000_000_000), Array.from(address)).accounts({pda: pdaAccount}).rpc();
await gatewayProgram.methods.deposit(new anchor.BN(1_000_000_000), Array.from(address)).accounts({}).rpc();
let bal1 = await conn.getBalance(pdaAccount);
console.log("pda account balance", bal1);
expect(bal1).to.be.gte(1_000_000_000);
Expand Down Expand Up @@ -341,7 +341,6 @@ describe("some tests", () => {
await gatewayProgram.methods.withdraw(
amount, Array.from(signatureBuffer), Number(recoveryParam), Array.from(message_hash), nonce)
.accounts({
pda: pdaAccount,
to: to,
}).rpc();
let bal2 = await conn.getBalance(pdaAccount);
Expand All @@ -353,7 +352,7 @@ describe("some tests", () => {

it("deposit and call", async () => {
let bal1 = await conn.getBalance(pdaAccount);
const txsig = await gatewayProgram.methods.depositAndCall(new anchor.BN(1_000_000_000), Array.from(address), Buffer.from("hello", "utf-8")).accounts({pda: pdaAccount}).rpc({commitment: 'confirmed'});
const txsig = await gatewayProgram.methods.depositAndCall(new anchor.BN(1_000_000_000), Array.from(address), Buffer.from("hello", "utf-8")).accounts({}).rpc({commitment: 'confirmed'});
const tx = await conn.getParsedTransaction(txsig, 'confirmed');
console.log("deposit and call parsed tx", tx);
let bal2 = await conn.getBalance(pdaAccount);
Expand All @@ -375,7 +374,6 @@ describe("some tests", () => {
// only the authority stored in PDA can update the TSS address; the following should fail
try {
await gatewayProgram.methods.updateTss(Array.from(newTss)).accounts({
pda: pdaAccount,
signer: mint.publicKey,
}).signers([mint]).rpc();
} catch (err) {
Expand All @@ -389,12 +387,12 @@ describe("some tests", () => {
randomFillSync(newTss);
// console.log("generated new TSS address", newTss);
await gatewayProgram.methods.setDepositPaused(true).accounts({
pda: pdaAccount,

}).rpc();

// now try deposit, should fail
try {
await gatewayProgram.methods.depositAndCall(new anchor.BN(1_000_000), Array.from(address), Buffer.from('hi', 'utf-8')).accounts({pda: pdaAccount}).rpc();
await gatewayProgram.methods.depositAndCall(new anchor.BN(1_000_000), Array.from(address), Buffer.from('hi', 'utf-8')).accounts({}).rpc();
} catch (err) {
console.log("Error message: ", err.message);
expect(err).to.be.instanceof(anchor.AnchorError);
Expand All @@ -405,15 +403,15 @@ describe("some tests", () => {
it("update authority", async () => {
const newAuthority = anchor.web3.Keypair.generate();
await gatewayProgram.methods.updateAuthority(newAuthority.publicKey).accounts({
pda: pdaAccount,

}).rpc();
// const pdaAccountData = await gatewayProgram.account.pda.fetch(pdaAccount);
// expect(pdaAccountData.authority).to.be.eq(newAuthority.publicKey);

// now the old authority cannot update TSS address and will fail
try {
await gatewayProgram.methods.updateTss(Array.from(new Uint8Array(20))).accounts({
pda: pdaAccount,

}).rpc();
} catch (err) {
console.log("Error message: ", err.message);
Expand All @@ -422,6 +420,38 @@ describe("some tests", () => {
}
});

it("create an account owned by the gateway program", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nits: maybe lets remove console logs and comment if we need this test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test case is removed in the subsequent PR #42

const gateway_id =gatewayProgram.programId;
console.log("gateway program id", gateway_id.toString());
const fake_pda = anchor.web3.Keypair.generate();
const rentExemption = await conn.getMinimumBalanceForRentExemption(100);
const instr1 = anchor.web3.SystemProgram.createAccount(
{
fromPubkey: wallet.publicKey,
newAccountPubkey: fake_pda.publicKey,
lamports: rentExemption,
space: 100,
programId: gatewayProgram.programId,
}
)
const tx = new anchor.web3.Transaction();
tx.add(instr1, );
await anchor.web3.sendAndConfirmTransaction(conn, tx, [wallet, fake_pda]);

const newTss = new Uint8Array(20);
randomFillSync(newTss);
// console.log("generated new TSS address", newTss);
try {
await gatewayProgram.methods.updateTss(Array.from(newTss)).accounts({
pda: fake_pda.publicKey,
}).rpc();
} catch (err) {
console.log("Error message: ", err.message);
expect(err).to.be.instanceof(anchor.AnchorError);
expect(err.message).to.include("AccountDiscriminatorMismatch.");
}
});


});

Expand Down
Loading