fix(tron): TRC-20 policy engine fails closed on missing constraints#595
Draft
neavra wants to merge 3 commits into
Draft
fix(tron): TRC-20 policy engine fails closed on missing constraints#595neavra wants to merge 3 commits into
neavra wants to merge 3 commits into
Conversation
validateTRC20Transfer iterated only the constraints present in the rule and never called validateTarget, so a tron.trc20.transfer rule that omitted the recipient (or amount/from_asset) constraint silently ALLOWED a transfer to any address for any amount — fail-open. The EVM engine fails closed (asserts target + requires a constraint per decoded arg); TRON did not. Require recipient+amount+from_asset to each be constrained before evaluating, else reject. The metarule always emits all three for legitimate rules (metarule_test asserts this), so only under-constrained policies are rejected. Adds engine/tron/trc20_failclosed_test.go regression test: zero/partial constraints rejected; fully-constrained+matching allowed; wrong recipient still caught by the value check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds fail-closed validation for TRON TRC-20 transfer rules. The engine now rejects rules that lack required parameter constraints ( ChangesTRC-20 fail-closed transfer validation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Team review found the presence gate alone wasn't enough: a recipient/from_asset constraint that is present but carries an empty FixedValue (and no magic constant) passed the gate, then the value loop's `!= ""` guard skipped enforcement — a wildcard, the same fail-open class one level down. Require each required constraint to carry an enforceable value. Adds two regression cases (empty recipient, empty from_asset). Follow-up (separate PR, out of scope here): TRC-20 binds the contract via the from_asset constraint rather than rule.target, unlike the EVM/TRX engines. Aligning it with the target model is a larger refactor (validateTarget reads tx.To(), which is empty for TriggerSmartContract, so it would need to target the contract address). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
engine/tron/trc20_failclosed_test.go (1)
100-151: ⚡ Quick winAdd explicit missing
amountand missingfrom_asset-presence cases.The table currently proves missing
recipient, but not the other two required keys individually. Add both to lock the gate semantics.Suggested additions
{ name: "missing recipient (amount+from_asset only) — rejected", constraints: []*types.ParameterConstraint{fixed("amount", "100000000"), fixed("from_asset", contractB58)}, wantErr: "missing required constraint", }, + { + name: "missing amount (recipient+from_asset only) — rejected", + constraints: []*types.ParameterConstraint{fixed("recipient", recipientB58), fixed("from_asset", contractB58)}, + wantErr: "missing required constraint", + }, + { + name: "missing from_asset (recipient+amount only) — rejected", + constraints: []*types.ParameterConstraint{fixed("recipient", recipientB58), fixed("amount", "100000000")}, + wantErr: "missing required constraint", + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/tron/trc20_failclosed_test.go` around lines 100 - 151, The test table `cases` in trc20_failclosed_test.go is missing explicit cases for a transaction missing the "amount" constraint and for a transaction missing the "from_asset" presence (only recipient+amount or recipient+from_asset provided respectively); add two entries to `cases` similar to the existing missing-recipient case that supply the other required fields but omit "amount" and omit "from_asset" respectively, and set their `wantErr` to "missing required constraint" so the gate semantics are fully exercised for each required parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engine/tron/trc20_failclosed_test.go`:
- Around line 64-67: The hex.DecodeString errors are being ignored in
trc20TransferCalldata (and the other hex.DecodeString calls later in this test
file), which can hide bad fixtures; update trc20TransferCalldata to check the
error returned from hex.DecodeString and propagate or fail on it (e.g., change
the function to return ([]byte, error) or call t.Fatalf in tests), and apply the
same pattern to the other hex.DecodeString usages in this file so decode
failures are handled rather than swallowed.
---
Nitpick comments:
In `@engine/tron/trc20_failclosed_test.go`:
- Around line 100-151: The test table `cases` in trc20_failclosed_test.go is
missing explicit cases for a transaction missing the "amount" constraint and for
a transaction missing the "from_asset" presence (only recipient+amount or
recipient+from_asset provided respectively); add two entries to `cases` similar
to the existing missing-recipient case that supply the other required fields but
omit "amount" and omit "from_asset" respectively, and set their `wantErr` to
"missing required constraint" so the gate semantics are fully exercised for each
required parameter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5cf40ab8-ce99-4e9c-8b5a-9eb84274a46d
📒 Files selected for processing (2)
engine/tron/trc20_failclosed_test.goengine/tron/tron.go
Comment on lines
+64
to
+67
| func trc20TransferCalldata(recipient20Hex, amountHex64 string) []byte { | ||
| recipient32 := strings.Repeat("0", 64-len(recipient20Hex)) + recipient20Hex | ||
| cd, _ := hex.DecodeString("a9059cbb" + recipient32 + amountHex64) | ||
| return cd |
Contributor
There was a problem hiding this comment.
Stop swallowing fixture decode errors.
Line 66 and Lines 83-84 ignore hex.DecodeString errors, which can turn bad fixtures into misleading downstream failures.
Suggested fix
import (
+ "fmt"
"encoding/hex"
"strings"
"testing"
@@
-func trc20TransferCalldata(recipient20Hex, amountHex64 string) []byte {
+func trc20TransferCalldata(recipient20Hex, amountHex64 string) ([]byte, error) {
recipient32 := strings.Repeat("0", 64-len(recipient20Hex)) + recipient20Hex
- cd, _ := hex.DecodeString("a9059cbb" + recipient32 + amountHex64)
- return cd
+ cd, err := hex.DecodeString("a9059cbb" + recipient32 + amountHex64)
+ if err != nil {
+ return nil, fmt.Errorf("decode TRC-20 calldata: %w", err)
+ }
+ return cd, nil
}
@@
- owner, _ := hex.DecodeString("41a614f803b6fd780986a42c78ec9c7f77e6ded13c")
- contract, _ := hex.DecodeString("41a614f803b6fd780986a42c78ec9c7f77e6ded13d")
+ owner, err := hex.DecodeString("41a614f803b6fd780986a42c78ec9c7f77e6ded13c")
+ if err != nil {
+ t.Fatalf("owner decode: %v", err)
+ }
+ contract, err := hex.DecodeString("41a614f803b6fd780986a42c78ec9c7f77e6ded13d")
+ if err != nil {
+ t.Fatalf("contract decode: %v", err)
+ }
@@
- txBytes := buildTronTRC20Tx(owner, contract, trc20TransferCalldata(recipient20, amount))
+ calldata, err := trc20TransferCalldata(recipient20, amount)
+ if err != nil {
+ t.Fatalf("calldata decode: %v", err)
+ }
+ txBytes := buildTronTRC20Tx(owner, contract, calldata)Also applies to: 83-84, 87-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/tron/trc20_failclosed_test.go` around lines 64 - 67, The
hex.DecodeString errors are being ignored in trc20TransferCalldata (and the
other hex.DecodeString calls later in this test file), which can hide bad
fixtures; update trc20TransferCalldata to check the error returned from
hex.DecodeString and propagate or fail on it (e.g., change the function to
return ([]byte, error) or call t.Fatalf in tests), and apply the same pattern to
the other hex.DecodeString usages in this file so decode failures are handled
rather than swallowed.
Lock in that each required TRC-20 constraint is individually mandatory, not just recipient. Cheap completeness improvement flagged by CodeRabbit + the tests reviewer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai Addressed in the latest commit — added explicit |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
(◕‿◕✿) 🐇 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
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
1 participant
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.
Problem
engine/tron/tron.govalidateTRC20Transferiterated only the constraints present in the rule and never calledvalidateTargetfor the TRC-20 (TriggerSmartContract) branch. So atron.trc20.transferrule that omitted therecipient(oramount/from_asset) constraint silently allowed a transfer to any address for any amount — the engine failed open.The EVM engine fails closed for the equivalent case (asserts the target unconditionally + requires a constraint per decoded arg). TRON TRC-20 was the outlier.
This was found by an audit loop and independently reproduced before fixing.
Fix
Require
recipient+amount+from_assetto each be constrained before evaluating a TRC-20 transfer; otherwise reject. Verified safe against the metarule, which always emits all three constraints for legitimatetron.trc20.transferrules (asserted inmetarule_test) — so only under-constrained policies are rejected, no legitimate policy breaks.Test
engine/tron/trc20_failclosed_test.go— 4 cases, all passing:No regressions:
engine/tron,metarule,engine/evmsuites all pass.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests