Fix ERC20uRWA canTransfer/forcedTransfer restriction checks#249
Fix ERC20uRWA canTransfer/forcedTransfer restriction checks#249mdimran29 wants to merge 1 commit into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. 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:
WalkthroughERC20uRWA now exposes separate send and receive permission checks, uses them for transfer validation, and applies receive checks to forced transfers. A configurable mock and tests cover default, asymmetric, balance-independent, and forced-transfer behaviors. ChangesERC20uRWA permissions
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
canTransfer incorrectly bundled a balance check into an availability query, and forcedTransfer gated recipients with canTransact instead of a receive-specific check. Add virtual canSend/canReceive (defaulting to canTransact) so send- and receive-side restrictions can be overridden independently, use them in canTransfer, and gate forcedTransfer's recipient with canReceive.
265c1d4 to
924ec70
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/token/ERC20/extensions/ERC20uRWA.sol (1)
117-127: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftEnforce
canSendandcanReceiveduring actual transfers and verify with tests.The PR introduces independent
canSendandcanReceivepermissions but does not enforce them during the actual token transfer lifecycle. Currently, relying on the inherited_updatechain only evaluatescanTransact(from)andcanTransact(to). If a derived contract overridescanSendorcanReceive, external queries tocanTransferwill correctly reflect these overrides, but actual token transfers will completely bypass them and succeed.
contracts/token/ERC20/extensions/ERC20uRWA.sol#L117-L127: Ensure_updateexplicitly evaluatescanSend(from)(if not minting) andcanReceive(to)(if not burning) instead of relying solely onERC20Restricted's genericcanTransactchecks. Note thatforcedTransferwill also need a mechanism (like a transient bypass flag) to skip these new checks since it currently bypasses restrictions by modifyingcanTransactstate, which would fail to bypass an overriddencanSend.test/token/ERC20/extensions/ERC20uRWA.t.sol#L90-L112: Add assertions usingvm.expectRevert()to execute actual transfers (token.transferortoken.transferFrom) and verify that the mocked send/receive restrictions are genuinely enforced at runtime, preventing the movement of tokens.🤖 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 `@contracts/token/ERC20/extensions/ERC20uRWA.sol` around lines 117 - 127, Update ERC20uRWA._update to enforce canSend(from) for non-mints and canReceive(to) for non-burns, while preserving forcedTransfer’s ability to bypass these checks through an appropriate transient bypass mechanism; retain the inherited restrictions where applicable. In test/token/ERC20/extensions/ERC20uRWA.t.sol lines 90-112, add vm.expectRevert() cases that perform actual transfer or transferFrom calls and verify mocked send and receive restrictions prevent token movement.
🤖 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.
Outside diff comments:
In `@contracts/token/ERC20/extensions/ERC20uRWA.sol`:
- Around line 117-127: Update ERC20uRWA._update to enforce canSend(from) for
non-mints and canReceive(to) for non-burns, while preserving forcedTransfer’s
ability to bypass these checks through an appropriate transient bypass
mechanism; retain the inherited restrictions where applicable. In
test/token/ERC20/extensions/ERC20uRWA.t.sol lines 90-112, add vm.expectRevert()
cases that perform actual transfer or transferFrom calls and verify mocked send
and receive restrictions prevent token movement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d805fd4e-03ae-46dd-a58b-a52d0911760d
📒 Files selected for processing (3)
contracts/token/ERC20/extensions/ERC20uRWA.soltest/token/ERC20/extensions/ERC20uRWA.t.soltest/token/ERC20/extensions/ERC20uRWAMock.t.sol
Summary
Fixes #244 (points 3 & 4).
canSend(address)andcanReceive(address), both defaulting tocanTransact(), so send-side and receive-side restrictions can be overridden independently.canTransfer(): remove theamount <= available(from)balance check (acanTransferavailability query shouldn't also encode balance sufficiency) and checkcanSend(from) && canReceive(to)instead ofcanTransact(from) && canTransact(to).forcedTransfer(): gate the recipient withcanReceive(to)instead ofcanTransact(to), consistent withcanTransfer.No changes to
setFrozenTokens, theforcedTransferfrom == tohandling, or any other files.Test plan
forge buildpassesforge testpasses (41/41, including 11 new tests intest/token/ERC20/extensions/ERC20uRWA.t.sol)canTransferreturnstrueeven whenamountexceeds available/frozen balancecanTransferreturnsfalsewhencanSendorcanReceiveis falseforcedTransferreverts whencanReceiveis false