fix(smart-contracts): address stranded balance issues for ERC20#1741
Conversation
Greptile SummaryThis PR fixes a stranded-balance bug in
Confidence Score: 5/5Safe to merge — the tare-balance snapshot is taken at the right point in the call sequence, the refund and batch-fee arithmetic remain correct, and the new rescue function is owner-gated with a proper success check. The change is surgical: a single subtraction in the refund formula, a new struct field, and an admin helper. The core logic is well-tested with a dedicated regression test that covers both the fee-limit and no-limit code paths. The only gap is a missing zero-address guard on the rescue function's recipient parameter, which is a low-risk omission in an owner-only function. packages/smart-contracts/src/contracts/BatchNoConversionPayments.sol — the Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Payer
participant BatchConversion
participant ConversionProxy
participant FeeAddress
Note over BatchConversion: getUTokens() called<br/>tareBalance = balanceOf(this) recorded here
Payer->>BatchConversion: transferFrom(maxToSpend)
BatchConversion->>ConversionProxy: transferFromWithReferenceAndFee(...)
ConversionProxy-->>BatchConversion: (spends reallySpent tokens)
Note over BatchConversion: excessAmount = balanceOf(this) - tareBalance<br/>(stranded tokens excluded)
BatchConversion->>Payer: safeTransfer(excessAmount)
Payer->>FeeAddress: transferFrom(batchFee on reallySpent)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Payer
participant BatchConversion
participant ConversionProxy
participant FeeAddress
Note over BatchConversion: getUTokens() called<br/>tareBalance = balanceOf(this) recorded here
Payer->>BatchConversion: transferFrom(maxToSpend)
BatchConversion->>ConversionProxy: transferFromWithReferenceAndFee(...)
ConversionProxy-->>BatchConversion: (spends reallySpent tokens)
Note over BatchConversion: excessAmount = balanceOf(this) - tareBalance<br/>(stranded tokens excluded)
BatchConversion->>Payer: safeTransfer(excessAmount)
Payer->>FeeAddress: transferFrom(batchFee on reallySpent)
Reviews (6): Last reviewed commit: "Update packages/smart-contracts/test/con..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
❌ Echidna Fuzzing ResultsMode: ( test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
github-actions
Bot
requested review from
LeoSlrRf,
MantisClone,
dave-rf and
rodrigopavezi
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
rodrigopavezi
left a comment
rodrigopavezi
left a comment
There was a problem hiding this comment.
hey, nice fix! the tareBalance approach is clean and the test coverage looks solid.
quick note — I checked BatchNoConversionPayments and it doesn't have excess refund logic, so the same stranded-balance bug doesn't apply there. the tareBalance field is currently only consumed by BatchConversionPayments, which is fine.
a few things I noticed inline:
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
Thanks @rodrigopavezi for your in-depth review. I addressed all your feedback. About the choice for implementing |
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
MantisClone
left a comment
MantisClone
left a comment
There was a problem hiding this comment.
Optimistically Approved 👍 pending comment resolution 🚧
Nice work. The stranded-balance fix makes sense to me, and the rescue path looks scoped correctly.
I left one small test comment: the sanity check should flip the payer-balance subtraction so it actually proves the spend was below maxToSpend. Once that is fixed, this looks good to me.
…s.test.ts Co-authored-by: MantisClone <david.huntmateo@request.network>
✅ Slither Security AnalysisStatus: Passed Findings Summary
📄 Full report available in workflow artifacts. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
✅ Echidna Fuzzing ResultsMode: ci (50000 test sequences) Property Test Results
📄 Full report and corpus available in workflow artifacts. ℹ️ About Echidna FuzzingEchidna is a property-based fuzzer that generates random sequences of transactions Properties tested:
|
yomarion
deleted the
smart-contracts/batch-conversion-0.2-0-stranded-balance
branch
4 participants
Description of the changes
Fixes https://github.com/RequestNetwork/private-issues/issues/23
The
BatchConversionContractRefunds ERC20 tokens not based on his balance but based on what was topped up by the payer at the beginning of the transaction.New method for the contract owner:
rescueERC20Funds.