From 522dda9c1143ae336a4a0ff49746bf1591ecdced Mon Sep 17 00:00:00 2001 From: Marvin Kruse Date: Wed, 24 Jun 2026 00:56:50 +0200 Subject: [PATCH 1/2] Fix: Block blacklisted spender in blacklist issuance tokens 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. --- .../ERC20IssuanceUpgradeable_Blacklist_v1.sol | 20 +++++++ .../token/ERC20Issuance_Blacklist_v1.sol | 20 +++++++ ...RC20IssuanceUpgradeable_blacklist_v1.t.sol | 60 +++++++++++++++++++ .../token/ERC20Issuance_blacklist_v1.t.sol | 60 +++++++++++++++++++ 4 files changed, 160 insertions(+) diff --git a/src/external/token/ERC20IssuanceUpgradeable_Blacklist_v1.sol b/src/external/token/ERC20IssuanceUpgradeable_Blacklist_v1.sol index 89d909489..3e7c082d3 100644 --- a/src/external/token/ERC20IssuanceUpgradeable_Blacklist_v1.sol +++ b/src/external/token/ERC20IssuanceUpgradeable_Blacklist_v1.sol @@ -229,6 +229,26 @@ contract ERC20IssuanceUpgradeable_Blacklist_v1 is super._update(from_, to_, amount_); } + /// @notice Internal hook to also block a blacklisted spender from moving + /// tokens via an allowance. + /// @dev The `_update` hook only sees `from` and `to`, so without this a + /// blacklisted address holding an allowance could still spend it + /// through `transferFrom` (or any allowance-based spend). Checking + /// the spender here closes that path. + /// @param owner_ Address the allowance is drawn from. + /// @param spender_ Address spending the allowance. + /// @param value_ Amount being spent. + function _spendAllowance(address owner_, address spender_, uint value_) + internal + virtual + override + { + if (isBlacklisted(spender_)) { + revert ERC20Issuance_Blacklist_BlacklistedAddress(spender_); + } + super._spendAllowance(owner_, spender_, value_); + } + /// @notice Internal function to set a blacklist manager. /// @param manager_ Address to set as blacklist manager. /// @param allowed_ Whether to grant or revoke the blacklist manager role. diff --git a/src/external/token/ERC20Issuance_Blacklist_v1.sol b/src/external/token/ERC20Issuance_Blacklist_v1.sol index 9203dce94..ee15aba46 100644 --- a/src/external/token/ERC20Issuance_Blacklist_v1.sol +++ b/src/external/token/ERC20Issuance_Blacklist_v1.sol @@ -222,6 +222,26 @@ contract ERC20Issuance_Blacklist_v1 is super._update(from_, to_, amount_); } + /// @notice Internal hook to also block a blacklisted spender from moving + /// tokens via an allowance. + /// @dev The `_update` hook only sees `from` and `to`, so without this a + /// blacklisted address holding an allowance could still spend it + /// through `transferFrom` (or any allowance-based spend). Checking + /// the spender here closes that path. + /// @param owner_ Address the allowance is drawn from. + /// @param spender_ Address spending the allowance. + /// @param value_ Amount being spent. + function _spendAllowance(address owner_, address spender_, uint value_) + internal + virtual + override + { + if (isBlacklisted(spender_)) { + revert ERC20Issuance_Blacklist_BlacklistedAddress(spender_); + } + super._spendAllowance(owner_, spender_, value_); + } + /// @notice Internal function to set a blacklist manager. /// @param manager_ Address to set as blacklist manager. /// @param allowed_ Whether to grant or revoke the blacklist manager role. diff --git a/test/unit/external/token/ERC20IssuanceUpgradeable_blacklist_v1.t.sol b/test/unit/external/token/ERC20IssuanceUpgradeable_blacklist_v1.t.sol index d39db46a6..45105c0f7 100644 --- a/test/unit/external/token/ERC20IssuanceUpgradeable_blacklist_v1.t.sol +++ b/test/unit/external/token/ERC20IssuanceUpgradeable_blacklist_v1.t.sol @@ -954,6 +954,66 @@ contract ERC20IssuanceUpgradeable_Blacklist_v1_Test is Test { ); } + /* Test: blacklisted spender via transferFrom + ├── Given a holder approved a spender and the spender is blacklisted + │ └── When transferFrom is called by the spender + │ └── Then it should revert with BlacklistedAddress(spender) + └── Given the spender is not blacklisted + └── When transferFrom is called by the spender + └── Then it should move the holder's tokens + */ + + function testTransferFrom_revertGivenSpenderIsBlacklisted(uint amount_) + public + { + amount_ = bound(amount_, 1, uint(type(uint).max - 1)); + address holder = makeAddr("holder"); + address spender = makeAddr("spender"); + address dest = makeAddr("dest"); + + _fundAddress(holder, amount_); + vm.prank(holder); + token.approve(spender, amount_); + + // Blacklist the spender only. Holder and dest stay clean, so the + // `_update(from, to)` checks would pass; the spender check must catch it. + _blacklistAddress(spender); + + vm.prank(spender); + vm.expectRevert( + abi.encodeWithSelector( + IERC20Issuance_Blacklist_v1 + .ERC20Issuance_Blacklist_BlacklistedAddress + .selector, + spender + ) + ); + token.transferFrom(holder, dest, amount_); + + // Funds must not have moved. + assertEq(token.balanceOf(holder), amount_, "holder balance untouched"); + assertEq(token.balanceOf(dest), 0, "dest received nothing"); + } + + function testTransferFrom_worksGivenSpenderIsNotBlacklisted(uint amount_) + public + { + amount_ = bound(amount_, 1, uint(type(uint).max - 1)); + address holder = makeAddr("holder"); + address spender = makeAddr("spender"); + address dest = makeAddr("dest"); + + _fundAddress(holder, amount_); + vm.prank(holder); + token.approve(spender, amount_); + + vm.prank(spender); + token.transferFrom(holder, dest, amount_); + + assertEq(token.balanceOf(holder), 0, "holder fully spent"); + assertEq(token.balanceOf(dest), amount_, "dest received funds"); + } + // ================================================================================ // Helper Functions diff --git a/test/unit/external/token/ERC20Issuance_blacklist_v1.t.sol b/test/unit/external/token/ERC20Issuance_blacklist_v1.t.sol index a5fc01755..61a4d12b4 100644 --- a/test/unit/external/token/ERC20Issuance_blacklist_v1.t.sol +++ b/test/unit/external/token/ERC20Issuance_blacklist_v1.t.sol @@ -917,6 +917,66 @@ contract ERC20Issuance_Blacklist_v1_Test is Test { ); } + /* Test: blacklisted spender via transferFrom + ├── Given a holder approved a spender and the spender is blacklisted + │ └── When transferFrom is called by the spender + │ └── Then it should revert with BlacklistedAddress(spender) + └── Given the spender is not blacklisted + └── When transferFrom is called by the spender + └── Then it should move the holder's tokens + */ + + function testTransferFrom_revertGivenSpenderIsBlacklisted(uint amount_) + public + { + amount_ = bound(amount_, 1, uint(type(uint).max - 1)); + address holder = makeAddr("holder"); + address spender = makeAddr("spender"); + address dest = makeAddr("dest"); + + _fundAddress(holder, amount_); + vm.prank(holder); + token.approve(spender, amount_); + + // Blacklist the spender only. Holder and dest stay clean, so the + // `_update(from, to)` checks would pass; the spender check must catch it. + _blacklistAddress(spender); + + vm.prank(spender); + vm.expectRevert( + abi.encodeWithSelector( + IERC20Issuance_Blacklist_v1 + .ERC20Issuance_Blacklist_BlacklistedAddress + .selector, + spender + ) + ); + token.transferFrom(holder, dest, amount_); + + // Funds must not have moved. + assertEq(token.balanceOf(holder), amount_, "holder balance untouched"); + assertEq(token.balanceOf(dest), 0, "dest received nothing"); + } + + function testTransferFrom_worksGivenSpenderIsNotBlacklisted(uint amount_) + public + { + amount_ = bound(amount_, 1, uint(type(uint).max - 1)); + address holder = makeAddr("holder"); + address spender = makeAddr("spender"); + address dest = makeAddr("dest"); + + _fundAddress(holder, amount_); + vm.prank(holder); + token.approve(spender, amount_); + + vm.prank(spender); + token.transferFrom(holder, dest, amount_); + + assertEq(token.balanceOf(holder), 0, "holder fully spent"); + assertEq(token.balanceOf(dest), amount_, "dest received funds"); + } + // ================================================================================ // Helper Functions From b251bd80154cda45354057c8c5c9e3969f5612a2 Mon Sep 17 00:00:00 2001 From: Marvin Kruse Date: Wed, 24 Jun 2026 01:28:03 +0200 Subject: [PATCH 2/2] Test: Lock blacklisted-spender checks for infinite allowance and the 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. --- ...RC20IssuanceUpgradeable_blacklist_v1.t.sol | 82 +++++++++++++++++++ .../token/ERC20Issuance_blacklist_v1.t.sol | 82 +++++++++++++++++++ 2 files changed, 164 insertions(+) diff --git a/test/unit/external/token/ERC20IssuanceUpgradeable_blacklist_v1.t.sol b/test/unit/external/token/ERC20IssuanceUpgradeable_blacklist_v1.t.sol index 45105c0f7..d00a0d654 100644 --- a/test/unit/external/token/ERC20IssuanceUpgradeable_blacklist_v1.t.sol +++ b/test/unit/external/token/ERC20IssuanceUpgradeable_blacklist_v1.t.sol @@ -1014,6 +1014,88 @@ contract ERC20IssuanceUpgradeable_Blacklist_v1_Test is Test { assertEq(token.balanceOf(dest), amount_, "dest received funds"); } + // The blacklist check runs before OZ's infinite-allowance short-circuit, + // so an unlimited approval to a blacklisted spender is still blocked. + function testTransferFrom_revertGivenBlacklistedSpenderWithInfiniteAllowance( + uint amount_ + ) public { + amount_ = bound(amount_, 1, uint(type(uint).max - 1)); + address holder = makeAddr("holder"); + address spender = makeAddr("spender"); + address dest = makeAddr("dest"); + + _fundAddress(holder, amount_); + vm.prank(holder); + token.approve(spender, type(uint).max); + + _blacklistAddress(spender); + + vm.prank(spender); + vm.expectRevert( + abi.encodeWithSelector( + IERC20Issuance_Blacklist_v1 + .ERC20Issuance_Blacklist_BlacklistedAddress + .selector, + spender + ) + ); + token.transferFrom(holder, dest, amount_); + + assertEq(token.balanceOf(holder), amount_, "holder balance untouched"); + assertEq(token.balanceOf(dest), 0, "dest received nothing"); + } + + /* Test: minter spendAllowance wrapper also goes through the spender check + ├── Given the spender is blacklisted + │ └── When the minter calls spendAllowance() + │ └── Then it should revert with BlacklistedAddress(spender) + └── Given the spender is not blacklisted + └── When the minter calls spendAllowance() + └── Then it should consume the allowance + */ + + function testSpendAllowanceWrapper_revertGivenSpenderIsBlacklisted( + uint amount_ + ) public { + amount_ = bound(amount_, 1, uint(type(uint).max - 1)); + address holder = makeAddr("holder"); + address spender = makeAddr("spender"); + + vm.prank(holder); + token.approve(spender, amount_); + _blacklistAddress(spender); + + // Caller is the test contract, which is the minter (set in setUp). + vm.expectRevert( + abi.encodeWithSelector( + IERC20Issuance_Blacklist_v1 + .ERC20Issuance_Blacklist_BlacklistedAddress + .selector, + spender + ) + ); + token.spendAllowance(holder, spender, amount_); + + assertEq( + token.allowance(holder, spender), amount_, "allowance untouched" + ); + } + + function testSpendAllowanceWrapper_worksGivenSpenderIsNotBlacklisted( + uint amount_ + ) public { + amount_ = bound(amount_, 1, uint(type(uint).max - 1)); + address holder = makeAddr("holder"); + address spender = makeAddr("spender"); + + vm.prank(holder); + token.approve(spender, amount_); + + token.spendAllowance(holder, spender, amount_); + + assertEq(token.allowance(holder, spender), 0, "allowance consumed"); + } + // ================================================================================ // Helper Functions diff --git a/test/unit/external/token/ERC20Issuance_blacklist_v1.t.sol b/test/unit/external/token/ERC20Issuance_blacklist_v1.t.sol index 61a4d12b4..5974dcbf7 100644 --- a/test/unit/external/token/ERC20Issuance_blacklist_v1.t.sol +++ b/test/unit/external/token/ERC20Issuance_blacklist_v1.t.sol @@ -977,6 +977,88 @@ contract ERC20Issuance_Blacklist_v1_Test is Test { assertEq(token.balanceOf(dest), amount_, "dest received funds"); } + // The blacklist check runs before OZ's infinite-allowance short-circuit, + // so an unlimited approval to a blacklisted spender is still blocked. + function testTransferFrom_revertGivenBlacklistedSpenderWithInfiniteAllowance( + uint amount_ + ) public { + amount_ = bound(amount_, 1, uint(type(uint).max - 1)); + address holder = makeAddr("holder"); + address spender = makeAddr("spender"); + address dest = makeAddr("dest"); + + _fundAddress(holder, amount_); + vm.prank(holder); + token.approve(spender, type(uint).max); + + _blacklistAddress(spender); + + vm.prank(spender); + vm.expectRevert( + abi.encodeWithSelector( + IERC20Issuance_Blacklist_v1 + .ERC20Issuance_Blacklist_BlacklistedAddress + .selector, + spender + ) + ); + token.transferFrom(holder, dest, amount_); + + assertEq(token.balanceOf(holder), amount_, "holder balance untouched"); + assertEq(token.balanceOf(dest), 0, "dest received nothing"); + } + + /* Test: minter spendAllowance wrapper also goes through the spender check + ├── Given the spender is blacklisted + │ └── When the minter calls spendAllowance() + │ └── Then it should revert with BlacklistedAddress(spender) + └── Given the spender is not blacklisted + └── When the minter calls spendAllowance() + └── Then it should consume the allowance + */ + + function testSpendAllowanceWrapper_revertGivenSpenderIsBlacklisted( + uint amount_ + ) public { + amount_ = bound(amount_, 1, uint(type(uint).max - 1)); + address holder = makeAddr("holder"); + address spender = makeAddr("spender"); + + vm.prank(holder); + token.approve(spender, amount_); + _blacklistAddress(spender); + + // Caller is the test contract, which is the minter (set in setUp). + vm.expectRevert( + abi.encodeWithSelector( + IERC20Issuance_Blacklist_v1 + .ERC20Issuance_Blacklist_BlacklistedAddress + .selector, + spender + ) + ); + token.spendAllowance(holder, spender, amount_); + + assertEq( + token.allowance(holder, spender), amount_, "allowance untouched" + ); + } + + function testSpendAllowanceWrapper_worksGivenSpenderIsNotBlacklisted( + uint amount_ + ) public { + amount_ = bound(amount_, 1, uint(type(uint).max - 1)); + address holder = makeAddr("holder"); + address spender = makeAddr("spender"); + + vm.prank(holder); + token.approve(spender, amount_); + + token.spendAllowance(holder, spender, amount_); + + assertEq(token.allowance(holder, spender), 0, "allowance consumed"); + } + // ================================================================================ // Helper Functions