Fix: Block blacklisted spender in blacklist issuance tokens#781
Open
marvinkruse wants to merge 2 commits into
Open
Fix: Block blacklisted spender in blacklist issuance tokens#781marvinkruse wants to merge 2 commits into
marvinkruse wants to merge 2 commits into
Conversation
The blacklist was enforced only in the _update(from, to, amount) hook, which never sees the spender. OZ transferFrom runs _spendAllowance(owner, spender) then _update(owner, to), so a blacklisted address holding a live allowance could still spend it to move a non-blacklisted holder's tokens. The blacklist did not cover the spender/operator. Override _spendAllowance in both ERC20IssuanceUpgradeable_Blacklist_v1 and ERC20Issuance_Blacklist_v1 to revert when the spender is blacklisted. This is the single chokepoint for every allowance-consuming path (transferFrom and the minter spendAllowance wrapper), so one override covers them all. Logic-only, no storage change. Adds regression tests that fail on the pre-fix code.
…spendAllowance wrapper Adds cases asserting the _spendAllowance chokepoint also blocks a blacklisted spender on an infinite (type(uint).max) approval and via the minter spendAllowance wrapper. Both revert cases fail on the pre-fix code.
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
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.
Summary
Closes a completeness gap in the blacklist on our two blacklist issuance tokens,
ERC20IssuanceUpgradeable_Blacklist_v1andERC20Issuance_Blacklist_v1.The blacklist was enforced only in the
_update(from, to, amount)hook, which sees the sender and the receiver but never the spender. OpenZeppelin'stransferFromruns_spendAllowance(owner, spender, value)and then_update(owner, to, value), so a blacklisted address holding a live allowance could still calltransferFromand move a non-blacklisted holder's tokens to a fresh address. The blacklist did not cover the spender/operator, unlike canonical blacklist stablecoins which also gate the spender in the allowance path.Fix
Override
_spendAllowancein both contracts to revert when the spender is blacklisted, then delegate tosuper:_spendAllowanceis the single internal chokepoint for every allowance-consuming path (transferFromand the minterspendAllowancewrapper), so one override covers them all. The check runs before OpenZeppelin's infinite-allowance short-circuit, so an unlimited approval to a blacklisted spender is blocked too. The change is logic-only with no storage added or reordered, so it is safe for the upgradeable variant behind its proxy.approveis intentionally not gated: allowing an approval to a blacklisted spender is harmless once the spend itself is blocked. Gating it would be optional defense-in-depth, out of scope here.Severity
Reported privately by an external security researcher. Low severity and bounded: it only reaches funds where an allowance already exists, the holder must not be blacklisted (a blacklisted holder's own balance stays frozen), and arbitrary holders cannot be touched. No funds are currently at risk. The value is restoring the blacklist as a complete freeze, including the case where a blacklisted contract holds allowances and you are blacklisting it to contain an incident.
Changes
src/external/token/ERC20IssuanceUpgradeable_Blacklist_v1.sol: add_spendAllowanceoverride.src/external/token/ERC20Issuance_Blacklist_v1.sol: add_spendAllowanceoverride.transferFromreverts and moves nothing, a clean spender'stransferFromstill works. Both revert tests fail on the pre-fix code.Deployment Steps
TransparentUpgradeableProxyinstances, so this is a proxy implementation upgrade, not a Governor beacon upgrade, and it is not live on any instance until each proxy is upgraded.ProxyAdminowner upgrades it to the new implementation. No reinitialization is needed (logic-only change).After each upgrade, verify the proxy's implementation slot points to the new implementation and that a blacklisted-spender
transferFromreverts on the live instance.Open item: there is no single factory registry for these tokens (they are deployed per workflow), so the full inventory of live instances must be assembled before rollout is considered complete.
Test Plan
Pre-merge checklist
forge buildsucceedsforge test --match-contract Blacklist_v1_Testpasses (both token suites)Post-deployment verification
transferFromreverts and a clean spender's still succeeds.