feat/add claim protocol fee 2 (requires signer as protocol-fee wrapper program PDA)#199
Merged
andrewsource147 merged 18 commits intorelease_0.2.1from Apr 9, 2026
Merged
Conversation
bangyro
force-pushed
the
feat/claim-protocol-fee-unchecked
branch
from
5a13487 to
c1a2dc7
Compare
bangyro
force-pushed
the
feat/claim-protocol-fee-unchecked
branch
2 times, most recently
from
3b33679 to
4828337
Compare
bangyro
changed the title
Collaborator
Author
|
/bot review |
|
Bot Agent progress:
Progress:
|
![metoeraag-bot[bot]](https://avatars.githubusercontent.com/u/126859799?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
Comment
Summary
Posted 3 validated inline finding(s) after final filtering.
Findings
- [P2]
programs/cp-amm/src/instructions/operator/ix_claim_protocol_fee.rs:25This changesclaim_protocol_feefrom a treasury-only payout into an arbitrary token-account payout. The old implementation calledvalidate_ata_token(..., &treasury::ID, ...); the new one only checks mint/program onreceiver_token_account, so CP-AMM itself no longer enforces where protocol fees land. If the newprotocol_fee_authorityis only meant to authorize claims, please keep a destination invariant here (or at least reject obviously unsafe targets like the source vault) instead of relying on another program's logic. - [P2]
tests/claimProtocolFee.test.ts:172After this rewrite there is no passingclaim_protocol_feecoverage left in-repo. The previous SPL and Token-2022 success cases were removed, and this file now only asserts the same wrong-signer failure path twice. Because the new flow depends on a hardcoded external PDA plus manual account validation, I'd strongly recommend adding a small CPI/integration test that signs asprotocol_fee_authorityand proves a real claim succeeds. - [P3]
tests/helpers/cpAmm.ts:491After the Rust change,claim_protocol_feecan only succeed whensigneris the fixed off-curveprotocol_fee_authorityPDA via CPI from the protocol-fee program. This helper still builds a top-level transaction signed by an arbitraryKeypair, so it cannot ever hit the success path and leaves the new behavior effectively untestable from the JS harness. Consider replacing it with a CPI harness (or renaming/removing it to avoid implying direct invocation still works).
Repo checks
Repo Checks
-
LLM checks planner: added package install step before running JS commands.
-
rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu clippy: ok
info: component clippy is up to date
- cargo fetch --locked: ok
- pnpm install --frozen-lockfile: ok
Lockfile is up to date, resolution step is skipped
Progress: resolved 1, reused 0, downloaded 0, added 0
Packages: +192
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Progress: resolved 192, reused 192, downloaded 0, added 192, done
dependencies:
+ @coral-xyz/anchor 0.31.1
+ @coral-xyz/borsh 0.31.1
+ @solana/spl-token 0.4.14
+ @solana/web3.js 1.98.4
+ borsh 2.0.0
+ decimal.js 10.6.0
+ litesvm 0.1.0
devDependencies:
+ @solana/spl-token-metadata 0.1.6
+ @types/bn.js 5.2.0
+ @types/chai 4.3.20
+ @types/mocha 9.1.1
+ chai 4.5.0
+ mocha 9.2.2
+ prettier 2.8.8
+ ts-mocha 10.1.0
+ typescript 5.9.3
╭ Warning ─────────────────────────────────────────────────────────────────────╮
│ │
│ Ignored build scripts: bigint-buffer@1.1.5, bufferutil@4.0.9, │
│ utf-8-validate@5.0.10. │
│ Run "pnpm approve-builds" to pick which dependencies should be allowed │
│ to run scripts. │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
Done in 1.1s using pnpm v10.33.0
- cargo check --workspace: ok
ing solana-message v2.2.1
Checking solana-instructions-sysvar v2.2.1
Checking solana-stake-interface v1.2.1
Checking solana-loader-v4-interface v2.2.1
Checking solana-feature-gate-interface v2.2.1
Checking solana-loader-v3-interface v3.0.0
Checking solana-vote-interface v2.2.1
Checking solana-loader-v2-interface v2.2.1
Checking solana-stable-layout v2.2.1
Checking spl-discriminator v0.4.1
Checking spl-memo v6.0.0
Checking spl-associated-token-account-client v2.0.0
Checking solana-example-mocks v2.2.1
Checking solana-sysvar v2.2.1
Checking solana-secp256k1-recover v2.2.1
Compiling anchor-attribute-program v0.31.1
Compiling anchor-attribute-account v0.31.1
Compiling anchor-derive-accounts v0.31.1
Compiling anchor-attribute-constant v0.31.1
Compiling anchor-attribute-event v0.31.1
Compiling anchor-attribute-error v0.31.1
Checking spl-pod v0.5.1
Checking solana-program v2.2.1
Checking spl-program-error v0.6.0
Checking spl-token v7.0.0
Checking spl-type-length-value v0.7.0
Checking spl-token-confidential-transfer-proof-extraction v0.2.1
Checking spl-tlv-account-resolution v0.9.0
Checking spl-elgamal-registry v0.1.1
Checking spl-token-metadata-interface v0.6.0
Checking spl-token-group-interface v0.5.0
Checking spl-transfer-hook-interface v0.9.0
Checking spl-token-confidential-transfer-proof-generation v0.2.0
Checking spl-token-confidential-transfer-ciphertext-arithmetic v0.2.1
Compiling anchor-derive-serde v0.31.1
Compiling anchor-attribute-access-control v0.31.1
Checking spl-token-2022 v6.0.0
Checking anchor-lang v0.31.1
Checking spl-associated-token-account v6.0.0
Checking anchor-spl v0.31.1
Checking cp-amm v0.2.0 (/data/workdir/2962945/MeteoraAg/damm-v2/programs/cp-amm)
Checking rust-sdk v0.2.0 (/data/workdir/2962945/MeteoraAg/damm-v2/rust-sdk)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 25.74s
- cargo clippy --workspace --all-targets -- -D warnings: failed
rt_comparison
help: replace it with `assert!(..)`
|
56 - assert_eq!(fee_mode.fees_on_input, false);
56 + assert!(!fee_mode.fees_on_input);
|
error: used `assert_eq!` with a literal bool
--> programs/cp-amm/src/tests/test_collect_fee_mode.rs:60:5
|
60 | assert_eq!(fee_mode.fees_on_token_a, false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
help: replace it with `assert!(..)`
|
60 - assert_eq!(fee_mode.fees_on_token_a, false);
60 + assert!(!fee_mode.fees_on_token_a);
|
error: importing legacy numeric constants
--> programs/cp-amm/src/tests/test_liquidity_compounding.rs:18:5
|
18 | use std::u64;
| ^^^^^^^^
|
= help: remove this import
= note: then `u64::<CONST>` will resolve to the respective associated constant
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#legacy_numeric_constants
error: manual `RangeInclusive::contains` implementation
--> programs/cp-amm/src/tests/test_liquidity_compounding.rs:34:9
|
34 | sqrt_price >= MIN_SQRT_PRICE && sqrt_price <= MAX_SQRT_PRICE,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `(MIN_SQRT_PRICE..=MAX_SQRT_PRICE).contains(&sqrt_price)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
error: casting to the same type is unnecessary (`u16` -> `u16`)
--> programs/cp-amm/src/params/fee_parameters.rs:167:38
|
167 | self.reduction_factor <= MAX_BASIS_POINT as u16,
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `MAX_BASIS_POINT`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
error: could not compile `cp-amm` (lib test) due to 182 previous errors
Collaborator
Author
|
/bot review |
|
Bot Agent progress:
Progress:
|
There was a problem hiding this comment.
Verdict
Request changes
Summary
Posted 2 validated inline finding(s) after final filtering.
Findings
- [P0]
programs/cp-amm/src/instructions/operator/ix_claim_protocol_fee.rs:13Thepoolaccount lost all itshas_oneconstraints that were previously present (has_one = token_a_vault, has_one = token_b_vault, has_one = token_a_mint, has_one = token_b_mint). Whilevalidate_accounts_and_return_withdraw_directiondoes runtime checks against pool fields, thepoolaccount itself is only constrained as#[account(mut)]— there is no constraint that it belongs to this program or has the correct discriminator beyond whatAccountLoader<'info, Pool>provides.
Critically, token_vault and token_mint are validated against pool fields using pool.token_a_vault, pool.token_b_vault, etc., but the receiver_token_account has NO constraint tying it to any specific owner or treasury. Anyone who controls the PDA signer (i.e., the external pFee3... program) can drain protocol fees to any token account they choose.
This may be intentional (the PDA signer from the external program is the sole gatekeeper), but if the intent is that protocol fees go to a specific treasury, there's no on-chain enforcement of that. Please confirm this is the desired security model, or add a constraint on the receiver.
- [P1]
tests/claimProtocolFee.test.ts:172There are no success test cases forclaim_protocol_fee. The test file only has fail cases that verify rejection when an arbitrary admin signs. There's no test that actually claims protocol fees successfully using the PDA signer, which means the happy path is completely untested. This is especially important given the new PDA-based signing mechanism — without a success test, you can't verify the PDA derivation and CPI signing work end-to-end.
Repo checks
Repo Checks
-
LLM checks planner: added package install step before running JS commands.
-
rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu clippy: ok
info: downloading component clippy
- cargo fetch --locked: ok
- pnpm install --frozen-lockfile: ok
Lockfile is up to date, resolution step is skipped
Already up to date
╭ Warning ─────────────────────────────────────────────────────────────────────╮
│ │
│ Ignored build scripts: bigint-buffer@1.1.5, bufferutil@4.0.9, │
│ utf-8-validate@5.0.10. │
│ Run "pnpm approve-builds" to pick which dependencies should be allowed │
│ to run scripts. │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
Done in 659ms using pnpm v10.33.0
- cargo check --workspace: ok
Checking cp-amm v0.2.0 (/data/workdir/2962945/MeteoraAg/damm-v2/programs/cp-amm)
Checking rust-sdk v0.2.0 (/data/workdir/2962945/MeteoraAg/damm-v2/rust-sdk)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.77s
- cargo clippy --workspace --all-targets -- -D warnings: failed
rt_comparison
help: replace it with `assert!(..)`
|
56 - assert_eq!(fee_mode.fees_on_input, false);
56 + assert!(!fee_mode.fees_on_input);
|
error: used `assert_eq!` with a literal bool
--> programs/cp-amm/src/tests/test_collect_fee_mode.rs:60:5
|
60 | assert_eq!(fee_mode.fees_on_token_a, false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
help: replace it with `assert!(..)`
|
60 - assert_eq!(fee_mode.fees_on_token_a, false);
60 + assert!(!fee_mode.fees_on_token_a);
|
error: importing legacy numeric constants
--> programs/cp-amm/src/tests/test_liquidity_compounding.rs:18:5
|
18 | use std::u64;
| ^^^^^^^^
|
= help: remove this import
= note: then `u64::<CONST>` will resolve to the respective associated constant
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#legacy_numeric_constants
error: manual `RangeInclusive::contains` implementation
--> programs/cp-amm/src/tests/test_liquidity_compounding.rs:34:9
|
34 | sqrt_price >= MIN_SQRT_PRICE && sqrt_price <= MAX_SQRT_PRICE,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `(MIN_SQRT_PRICE..=MAX_SQRT_PRICE).contains(&sqrt_price)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
error: casting to the same type is unnecessary (`u16` -> `u16`)
--> programs/cp-amm/src/params/fee_parameters.rs:167:38
|
167 | self.reduction_factor <= MAX_BASIS_POINT as u16,
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `MAX_BASIS_POINT`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
error: could not compile `cp-amm` (lib test) due to 181 previous errors
Collaborator
Author
|
/bot review |
|
Bot Agent progress:
Progress:
|
There was a problem hiding this comment.
Verdict
Request changes
Summary
Posted 1 validated inline finding(s) after final filtering.
Findings
- [P1]
programs/cp-amm/src/instructions/operator/ix_claim_protocol_fee.rs:108Please rejectreceiver_token_accountwhen it is the same account as the selected vault before mutating fee state. Right now the receiver is only validated by mint/program, so the protocol-fee program can accidentally passtoken_a_vault/token_b_vaultback as the destination. SPL Token self-transfers succeed as a no-op, which meanspool.claim_protocol_fee(...)can decrement the tracked protocol fee while leaving the tokens in the vault, permanently drifting accounting. The previous treasury ATA validation prevented this case.
Potential concerns (lower confidence)
- [P2]
tests/claimProtocolFee.test.ts:172This suite now only covers auth failures. The previous successful SPL and Token2022 claim tests were removed, and there is no replacement that exercises the new CPI/PDA-signed happy path. That leaves the core behavior of this feature unverified, including the hard-coded PDA derivation and actual token transfer. Please add at least one positive claim test via a small harness program (ideally keeping a Token2022 case too).
Repo checks
Repo Checks
-
LLM checks planner: added package install step before running JS commands.
-
rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu rustfmt: ok
info: downloading component rustfmt
- rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu clippy: ok
info: component clippy is up to date
- cargo fetch --locked: ok
- pnpm install --frozen-lockfile: ok
Lockfile is up to date, resolution step is skipped
Already up to date
╭ Warning ─────────────────────────────────────────────────────────────────────╮
│ │
│ Ignored build scripts: bigint-buffer@1.1.5, bufferutil@4.0.9, │
│ utf-8-validate@5.0.10. │
│ Run "pnpm approve-builds" to pick which dependencies should be allowed │
│ to run scripts. │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
Done in 674ms using pnpm v10.33.0
- cargo fmt --check: ok
- cargo clippy --workspace --all-targets -- -D warnings: failed
rt_comparison
help: replace it with `assert!(..)`
|
56 - assert_eq!(fee_mode.fees_on_input, false);
56 + assert!(!fee_mode.fees_on_input);
|
error: used `assert_eq!` with a literal bool
--> programs/cp-amm/src/tests/test_collect_fee_mode.rs:60:5
|
60 | assert_eq!(fee_mode.fees_on_token_a, false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
help: replace it with `assert!(..)`
|
60 - assert_eq!(fee_mode.fees_on_token_a, false);
60 + assert!(!fee_mode.fees_on_token_a);
|
error: importing legacy numeric constants
--> programs/cp-amm/src/tests/test_liquidity_compounding.rs:18:5
|
18 | use std::u64;
| ^^^^^^^^
|
= help: remove this import
= note: then `u64::<CONST>` will resolve to the respective associated constant
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#legacy_numeric_constants
error: manual `RangeInclusive::contains` implementation
--> programs/cp-amm/src/tests/test_liquidity_compounding.rs:34:9
|
34 | sqrt_price >= MIN_SQRT_PRICE && sqrt_price <= MAX_SQRT_PRICE,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `(MIN_SQRT_PRICE..=MAX_SQRT_PRICE).contains(&sqrt_price)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
error: casting to the same type is unnecessary (`u16` -> `u16`)
--> programs/cp-amm/src/params/fee_parameters.rs:167:38
|
167 | self.reduction_factor <= MAX_BASIS_POINT as u16,
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `MAX_BASIS_POINT`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
error: could not compile `cp-amm` (lib test) due to 181 previous errors
| ) | ||
| }; | ||
|
|
||
| transfer_from_pool( |
There was a problem hiding this comment.
P1 Please reject receiver_token_account when it is the same account as the selected vault before mutating fee state. Right now the receiver is only validated by mint/program, so the protocol-fee program can accidentally pass token_a_vault/token_b_vault back as the destination. SPL Token self-transfers succeed as a no-op, which means pool.claim_protocol_fee(...) can decrement the tracked protocol fee while leaving the tokens in the vault, permanently drifting accounting. The previous treasury ATA validation prevented this case.
bangyro
changed the title
bangyro
force-pushed
the
feat/claim-protocol-fee-unchecked
branch
from
ae856fb to
f890f0e
Compare
bangyro
commented
Mar 30, 2026
| @@ -0,0 +1 @@ | |||
| {} | |||
Collaborator
Author
There was a problem hiding this comment.
prettier vscode extension seems to not respect the default setting unless we have a config.
The empty config means we use the defaults
Collaborator
Author
|
/bot review |
|
Bot Agent progress:
Progress:
|
There was a problem hiding this comment.
Verdict
Request changes
Summary
Posted 5 validated inline finding(s) after final filtering.
Findings
- [P1]
programs/cp-amm/src/instructions/operator/ix_claim_protocol_fee2.rs:24No destination validation onreceiver_token_account: Unlikeclaim_protocol_fee(v1) which validates that the destination is the treasury's ATA,claim_protocol_fee2only checks that the receiver's mint matches one of the pool's mints. Any account with the correct mint is accepted as a destination. This means anyone who can invoke the protocol-fee wrapper program (and thus produce the PDA signature) can drain protocol fees to an arbitrary token account, not just the treasury.
If the intent is that the wrapper program itself enforces the treasury destination, this should be explicitly documented (e.g., a comment or in the CHANGELOG). Otherwise, consider adding a treasury check here as well, or at minimum a has_one or address constraint on the receiver.
- [P1]
programs/cp-amm/src/tests/test_safe_math.rs:5This file reintroduces theassert_eq!(..., true/false)pattern that clippy rejects viabool_assert_comparison. The local run is already failing on these new lines, so this change currently adds lint failures undercargo clippy --workspace --all-targets -- -D warnings. Please switch these toassert!(...)/assert!(!...)throughout the file. - [P1]
tests/claimProtocolFee2.test.ts:172There is no happy-path test forclaim_protocol_fee2. Both tests in theFail caseblock passadminas the signer (which is not the protocol-fee-authority PDA) and assert failure. A successful invocation would require the wrapper program to CPI with the PDA as signer. Without a success test, there's no coverage proving the instruction works end-to-end. Consider adding at least a test that verifies the correct PDA signer succeeds (even if simulated). - [P2]
programs/cp-amm/src/instructions/operator/ix_claim_protocol_fee2.rs:111Please explicitly rejectreceiver_token_account.key() == token_vault.key()before transferring. Right now the destination is only validated by mint/program, so an authorized caller can point it at the corresponding pool vault. If the token program accepts same-source/destination transfers (SPL Token does),pool.claim_protocol_fee(...)decrements the protocol-fee counters but no tokens actually leave the vault, which can desync protocol-fee accounting from the real balances. - [P2]
tests/claimProtocolFee2.test.ts:173The two test cases (lines 173 and 183) are identical — both passadminas signer and check the same error code. The second test's description says "rejects when admin signs without a registered operator account" but the code is the same as the first. One of these should be differentiated (e.g., test with a random keypair, or test a different failure mode) or removed as a duplicate.
Repo checks
Repo Checks
-
LLM checks planner: added package install step before running JS commands.
-
rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu clippy: ok
info: component clippy is up to date
- cargo fetch --locked: ok
- pnpm install --frozen-lockfile: ok
Lockfile is up to date, resolution step is skipped
Already up to date
╭ Warning ─────────────────────────────────────────────────────────────────────╮
│ │
│ Ignored build scripts: bigint-buffer@1.1.5, bufferutil@4.0.9, │
│ utf-8-validate@5.0.10. │
│ Run "pnpm approve-builds" to pick which dependencies should be allowed │
│ to run scripts. │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
Done in 669ms using pnpm v10.33.0
- cargo check --workspace: ok
Checking pinocchio v0.9.2
Checking jupiter v0.1.0 (https://github.com/MeteoraAg/zap-program?rev=064c58b317b9a85f212c0de72caea286fc72fdb4#064c58b3)
Checking zap-sdk v0.1.0 (https://github.com/MeteoraAg/zap-program?rev=064c58b317b9a85f212c0de72caea286fc72fdb4#064c58b3)
Checking pinocchio-pubkey v0.3.0
Checking protocol-zap v0.1.0 (https://github.com/MeteoraAg/zap-program?rev=064c58b317b9a85f212c0de72caea286fc72fdb4#064c58b3)
Checking cp-amm v0.2.0 (/data/workdir/2962945/MeteoraAg/damm-v2/programs/cp-amm)
Checking rust-sdk v0.2.0 (/data/workdir/2962945/MeteoraAg/damm-v2/rust-sdk)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 10.82s
- cargo clippy --workspace --all-targets -- -D warnings: failed
());
|
error: used `assert_eq!` with a literal bool
--> programs/cp-amm/src/tests/test_safe_math.rs:34:5
|
34 | assert_eq!(100u128.safe_shl(128).is_err(), true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
help: replace it with `assert!(..)`
|
34 - assert_eq!(100u128.safe_shl(128).is_err(), true);
34 + assert!(100u128.safe_shl(128).is_err());
|
error: used `assert_eq!` with a literal bool
--> programs/cp-amm/src/tests/test_safe_math.rs:40:5
|
40 | assert_eq!(100u128.safe_shr(1).is_ok(), true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
help: replace it with `assert!(..)`
|
40 - assert_eq!(100u128.safe_shr(1).is_ok(), true);
40 + assert!(100u128.safe_shr(1).is_ok());
|
error: used `assert_eq!` with a literal bool
--> programs/cp-amm/src/tests/test_safe_math.rs:41:5
|
41 | assert_eq!(200u128.safe_shr(129).is_err(), true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
help: replace it with `assert!(..)`
|
41 - assert_eq!(200u128.safe_shr(129).is_err(), true);
41 + assert!(200u128.safe_shr(129).is_err());
|
error: casting to the same type is unnecessary (`u16` -> `u16`)
--> programs/cp-amm/src/params/fee_parameters.rs:167:38
|
167 | self.reduction_factor <= MAX_BASIS_POINT as u16,
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `MAX_BASIS_POINT`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
error: could not compile `cp-amm` (lib test) due to 182 previous errors
3e2ebf6 to
2df4ac5
Compare
Collaborator
Author
|
/bot review |
|
Bot Agent progress:
Progress:
|
There was a problem hiding this comment.
Verdict
Request changes
Summary
Posted 2 validated inline finding(s) after final filtering.
Findings
- [P1]
programs/cp-amm/src/tests/test_safe_math.rs:5This new test file is a direct cause of the currentcargo clippy --workspace --all-targets -- -D warningsfailure:assert_eq!(...is_ok()/is_err(), true)triggersclippy::bool_assert_comparisonhere and in the other similar assertions below. Please switch these toassert!(...)/assert!(!...)so the lint gate passes. - [P2]
programs/cp-amm/src/instructions/operator/ix_claim_protocol_fee2.rs:53The only destination checks here are "mint is token A or B" and "account is owned by the matching token program". Unlikeclaim_protocol_fee, this no longer pins the payout to the treasury ATA insidecp-amm, so any bug or future change in the hard-coded wrapper program can redirect accumulated protocol fees to an arbitrary token account of the same mint. If the intent is only to replace the operator signer, please preserve the treasury-account validation here as a defense-in-depth check.
Repo checks
Repo Checks
-
LLM checks planner: added package install step before running JS commands.
-
rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu clippy: ok
info: component clippy is up to date
- cargo fetch --locked: ok
- pnpm install --frozen-lockfile: ok
Lockfile is up to date, resolution step is skipped
Already up to date
╭ Warning ─────────────────────────────────────────────────────────────────────╮
│ │
│ Ignored build scripts: bigint-buffer@1.1.5, bufferutil@4.0.9, │
│ utf-8-validate@5.0.10. │
│ Run "pnpm approve-builds" to pick which dependencies should be allowed │
│ to run scripts. │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
Done in 656ms using pnpm v10.33.0
- cargo check --workspace: ok
Checking cp-amm v0.2.0 (/data/workdir/2962945/MeteoraAg/damm-v2/programs/cp-amm)
Checking rust-sdk v0.2.0 (/data/workdir/2962945/MeteoraAg/damm-v2/rust-sdk)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.28s
- cargo clippy --workspace --all-targets -- -D warnings: failed
());
|
error: used `assert_eq!` with a literal bool
--> programs/cp-amm/src/tests/test_safe_math.rs:34:5
|
34 | assert_eq!(100u128.safe_shl(128).is_err(), true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
help: replace it with `assert!(..)`
|
34 - assert_eq!(100u128.safe_shl(128).is_err(), true);
34 + assert!(100u128.safe_shl(128).is_err());
|
error: used `assert_eq!` with a literal bool
--> programs/cp-amm/src/tests/test_safe_math.rs:40:5
|
40 | assert_eq!(100u128.safe_shr(1).is_ok(), true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
help: replace it with `assert!(..)`
|
40 - assert_eq!(100u128.safe_shr(1).is_ok(), true);
40 + assert!(100u128.safe_shr(1).is_ok());
|
error: used `assert_eq!` with a literal bool
--> programs/cp-amm/src/tests/test_safe_math.rs:41:5
|
41 | assert_eq!(200u128.safe_shr(129).is_err(), true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
help: replace it with `assert!(..)`
|
41 - assert_eq!(200u128.safe_shr(129).is_err(), true);
41 + assert!(200u128.safe_shr(129).is_err());
|
error: casting to the same type is unnecessary (`u16` -> `u16`)
--> programs/cp-amm/src/params/fee_parameters.rs:167:38
|
167 | self.reduction_factor <= MAX_BASIS_POINT as u16,
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `MAX_BASIS_POINT`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
error: could not compile `cp-amm` (lib test) due to 182 previous errors
2df4ac5 to
d04edde
Compare
There was a problem hiding this comment.
Verdict
Request changes
Summary
Posted 1 validated inline finding(s) after final filtering.
Findings
- [P1]
programs/cp-amm/src/instructions/operator/ix_claim_protocol_fee2.rs:24Security:receiver_token_accountis not validated against the treasury address.
Unlike the original claim_protocol_fee which validates that the destination is the treasury's ATA (via IncorrectATA checks), claim_protocol_fee2 only checks that receiver_token_account.mint matches one of the pool mints. This means whatever program holds the protocol_fee_authority PDA signing key can direct protocol fees to any token account, not necessarily the treasury.
The intent seems to be that the protocol-fee wrapper program handles this validation, but since this instruction is on the AMM program itself and is callable by anyone who can get the PDA to sign (i.e., the wrapper program code), the security boundary is only as strong as the wrapper program. If the wrapper program has a bug or is upgradeable, fees could be stolen.
Consider whether a treasury address check should be enforced here as a defense-in-depth measure (similar to the original claim_protocol_fee), or at minimum document clearly that the security of this instruction relies entirely on the correctness of the external protocol-fee wrapper program at pFee3tb7qh5z53jRF4PbLwmNd148Q8ypLNZbqsMeinA.
Repo checks
Repo Checks
-
LLM checks planner: added package install step before running JS commands.
-
rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu rustfmt: ok
info: component rustfmt is up to date
- cargo fetch --locked: ok
- pnpm install --frozen-lockfile: ok
Lockfile is up to date, resolution step is skipped
Already up to date
╭ Warning ─────────────────────────────────────────────────────────────────────╮
│ │
│ Ignored build scripts: bigint-buffer@1.1.5, bufferutil@4.0.9, │
│ utf-8-validate@5.0.10. │
│ Run "pnpm approve-builds" to pick which dependencies should be allowed │
│ to run scripts. │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
Done in 664ms using pnpm v10.33.0
- cargo check --workspace: ok
Checking cp-amm v0.2.1 (/data/workdir/2962945/MeteoraAg/damm-v2/programs/cp-amm)
Checking rust-sdk v0.2.1 (/data/workdir/2962945/MeteoraAg/damm-v2/rust-sdk)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 6.46s
- cargo fmt --check: ok
bangyro
force-pushed
the
feat/claim-protocol-fee-unchecked
branch
from
a13f7c6 to
2650a5e
Compare
- remove claim protocl fee unchecked - remove zap protocol fee
- update zap.so to 0.2.2
- consistent account order - remove event_cpi
- add comment for emit! log could be truncated
bangyro
force-pushed
the
feat/claim-protocol-fee-unchecked
branch
2 times, most recently
from
d863fa7 to
84f0374
Compare
bangyro
force-pushed
the
feat/claim-protocol-fee-unchecked
branch
3 times, most recently
from
f9739ac to
93aa924
Compare
bangyro
force-pushed
the
feat/claim-protocol-fee-unchecked
branch
from
93aa924 to
c7bc4cf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.