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..d00a0d654 100644 --- a/test/unit/external/token/ERC20IssuanceUpgradeable_blacklist_v1.t.sol +++ b/test/unit/external/token/ERC20IssuanceUpgradeable_blacklist_v1.t.sol @@ -954,6 +954,148 @@ 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"); + } + + // 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 a5fc01755..5974dcbf7 100644 --- a/test/unit/external/token/ERC20Issuance_blacklist_v1.t.sol +++ b/test/unit/external/token/ERC20Issuance_blacklist_v1.t.sol @@ -917,6 +917,148 @@ 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"); + } + + // 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