feat(tip-1028): implement address-level receive policies#3800
feat(tip-1028): implement address-level receive policies#3800
Conversation
|
0xrusowsky
left a comment
0xrusowsky
left a comment
There was a problem hiding this comment.
overall i think this is a great start.
i think we should lean more into the Recipient struct to simplify and offload some of the cross-precompile logic out of the TIP20 transfer/mint paths, similar to what we did with virtual addresses.
i'll open a draft with a proposal to build on top of this foundation
0xKitsune
force-pushed
the
kit/tip-1028-impl
branch
from
dd23bcf to
7916070
Compare
0xKitsune
force-pushed
the
kit/tip-1028-impl
branch
from
1f764de to
b90d340
Compare
📊 Tempo Precompiles CoverageprecompilesCoverage: 5683/8142 lines (69.80%) File details
contractsCoverage: 1/271 lines (0.37%) File details
Total: 5684/8413 lines (67.56%) |
0xKitsune
requested review from
SuperFluffy,
fgimenez,
hamdiallam,
joshieDo,
klkvr,
legion2002 and
mattsse
as code owners
| } | ||
|
|
||
| function blockedReceiptBalance(address token, address recoveryContract, uint8 receiptVersion, bytes calldata receipt) external view returns (uint256 amount); | ||
| function claimBlocked(address token, address recoveryContract, uint8 receiptVersion, bytes calldata receipt, address to) external; |
There was a problem hiding this comment.
nit: should we rename to simply claim?
| return Err(TIP1028EscrowError::invalid_receipt_claim().into()); | ||
| } | ||
|
|
||
| let guard = self.storage.checkpoint(); |
There was a problem hiding this comment.
why do we need to checkpoint here? afaict any error after this point propagates to the precompile boundary, so revm would revert the whole frame (as usual).
this is only necessary when there's a caller that swallows the error and continues, which doesn't seem to be the case
| let to = Recipient::resolve(call.to)?; | ||
| self._mint(msg_sender, &to, call.amount)?; | ||
| self.check_role(msg_sender, *ISSUER_ROLE)?; | ||
| self.validate_mint(&to)?; |
There was a problem hiding this comment.
this is technically breaking if validate_mint() fails:
BEFORE inside fn _mint():
1. check_role(..)?;
2. total_supply()?; // SLOAD
3. validate_mint(..)?;
NOW
1. check_role(..)?;
2. validate_mint(..)?;
3. total_supply()?;
that's in my previous proposal to enhance Receiver i was performing the inbound validation inside _mint()
| let recovery = registry.receive_policy_recovery(to.target)?; | ||
| let recipient = to.virtual_addr.unwrap_or(to.target); | ||
|
|
||
| let guard = self.storage.checkpoint(); |
There was a problem hiding this comment.
why do we checkpoint here? same question as before, afaik we always propagate errors?
|
|
||
| /// Validates the TIP-1028 receive-policy check for the destination address. If the receive | ||
| /// policy prohibits the action, the funds are escrowed. | ||
| fn validate_or_escrow_funds( |
There was a problem hiding this comment.
nits:
- can we document what when the return type means "whether the funds were escrowed or not", and explicitly explain that the caller should not move the funds if that's the case?
- can we maybe rename the fn to indicate what we are validating? personally i like
validate_inbound_or_escrowbut if we want a shorter version we could also usevalidate_inboundorvalidate_receive_policies
There was a problem hiding this comment.
the refactor with this function is an elegant way to simplify, i really like it.
we only need to fix the re-execution issue on mints, but this is great.
| /// Account receive policy configuration. | ||
| address_receive_config: Mapping<Address, ReceivePolicyConfig>, |
There was a problem hiding this comment.
nit: can we keep consistent naming with pre-existing vars and use "policy"?
| /// Account receive policy configuration. | |
| address_receive_config: Mapping<Address, ReceivePolicyConfig>, | |
| /// Account-level receive policy configuration. | |
| receive_policy_config: Mapping<Address, ReceivePolicyConfig>, | |
| } |
| /// Returns `account`'s receive-policy configuration. | ||
| pub fn receive_policy( | ||
| &self, | ||
| call: ITIP403Registry::receivePolicyCall, |
There was a problem hiding this comment.
let's flatten the call and take account: Address directly
| msg_sender: Address, | ||
| call: ITIP403Registry::setReceivePolicyCall, | ||
| ) -> Result<()> { | ||
| if msg_sender == ESCROW_ADDRESS { |
There was a problem hiding this comment.
shouldn't this also verify the recovery address?
it shouldn't be a TIP20 nor the ESCROW precompile
There was a problem hiding this comment.
this requires a PR on its own.
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
Audited at c3b5f75778de; head drift detected. Findings re-checked against the current head c131127. The previously-flagged "missing T6 bytecode deploy for ESCROW_ADDRESS" callout has been fixed in this PR (crates/evm/src/block.rs:457-462 + new test_apply_pre_execution_deploys_escrow_code) and is dropped.
Body-only finding (target file is in the PR but the affected lines are outside the diff)
🚨 [SECURITY] burn_blocked permanently destroys escrowed funds — ESCROW_ADDRESS missing from protected-address allow-list
Severity: Medium
File: crates/precompiles/src/tip20/mod.rs:537 (existing burn_blocked guard, unchanged in this PR) interacting with new escrow custody at crates/precompiles/src/tip20/mod.rs:1073-1118
After T6, validate_or_escrow_funds parks blocked transfers/mints at ESCROW_ADDRESS. burn_blocked only protects TIP_FEE_MANAGER_ADDRESS | STABLECOIN_DEX_ADDRESS; ESCROW_ADDRESS is not in the list. For tokens whose transfer policy treats ESCROW_ADDRESS as unauthorized as a sender (REJECT_ALL_POLICY_ID, custom whitelists that omit escrow, blacklists that include it — exactly the tokens most likely to use TIP-1028), an account holding BURN_BLOCKED_ROLE can call burn_blocked(ESCROW_ADDRESS, amount) and zero out custody backing unrelated user receipts. Subsequent claimBlocked calls then revert with InsufficientEscrowBalance. Verified at the current head — the guard is still:
if matches!(call.from, TIP_FEE_MANAGER_ADDRESS | STABLECOIN_DEX_ADDRESS) {
return Err(TIP20Error::protected_address().into());
}Recommended Fix: Add ESCROW_ADDRESS to the protected list and mirror test_unable_to_burn_blocked_from_protected_address for it.
Reviewer Callouts
- ⚡ Other system-address allow-lists: Grep every site that special-cases
TIP_FEE_MANAGER_ADDRESS/STABLECOIN_DEX_ADDRESSand decide whetherESCROW_ADDRESSbelongs there too (snapshot tools, fee accounting invariants, admin levers). The verified impactful site isburn_blocked, but a sweep is the right hygiene. - ⚡ Recovery key compromise of an existing receipt is irrevocable:
receipt_keybindsrecovery_addressat store time. If the receiver later rotatesrecovery_addressaway from a compromised contract, receipts already in escrow remain bound to the old recovery. Confirm intended TIP-1028 semantics. - ⚡ DEX swap output silently routes to escrow:
swap_exact_amount_in/out,withdraw, and other DEX paths delivertoken_outtosenderviaTIP20Token::transfer(DEX, sender, amount). After T6, ifsenderconfigured a receive policy that rejects the DEX address, the output goes to escrow but the DEX functions still returnOk(amount). Composing contracts will operate on stale balances. Spec/UX call. - ⚡ Mint / TransferWithMemo events suppressed on escrow path: When mint/transfer-with-memo escrow-routes, only
Transfer(*, ESCROW, *)fires (plusTransferBlockedfor transfers). Off-chain indexers tracking supply/memos must combine TIP-1028 events to reconstruct intent. - ⚡ Stale (resolved): "Missing T6 bytecode deploy for
ESCROW_ADDRESS" — fixed in the current head; no action.
|
|
||
| /// Releases escrowed funds to `to`. Self-recovery skips policy checks. Redirects | ||
| /// revalidate the transfer and receive policies and meter the spending limit when set. | ||
| pub(crate) fn release_from_escrow( |
There was a problem hiding this comment.
release_from_escrow ignores token pause state
Existing TIP-20 explicitly documents the pause invariant in transfer_fee_pre_tx: "no other token transfers can occur after a pause event" except transfer_fee_post_tx refunds. validate_transfer, _burn, validate_mint, transfer_fee_pre_tx, and reward-claim paths all call check_not_paused. release_from_escrow does not — so claims and redirects move funds even while the token is paused. This silently breaks the issuer's incident-response guarantee for already-escrowed funds.
Recommended Fix: Either gate release_from_escrow on self.check_not_paused()? at entry, or — if escrow release is intentionally a second pause exception — document it next to the existing fee-refund exception comment and add a regression test that exercises the paused-claim case.
| let destination = Recipient::resolve(to)?; | ||
| destination.validate()?; | ||
|
|
||
| if destination.target != receiver { |
There was a problem hiding this comment.
This branch checks only that destination is recipient-authorized under the token transfer policy and that the destination's receive policy accepts originator as the sender. It does not check that receiver is sender-authorized under the token transfer policy. With a compound policy that whitelists only {originator} as sender and {receiver, destination} as recipients, a normal transfer(receiver → destination) reverts with PolicyForbids, but routing the same value through escrow + claimBlocked(to=destination) succeeds — turning TIP-1028 release into a compliance/freeze bypass for blocked funds.
Metering further uses receiver as the actor (line 1150) while validate_receive_policy is called with originator as the sender — three distinct identities are treated as "the sender" along the redirect path.
Recommended Fix: Make redirects policy-equivalent to receiver → destination:
if destination.target != receiver {
self.ensure_transfer_authorized(receiver, destination.target)?; // sender-side check on receiver
TIP403Registry::new()
.validate_receive_policy(self.address, receiver, destination.target)?;
if meter_spending_limit {
self.check_and_update_spending_limit(receiver, amount)?;
}
}or document explicitly in the TIP-1028 spec why redirects are intentionally weaker than direct transfers.
💡 [SUGGESTION] Mint redirected via escrow downgrades from mint_recipient to recipient policy
is_authorized_as(.., destination.target, AuthRole::recipient()) is used here even for receipts whose kind == MINT. The original mint was gated by validate_mint's AuthRole::mint_recipient() check, so a claimBlocked redirect of an escrowed mint can deliver freshly-minted units to an address that is permitted as a regular recipient but not as a mint recipient. Probably acceptable (the mint is "complete" at custody time), but it changes the TIP-403 invariant slightly — call out in the TIP-1028 spec or, if mint_recipient is meant to gate the delivery address, preserve kind in the receipt and use mint_recipient when redirecting MINT receipts.
There was a problem hiding this comment.
i previously discussed this with @malleshpai and i believe current impl is correct, and this would be overreaching with validations. however, it would be nice if we got an explicit ACK that we can disregard this concern
| } | ||
|
|
||
| /// Sets the caller's TIP-1028 receive policy. | ||
| pub fn set_receive_policy( |
There was a problem hiding this comment.
💡 [SUGGESTION] setReceivePolicy does not validate recoveryAddress
This function rejects msg_sender == ESCROW_ADDRESS and msg_sender.is_virtual() but applies no analogous check on call.recoveryAddress. A user can set recoveryAddress to ESCROW_ADDRESS, a TIP-20-prefixed address, a virtual address, or any other address that can never appear as the msg_sender of a claim_blocked call — any receipts later created with that recovery become permanently unclaimable (the only auth branch is msg_sender == recovery_address). Combined with the recovery=self spending-limit bypass flagged above, validating this field is the cleanest defense.
Recommended Fix: Reject recoveryAddress == ESCROW_ADDRESS, recoveryAddress.is_tip20(), recoveryAddress.is_virtual(), and (per the security finding) recoveryAddress == msg_sender. Keep Address::ZERO allowed as the self-recovery sentinel.
proposed fix |
3 participants
This PR Implements TIP-1028 receive policies, token filters, and receipt based escrow for blocked TIP-20 transfers and mints.
Changes are gated behind the T6 hardfork. Unit and e2e coverage is also added for receipt lifecycle, claim authorization, recovery address flows, and escrow balance accounting.
Ref #3791