Fix/bug78 prevent foreign token withdrawal#150
Fix/bug78 prevent foreign token withdrawal#150sameezy667 wants to merge 4 commits intoStabilityNexus:mainfrom
Conversation
…related tokens; fix Vitest config with wallet-svelte-component mocks
…tion invariant (NonCoreTokensRemainConstant)
…s (noForeignTokens)
…e green, REFUND suite has failing cases to address
WalkthroughThe pull request adds foreign token validation to the Bene contract's replication and withdrawal flows. It introduces two new validation checks: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/contracts/withdraw_funds_unrelated_tokens.test.ts (1)
8-14: Update comment to reflect that the guard has been implemented.The comment states "demonstrates a bug" and "The contract does not restrict removal" (lines 8-12), but the PR has actually added the
NonCoreTokensRemainConstantguard to prevent this. The comment should be updated to reflect that this test validates the fix rather than demonstrating a bug.Apply this diff to update the comment:
-// This test demonstrates a bug in contract_v2.es: during withdrawFunds, the contract allows moving arbitrary extra tokens -// that were previously injected into the SELF box, as long as APT and PFT remain constant and the base-token amounts are correct. +// This test validates that the contract correctly prevents moving arbitrary extra tokens +// during withdrawFunds via the NonCoreTokensRemainConstant guard. // Expected behavior: Only the base token (ERG or configured token) should be extracted; any unrelated tokens on the contract // must remain on the replicated SELF box. -// Current behavior: The contract does not restrict removal of unrelated tokens in withdrawFunds, enabling owner to sweep them. -// This test is intentionally written to expect failure (result === false). It currently passes under the buggy contract, -// therefore the test will FAIL, proving the issue. +// The test expects the withdrawal attempt to be rejected (result === false) when trying to sweep foreign tokens.tests/mocks/wallet-svelte-component.ts (1)
57-60: Consider making the mock address more obviously fake.The dummy P2PK address at line 59 looks somewhat realistic. For clarity in tests, consider using an obviously fake format or adding more "Mock" text.
- return "9fMockChangeAddress000000000000000000000000000000000000000000000000000"; + return "9MockChangeAddressNotRealDoNotUse000000000000000000000000000000000";This makes it more obvious if this address accidentally gets used in a way that could cause confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/bene_contract/contract_v2.es(3 hunks)tests/contracts/withdraw_funds_unrelated_tokens.test.ts(1 hunks)tests/mocks/wallet-svelte-component.ts(1 hunks)vitest.config.ts(1 hunks)
🔇 Additional comments (5)
contracts/bene_contract/contract_v2.es (2)
139-149: LGTM! Foreign token guard correctly prevents unrelated tokens during replication.The
noForeignTokensvalidation correctly filters allowed tokens (APT, PFT, and base token for non-ERG mode) and ensures the output contains only these core tokens. The logic properly handles both ERG and non-ERG base token modes, and the integration into the replication validation at line 149 ensures this guard is enforced.
173-187: The non-core token comparison logic is correct and properly guarded.The
NonCoreTokensRemainConstantvalidation uses a short-circuit pattern (!isReplicationBoxPresent || { ... }) that ensures the token check only runs when OUTPUTS(0) is a replicated box (same script). The logic correctly verifies that:
- Every non-core token in SELF exists in OUTPUTS(0) with the same ID and amount (via the
badMatchesfilter)- No extra tokens are present in the output (via the
selfOther.size == outOther.sizecheck)This prevents the edge case where SELF has token A and OUTPUT has token B with the same amount—the
badMatchescheck would catch the missing token A and reject the transaction.vitest.config.ts (1)
14-15: LGTM! Test aliases correctly configured.The path aliases properly map both the main
wallet-svelte-componentmodule and itsdist/wallet/wallet-managersubpath to the test mock. This allows tests to resolve wallet dependencies without requiring the actual wallet library in the test environment.tests/mocks/wallet-svelte-component.ts (2)
16-21: LGTM! Error handling ensures test stability.The subscribe function correctly emits the current value on subscription (line 19) and includes try-catch to prevent subscriber errors from breaking the mock (line 19). The unsubscribe function properly removes the subscriber (line 20).
23-28: Good defensive programming in the set function.The try-catch blocks (line 26) ensure that errors in subscriber callbacks don't prevent other subscribers from being notified or crash the test suite. This is important for test stability.
| ProofFundingTokenRemainsConstant, // There is no need to modify the proof of funding token, so it must be constant | ||
| NonCoreTokensRemainConstant // Prevents withdrawing unrelated tokens |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l contracts/bene_contract/contract_v2.esRepository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 125
🏁 Script executed:
# First, let's examine the area around the reviewed lines (471-472)
sed -n '465,480p' contracts/bene_contract/contract_v2.esRepository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 1295
🏁 Script executed:
# Now let's look at the NonCoreTokensRemainConstant check and isReplicationBoxPresent at line 173
sed -n '170,180p' contracts/bene_contract/contract_v2.esRepository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 634
🏁 Script executed:
# Let's examine the endOrReplicate function around lines 456-461
sed -n '450,465p' contracts/bene_contract/contract_v2.esRepository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 783
🏁 Script executed:
# Get a broader view of the NonCoreTokensRemainConstant definition
sed -n '1,500p' contracts/bene_contract/contract_v2.es | grep -n -B 5 -A 5 "NonCoreTokensRemainConstant"Repository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 1348
🏁 Script executed:
# Get the complete NonCoreTokensRemainConstant definition
sed -n '173,195p' contracts/bene_contract/contract_v2.esRepository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 1389
🏁 Script executed:
# Check what isReplicationBoxPresent is and how it's defined
grep -n "isReplicationBoxPresent" contracts/bene_contract/contract_v2.es | head -20Repository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 1262
🏁 Script executed:
# Look for token initialization and what tokens should exist
sed -n '1,150p' contracts/bene_contract/contract_v2.es | grep -n -i "token\|pft\|base"Repository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 4613
🏁 Script executed:
# Check if there are constraints on what tokens can be in outputs
sed -n '300,450p' contracts/bene_contract/contract_v2.esRepository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 7290
🏁 Script executed:
# Check the withdrawal validation for outputs and tokens
sed -n '455,495p' contracts/bene_contract/contract_v2.esRepository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 2585
🏁 Script executed:
# Look at what happens with tokens in the withdrawal flow
sed -n '240,280p' contracts/bene_contract/contract_v2.esRepository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 1584
🏁 Script executed:
# Check if there are any other guards on withdrawal outputs
grep -n "OUTPUTS\|owner\|devFee" contracts/bene_contract/contract_v2.es | sed -n '80,120p'Repository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 65
🏁 Script executed:
# Look at the complete isWithdrawFunds validation including final sigmaProp
sed -n '400,530p' contracts/bene_contract/contract_v2.esRepository: StabilityNexus/BenefactionPlatform-Ergo
Length of output: 6962
Verify interaction between endOrReplicate and NonCoreTokensRemainConstant during termination.
The NonCoreTokensRemainConstant guard short-circuits to true when terminating (no replication box present), allowing foreign tokens to bypass validation. At line 173, the check is defined as !isReplicationBoxPresent || {...}, which means:
- During partial withdrawals (replication present): Foreign tokens are validated and must remain constant
- During termination (no replication): The guard unconditionally returns true without checking token amounts
The isWithdrawFunds action at lines 456-461 allows termination when allTokensWithdrawn is satisfied, but this only verifies that the PFT token is absent—it does not validate that all tokens are extracted. When combined with the disabled NonCoreTokensRemainConstant guard, foreign tokens can be extracted during final termination without enforcement.
This creates a gap: the withdrawal validation constrains only the owner and dev fee outputs (lines 419-432), with no validation preventing foreign tokens from being sent to other outputs or swept entirely. If the PR objective of "prevents withdrawing or interacting with foreign/non-core tokens" applies to both partial and full termination flows, this needs to be addressed.
🤖 Prompt for AI Agents
In contracts/bene_contract/contract_v2.es around lines 471-472 (and related
logic at ~line 173 and withdrawal flow at ~lines 419-461), the
NonCoreTokensRemainConstant guard currently short-circuits when
isReplicationBoxPresent is false, allowing termination to bypass validation and
let foreign tokens be extracted; change the termination path so it also enforces
non-core token constraints by either (A) extending NonCoreTokensRemainConstant
to perform token equality checks even when isReplicationBoxPresent is false, or
(B) adding an explicit termination-time validation in endOrReplicate/end
withdrawal logic that asserts all non-core token amounts in inputs equal the
amounts in permitted outputs (or that no foreign tokens remain in any outputs
except the designated owner/dev outputs), and ensure this check runs when
isWithdrawFunds/termination is taken so foreign tokens cannot be swept during
final termination.
| it("should fail when withdraw attempts to move a foreign token from the contract", () => { | ||
| // Compute expected split | ||
| const withdrawAmount = ctx.isErgMode ? (collectedFunds - SAFE_MIN_BOX_VALUE) / 2n : collectedFunds / 2n; | ||
| const devFeeAmount = (withdrawAmount * BigInt(ctx.devFeePercentage)) / 100n; | ||
| const projectAmount = withdrawAmount - devFeeAmount; | ||
| const devFeeContract = compile(`{ sigmaProp(true) }`); | ||
|
|
||
| // Build outputs with partial withdraw (replicate SELF) and sweep the foreign token to owner | ||
| let remainingValue: bigint; | ||
| let remainingAssets: { tokenId: string; amount: bigint }[]; | ||
| let projectValue: bigint; | ||
| let projectAssets: { tokenId: string; amount: bigint }[]; | ||
| let devFeeValue: bigint; | ||
| let devFeeAssets: { tokenId: string; amount: bigint }[]; | ||
|
|
||
| const foreignTokenIdOnSelf = projectBox.assets.find(t => t.tokenId !== ctx.projectNftId && t.tokenId !== ctx.pftTokenId && t.tokenId !== ctx.baseTokenId)?.tokenId!; | ||
| const foreignTokenAmountOnSelf = projectBox.assets.find(t => t.tokenId === foreignTokenIdOnSelf)?.amount ?? 0n; | ||
|
|
||
| if (ctx.isErgMode) { | ||
| // ERG mode: decrease ERG on SELF, move ERG to project/dev, and also move foreign token to project | ||
| remainingValue = collectedFunds - withdrawAmount; // keep remainder on self | ||
| remainingAssets = [ | ||
| { tokenId: ctx.projectNftId, amount: projectBox.assets[0].amount }, // keep APT | ||
| // NOTE: we DO NOT keep the foreign token on SELF; we will try to move it out | ||
| ]; | ||
|
|
||
| projectValue = projectAmount; | ||
| projectAssets = []; | ||
|
|
||
| devFeeValue = devFeeAmount; | ||
| devFeeAssets = []; | ||
| } else { | ||
| // Token mode: keep half base tokens on SELF, move half to project/dev, and also move foreign token to project | ||
| remainingValue = SAFE_MIN_BOX_VALUE; | ||
| remainingAssets = [ | ||
| { tokenId: ctx.projectNftId, amount: projectBox.assets[0].amount }, | ||
| { tokenId: ctx.baseTokenId, amount: collectedFunds - withdrawAmount }, | ||
| // NOTE: we DO NOT keep the foreign token on SELF; we will try to move it out | ||
| ]; | ||
|
|
||
| projectValue = SAFE_MIN_BOX_VALUE; | ||
| projectAssets = [ | ||
| { tokenId: ctx.baseTokenId, amount: projectAmount }, | ||
| // BUG: Also extracting unrelated token to owner | ||
| { tokenId: foreignTokenIdOnSelf, amount: foreignTokenAmountOnSelf }, | ||
| ]; | ||
|
|
||
| devFeeValue = SAFE_MIN_BOX_VALUE; | ||
| devFeeAssets = [ | ||
| { tokenId: ctx.baseTokenId, amount: devFeeAmount }, | ||
| ]; | ||
| } | ||
|
|
||
| const transaction = new TransactionBuilder(ctx.mockChain.height) | ||
| .from([projectBox, ...ctx.projectOwner.utxos.toArray()]) | ||
| .to([ | ||
| // Output 0: Replicated SELF with updated funds (but foreign token removed) | ||
| new OutputBuilder(remainingValue, ctx.beneErgoTree) | ||
| .addTokens(remainingAssets) | ||
| .setAdditionalRegisters({ | ||
| R4: projectBox.additionalRegisters.R4, | ||
| R5: SLong(ctx.minimumTokensSold).toHex(), | ||
| R6: SColl(SLong, [soldTokens, 0n, ctx.totalPFTokens]).toHex(), | ||
| R7: SLong(ctx.exchangeRate).toHex(), | ||
| R8: projectBox.additionalRegisters.R8, | ||
| R9: projectBox.additionalRegisters.R9, | ||
| }), | ||
| // Output 1: Project | ||
| new OutputBuilder(projectValue, ctx.projectOwner.address).addTokens(projectAssets), | ||
| // Output 2: Dev fee | ||
| new OutputBuilder(devFeeValue, devFeeContract).addTokens(devFeeAssets), | ||
| ]) | ||
| .sendChangeTo(ctx.projectOwner.address) | ||
| .payFee(RECOMMENDED_MIN_FEE_VALUE) | ||
| .build(); | ||
|
|
||
| const result = ctx.mockChain.execute(transaction, { signers: [ctx.projectOwner], throw: false }); | ||
|
|
||
| // EXPECTED: The contract should reject removing unrelated tokens from SELF during withdraw | ||
| // ACTUAL: The contract currently allows it (APT and PFT remain constant; base token amounts are correct) | ||
| expect(result).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test won't catch the termination path vulnerability.
This test validates partial withdrawal (with replication), but as identified in the contract review, the NonCoreTokensRemainConstant guard doesn't protect against sweeping foreign tokens during full termination (when isReplicationBoxPresent is false).
Add a second test case that validates full contract termination cannot sweep foreign tokens. The test should:
- Set up a contract box with all tokens sold and all funds collected
- Inject a foreign token
- Attempt to terminate (no replication) while moving the foreign token to owner
- Assert the transaction is rejected
Would you like me to generate this additional test case?
| if (ctx.isErgMode) { | ||
| // ERG mode: decrease ERG on SELF, move ERG to project/dev, and also move foreign token to project | ||
| remainingValue = collectedFunds - withdrawAmount; // keep remainder on self | ||
| remainingAssets = [ | ||
| { tokenId: ctx.projectNftId, amount: projectBox.assets[0].amount }, // keep APT | ||
| // NOTE: we DO NOT keep the foreign token on SELF; we will try to move it out | ||
| ]; | ||
|
|
||
| projectValue = projectAmount; | ||
| projectAssets = []; | ||
|
|
||
| devFeeValue = devFeeAmount; | ||
| devFeeAssets = []; | ||
| } else { | ||
| // Token mode: keep half base tokens on SELF, move half to project/dev, and also move foreign token to project | ||
| remainingValue = SAFE_MIN_BOX_VALUE; | ||
| remainingAssets = [ | ||
| { tokenId: ctx.projectNftId, amount: projectBox.assets[0].amount }, | ||
| { tokenId: ctx.baseTokenId, amount: collectedFunds - withdrawAmount }, | ||
| // NOTE: we DO NOT keep the foreign token on SELF; we will try to move it out | ||
| ]; | ||
|
|
||
| projectValue = SAFE_MIN_BOX_VALUE; | ||
| projectAssets = [ | ||
| { tokenId: ctx.baseTokenId, amount: projectAmount }, | ||
| // BUG: Also extracting unrelated token to owner | ||
| { tokenId: foreignTokenIdOnSelf, amount: foreignTokenAmountOnSelf }, | ||
| ]; | ||
|
|
||
| devFeeValue = SAFE_MIN_BOX_VALUE; | ||
| devFeeAssets = [ | ||
| { tokenId: ctx.baseTokenId, amount: devFeeAmount }, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Critical test coverage gap: ERG mode foreign token sweep not tested.
The test constructs different scenarios for ERG vs token mode (lines 92-125), but in ERG mode (lines 92-104), the foreign token is not moved to the project output. Lines 100-104 show projectAssets = [] with no foreign token, meaning the ERG mode test doesn't actually attempt to sweep the foreign token—it only tests removing it from SELF without moving it anywhere.
Meanwhile, the token mode correctly tests sweeping the foreign token to the project owner (lines 117-119).
The ERG mode branch needs to attempt moving the foreign token to validate the guard. Apply this diff:
projectValue = projectAmount;
- projectAssets = [];
+ projectAssets = [
+ // BUG: Also extracting unrelated token to owner in ERG mode
+ { tokenId: foreignTokenIdOnSelf, amount: foreignTokenAmountOnSelf },
+ ];
devFeeValue = devFeeAmount;
devFeeAssets = [];Without this, the ERG mode test doesn't validate that the guard prevents foreign token extraction.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (ctx.isErgMode) { | |
| // ERG mode: decrease ERG on SELF, move ERG to project/dev, and also move foreign token to project | |
| remainingValue = collectedFunds - withdrawAmount; // keep remainder on self | |
| remainingAssets = [ | |
| { tokenId: ctx.projectNftId, amount: projectBox.assets[0].amount }, // keep APT | |
| // NOTE: we DO NOT keep the foreign token on SELF; we will try to move it out | |
| ]; | |
| projectValue = projectAmount; | |
| projectAssets = []; | |
| devFeeValue = devFeeAmount; | |
| devFeeAssets = []; | |
| } else { | |
| // Token mode: keep half base tokens on SELF, move half to project/dev, and also move foreign token to project | |
| remainingValue = SAFE_MIN_BOX_VALUE; | |
| remainingAssets = [ | |
| { tokenId: ctx.projectNftId, amount: projectBox.assets[0].amount }, | |
| { tokenId: ctx.baseTokenId, amount: collectedFunds - withdrawAmount }, | |
| // NOTE: we DO NOT keep the foreign token on SELF; we will try to move it out | |
| ]; | |
| projectValue = SAFE_MIN_BOX_VALUE; | |
| projectAssets = [ | |
| { tokenId: ctx.baseTokenId, amount: projectAmount }, | |
| // BUG: Also extracting unrelated token to owner | |
| { tokenId: foreignTokenIdOnSelf, amount: foreignTokenAmountOnSelf }, | |
| ]; | |
| devFeeValue = SAFE_MIN_BOX_VALUE; | |
| devFeeAssets = [ | |
| { tokenId: ctx.baseTokenId, amount: devFeeAmount }, | |
| ]; | |
| } | |
| if (ctx.isErgMode) { | |
| // ERG mode: decrease ERG on SELF, move ERG to project/dev, and also move foreign token to project | |
| remainingValue = collectedFunds - withdrawAmount; // keep remainder on self | |
| remainingAssets = [ | |
| { tokenId: ctx.projectNftId, amount: projectBox.assets[0].amount }, // keep APT | |
| // NOTE: we DO NOT keep the foreign token on SELF; we will try to move it out | |
| ]; | |
| projectValue = projectAmount; | |
| projectAssets = [ | |
| // BUG: Also extracting unrelated token to owner in ERG mode | |
| { tokenId: foreignTokenIdOnSelf, amount: foreignTokenAmountOnSelf }, | |
| ]; | |
| devFeeValue = devFeeAmount; | |
| devFeeAssets = []; | |
| } else { | |
| // Token mode: keep half base tokens on SELF, move half to project/dev, and also move foreign token to project | |
| remainingValue = SAFE_MIN_BOX_VALUE; | |
| remainingAssets = [ | |
| { tokenId: ctx.projectNftId, amount: projectBox.assets[0].amount }, | |
| { tokenId: ctx.baseTokenId, amount: collectedFunds - withdrawAmount }, | |
| // NOTE: we DO NOT keep the foreign token on SELF; we will try to move it out | |
| ]; | |
| projectValue = SAFE_MIN_BOX_VALUE; | |
| projectAssets = [ | |
| { tokenId: ctx.baseTokenId, amount: projectAmount }, | |
| // BUG: Also extracting unrelated token to owner | |
| { tokenId: foreignTokenIdOnSelf, amount: foreignTokenAmountOnSelf }, | |
| ]; | |
| devFeeValue = SAFE_MIN_BOX_VALUE; | |
| devFeeAssets = [ | |
| { tokenId: ctx.baseTokenId, amount: devFeeAmount }, | |
| ]; | |
| } |
🤖 Prompt for AI Agents
In tests/contracts/withdraw_funds_unrelated_tokens.test.ts around lines 92 to
125, the ERG-mode branch leaves projectAssets empty so the foreign token on SELF
is not moved to the project output and the guard is never exercised; update the
ERG-mode branch to add the foreign token to projectAssets (e.g., include {
tokenId: foreignTokenIdOnSelf, amount: foreignTokenAmountOnSelf }) and ensure
remainingAssets does not include that foreign token so the test models sweeping
the foreign token to the project box just like the token-mode branch.
3 participants
Aummary
Introduces a guard in Bene Contract v2 to prevent withdrawing or interacting with foreign/non-core tokens on project boxes.
Ensures the contract adheres to the invariant that only the allowed core tokens (APT/PFT and base token per mode) are present during withdrawal and replication flows.
Motivation
Recent issues showed the contract allowed withdrawals when unrelated tokens were present, which could lead to unintended asset movements or state inconsistencies.
This PR tightens token invariants to protect project funds and maintain counters integrity.
Changes
Contract: tightened token invariants in Bene Contract v2 to disallow foreign tokens during withdrawal flow.
Minimal change: 1-line insertion in the contract to enforce the guard.
References
Contract file updated:
contract_v2.esTest Results
Buy Tokens suite: all tests passing (12/12).
Refund Tokens suite: failing cases remain.
Other suites were not fully re-run in this PR context; focus was on the foreign-token guard.
Known Issues
Refund path requires follow-up fixes:
This PR is intended to land the foreign-token guard first; a subsequent PR will address refund path failures.
How to Reproduce Failures
Run: npx vitest run tests/contracts/refund_tokens.test.ts --reporter=verbose
Alternatively: run without bail to collect all failures, or use --reporter=json for structured output.
Risk & Impact
Low risk to buy/exchange paths based on current green tests.
Refund paths currently impacted; merging as-is will keep those tests failing until follow-up fix lands.
Suggest marking this PR as Draft or merging with a clear note that refund fixes will follow immediately.
Follow-up Plan
Investigate refund constraints in Bene Contract v2:
Reviewer Checklist
Confirm the new foreign-token guard is correctly scoped to withdrawal/replication paths.
Validate no regressions in buy/exchange flows.
Review whether the guard should also be applied to other flows (unsold tokens withdrawal, funds withdrawal with minimumReached) for consistency.
Acknowledge the refund tests are failing and will be addressed next.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.