Skip to content

refactor(asset): remove batchBurn and BURN_FROM_ROLE (BOP-250) #117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ilikesymmetry merged 2 commits into from
Jun 2, 2026
Merged

refactor(asset): remove batchBurn and BURN_FROM_ROLE (BOP-250) #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06e042f
refactor(asset): remove batchBurn and BURN_FROM_ROLE (BOP-250)
ilikesymmetry Jun 2, 2026
91f1342
fix(test): include REDEEM_SENDER_POLICY in _isKnownPolicyType fuzz fi…
ilikesymmetry Jun 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions docs/B20/Security.md
View file
Open in desktop
Copy link
Copy Markdown
Collaborator Author

@ilikesymmetry ilikesymmetry Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore docs for now, will get big rewrite

Copy link
Copy Markdown
Member

@stevieraykatz stevieraykatz Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you link to where we're tracking this? seems like we'll need to take a fine-toothed anti-Security comb to the whole repo

Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,17 @@ IB20Security(token).announce({
});
```

The four corporate-actions setters should be wrapped in `announce()`:
The three corporate-actions setters should be wrapped in `announce()`:

- `updateShareRatio(...)`
- `batchMint(...)`
- `batchBurn(...)`
- `updateSecurityIdentifier(...)`

Direct invocation by a role holder is permitted as an **emergency override** — it succeeds but produces no bracket events. Suitable only for break-glass scenarios where the inability to emit an announcement is itself part of the response.

## Batch Mint/Burn
## Batch Mint

`batchMint(recipients, amounts)` mints to many accounts in one call, gated by `MINT_ROLE`. `batchBurn(holders, amounts)` burns from many accounts in one call, gated by `BURN_FROM_ROLE`. Both should be wrapped in `announce()`, which additionally requires the operator to hold `SECURITY_OPERATOR_ROLE` (typically granted as a single bundle).
`batchMint(recipients, amounts)` mints to many accounts in one call, gated by `MINT_ROLE`. It should be wrapped in `announce()`, which additionally requires the operator to hold `SECURITY_OPERATOR_ROLE` (typically granted as a single bundle).

## Redemptions

Expand All @@ -71,11 +70,7 @@ Each Security token can carry one or more standardized identifiers (ISIN, CUSIP,

### `SECURITY_OPERATOR_ROLE`

Gates the four corporate-actions setters (`updateShareRatio`, `batchMint`, `batchBurn`, `updateSecurityIdentifier`) and the `announce` wrapper itself. Held separately from `DEFAULT_ADMIN_ROLE` so corporate-actions operators don't need full admin authority. Operationally paired with `METADATA_ROLE` — when granting one, you typically grant the other to the same address.

### `BURN_FROM_ROLE`

Gates `batchBurn`, which burns balances held by other accounts as part of an announced corporate action. Distinct from `BURN_ROLE` (caller burns their own balance) and `BURN_BLOCKED_ROLE` (sanctions-seizure against policy-blocked addresses) — three burn primitives serve three different operational scenarios.
Gates the three corporate-actions setters (`updateShareRatio`, `batchMint`, `updateSecurityIdentifier`) and the `announce` wrapper itself. Held separately from `DEFAULT_ADMIN_ROLE` so corporate-actions operators don't need full admin authority. Operationally paired with `METADATA_ROLE` — when granting one, you typically grant the other to the same address.

## Fixed Decimals (6)

Expand Down
23 changes: 2 additions & 21 deletions src/interfaces/IB20Security.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {IB20} from "./IB20.sol";
/// @author Coinbase
///
/// @notice A B-20 token variant for tokenized securities. Extends `IB20` with announcements,
/// share-ratio accounting, batched mint/burn for corporate actions, holder-initiated
/// share-ratio accounting, batched mint for corporate actions, holder-initiated
/// redemption, and security-identifier metadata.
interface IB20Security is IB20 {
/*//////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -80,10 +80,6 @@ interface IB20Security is IB20 {
/// @return Role identifier.
function SECURITY_OPERATOR_ROLE() external view returns (bytes32);

/// @notice Required to call `batchBurn`.
/// @return Role identifier.
function BURN_FROM_ROLE() external view returns (bytes32);

/*//////////////////////////////////////////////////////////////
PRECISION
//////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -164,7 +160,7 @@ interface IB20Security is IB20 {
function updateShareRatio(uint256 newSharesToTokensRatio) external;

/*//////////////////////////////////////////////////////////////
BATCHED ISSUANCE AND CORP-ACTION CLAWBACK
BATCHED ISSUANCE
//////////////////////////////////////////////////////////////*/

/// @notice Mints `amounts[i]` to `recipients[i]` in one call. All-or-nothing: any element
Expand All @@ -183,21 +179,6 @@ interface IB20Security is IB20 {
/// @param amounts Per-recipient amounts, parallel to `recipients`.
function batchMint(address[] calldata recipients, uint256[] calldata amounts) external;

/// @notice Burns `amounts[i]` from `accounts[i]` in one call, unconditionally on the supplied
/// accounts (no policy gate). All-or-nothing: any element revert unwinds the whole
/// transaction. Emits `Transfer(accounts[i], address(0), amounts[i])` per element;
/// does NOT emit `BurnedBlocked`.
///
/// @dev Reverts with `ContractPaused(BURN)` when `BURN` is paused.
/// @dev Reverts with `AccessControlUnauthorizedAccount` when the caller does not hold `BURN_FROM_ROLE`. Strict — no factory-bootstrap bypass.
/// @dev Reverts with `LengthMismatch` when `accounts.length != amounts.length`.
/// @dev Reverts with `EmptyBatch` when either array is empty.
/// @dev Reverts with `InsufficientBalance` when any `accounts[i]`'s balance is below `amounts[i]`.
///
/// @param accounts Accounts whose balances will be debited.
/// @param amounts Per-account amounts, parallel to `accounts`.
function batchBurn(address[] calldata accounts, uint256[] calldata amounts) external;

/*//////////////////////////////////////////////////////////////
REDEMPTION
//////////////////////////////////////////////////////////////*/
Expand Down
1 change: 0 additions & 1 deletion src/lib/B20Constants.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ library B20Constants {
bytes32 internal constant MINT_ROLE = keccak256("MINT_ROLE");
bytes32 internal constant BURN_ROLE = keccak256("BURN_ROLE");
bytes32 internal constant BURN_BLOCKED_ROLE = keccak256("BURN_BLOCKED_ROLE");
bytes32 internal constant BURN_FROM_ROLE = keccak256("BURN_FROM_ROLE");
bytes32 internal constant PAUSE_ROLE = keccak256("PAUSE_ROLE");
bytes32 internal constant UNPAUSE_ROLE = keccak256("UNPAUSE_ROLE");
bytes32 internal constant METADATA_ROLE = keccak256("METADATA_ROLE");
Expand Down
26 changes: 11 additions & 15 deletions src/lib/B20FactoryLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ library B20FactoryLib {
}

/// @notice Bootstrap role-grant bundle for `B20Variant.SECURITY`. Superset of `B20RoleHolders`
/// with `BURN_FROM_ROLE` and `SECURITY_OPERATOR_ROLE` slots.
/// with a `SECURITY_OPERATOR_ROLE` slot.
///
/// @dev `DEFAULT_ADMIN_ROLE` is assigned via `B20SecurityCreateParams.initialAdmin`, not this struct.
struct B20SecurityRoleHolders {
Expand All @@ -60,8 +60,6 @@ library B20FactoryLib {
address burner;
/// @dev Account granted `BURN_BLOCKED_ROLE`.
address burnBlocker;
/// @dev Account granted `BURN_FROM_ROLE`.
address burnFromOperator;
/// @dev Account granted `PAUSE_ROLE`.
address pauser;
/// @dev Account granted `UNPAUSE_ROLE`.
Expand Down Expand Up @@ -253,25 +251,23 @@ library B20FactoryLib {
/// @param holders Security role-holder bundle.
/// @return initCalls ABI-encoded `grantRole` initCalls.
function buildRoleGrants(B20SecurityRoleHolders memory holders) internal pure returns (bytes[] memory initCalls) {
bytes32[] memory roles = new bytes32[](8);
bytes32[] memory roles = new bytes32[](7);
roles[0] = B20Constants.MINT_ROLE;
roles[1] = B20Constants.BURN_ROLE;
roles[2] = B20Constants.BURN_BLOCKED_ROLE;
roles[3] = B20Constants.BURN_FROM_ROLE;
roles[4] = B20Constants.PAUSE_ROLE;
roles[5] = B20Constants.UNPAUSE_ROLE;
roles[6] = B20Constants.METADATA_ROLE;
roles[7] = B20Constants.SECURITY_OPERATOR_ROLE;
roles[3] = B20Constants.PAUSE_ROLE;
roles[4] = B20Constants.UNPAUSE_ROLE;
roles[5] = B20Constants.METADATA_ROLE;
roles[6] = B20Constants.SECURITY_OPERATOR_ROLE;

address[] memory accounts = new address[](8);
address[] memory accounts = new address[](7);
accounts[0] = holders.minter;
accounts[1] = holders.burner;
accounts[2] = holders.burnBlocker;
accounts[3] = holders.burnFromOperator;
accounts[4] = holders.pauser;
accounts[5] = holders.unpauser;
accounts[6] = holders.metadataAdmin;
accounts[7] = holders.securityOperator;
accounts[3] = holders.pauser;
accounts[4] = holders.unpauser;
accounts[5] = holders.metadataAdmin;
accounts[6] = holders.securityOperator;

return buildRoleGrants(roles, accounts);
}
Expand Down
16 changes: 3 additions & 13 deletions test/lib/B20SecurityTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,16 @@ import {IB20Security} from "src/interfaces/IB20Security.sol";
/// Extends `B20Test` for the inherited test surface (actors, labels,
/// setUp wiring, the `_singleFeature` helper, the `_grantRole` /
/// `_mint` / `_pause` action wrappers, and the security-variant token
/// deployed by `_deployToken`). Adds the variant-specific role holders
/// (`operator`, `burnFromActor`) plus helpers for the announcement,
/// share-ratio, redemption, and identifier surfaces.
/// deployed by `_deployToken`). Adds the variant-specific role holder
/// (`operator`) plus helpers for the announcement, share-ratio,
/// redemption, and identifier surfaces.
///
/// The inherited `token` member is typed `IB20`. Tests that need the
/// variant-only surface (`announce`, `redeem`, etc.) cast inline via
/// the `security` view-helper.
contract B20SecurityTest is B20Test {
// -- Security-variant role-holder actors --
address internal operator = makeAddr("operator");
address internal burnFromActor = makeAddr("burnFromActor");

// ============================================================
// SECURITY-VARIANT IDENTIFIER FIXTURES
Expand All @@ -43,7 +42,6 @@ contract B20SecurityTest is B20Test {
function setUp() public virtual override {
super.setUp();
vm.label(operator, "operator");
vm.label(burnFromActor, "burnFromActor");
}

// ============================================================
Expand All @@ -67,13 +65,6 @@ contract B20SecurityTest is B20Test {
if (!token.hasRole(role, operator)) _grantRole(role, operator);
}

/// @notice Grants `BURN_FROM_ROLE` to the `burnFromActor` actor as
/// the admin, idempotent.
function _grantBurnFrom() internal {
bytes32 role = security().BURN_FROM_ROLE();
if (!token.hasRole(role, burnFromActor)) _grantRole(role, burnFromActor);
}

// ============================================================
// SHARE-RATIO HELPERS
// ============================================================
Expand Down Expand Up @@ -188,6 +179,5 @@ contract B20SecurityTest is B20Test {
// `test/unit/B20Security/constants/` pins that down.

bytes32 internal constant SECURITY_OPERATOR_ROLE = keccak256("SECURITY_OPERATOR_ROLE");
bytes32 internal constant BURN_FROM_ROLE = keccak256("BURN_FROM_ROLE");
bytes32 internal constant REDEEM_SENDER_POLICY = keccak256("REDEEM_SENDER_POLICY");
}
16 changes: 11 additions & 5 deletions test/lib/B20Test.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,19 @@ contract B20Test is B20FactoryTest {
return B20Constants.MINT_RECEIVER_POLICY;
}

/// @notice True iff `policyType` is one of the four base-token
/// supported policy types. Used by tests that fuzz arbitrary
/// `bytes32` and need to filter to the supported / unsupported
/// partition.
/// @notice True iff `policyType` is supported by the deployed token.
/// Used by tests that fuzz arbitrary `bytes32` and need to filter
/// to the supported / unsupported partition. Includes the four
/// base-token policy types AND the security variant's own
/// `REDEEM_SENDER_POLICY`, because `_deployToken()` returns a
/// security-variant token (see contract-level natspec) — fuzzing
/// a `bytes32` that happens to equal `keccak256("REDEEM_SENDER_POLICY")`
/// would otherwise fall through to the variant's `_readPolicyId`
/// override and not revert as `UnsupportedPolicyType`.
function _isKnownPolicyType(bytes32 policyType) internal pure returns (bool) {
return policyType == B20Constants.TRANSFER_SENDER_POLICY || policyType == B20Constants.TRANSFER_RECEIVER_POLICY
|| policyType == B20Constants.TRANSFER_EXECUTOR_POLICY || policyType == B20Constants.MINT_RECEIVER_POLICY;
|| policyType == B20Constants.TRANSFER_EXECUTOR_POLICY || policyType == B20Constants.MINT_RECEIVER_POLICY
|| policyType == keccak256("REDEEM_SENDER_POLICY");
Copy link
Copy Markdown
Collaborator Author

@ilikesymmetry ilikesymmetry Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we adding this? Can we keep out as we are removing REDEEM_SENDER_POLICY in #119

Copy link
Copy Markdown
Collaborator Author

@ilikesymmetry ilikesymmetry Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping it in: without this line, this PR's CI breaks. The fuzz tests in question (test_policyId_revert_unsupportedPolicyType / test_updatePolicy_revert_unsupportedPolicyType) call vm.assume(!_isKnownPolicyType(x)) and then expect UnsupportedPolicyType. Because B20Test._deployToken() returns a security-variant token, REDEEM_SENDER_POLICY is actually a supported slot at runtime (via MockB20Security._readPolicyId override at test/lib/mocks/MockB20Security.sol:222) — when the fuzz seed hits keccak256("REDEEM_SENDER_POLICY") the call doesn't revert. This was a latent bug that the seed shift from the batchBurn removal happened to surface (see commit 91f1342).

You're right that it becomes dead the moment #119 lands — both the override and the constant go away — so removing this line is a trivial part of that PR's cleanup. Happy to leave a TODO here pointing at #119 if you'd like.

}

/// @notice Pauses a single `PausableFeature`, lazily granting `PAUSE_ROLE`
Expand Down
8 changes: 4 additions & 4 deletions test/lib/mocks/MockB20.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,10 +755,10 @@ abstract contract MockB20 is IB20 {
/// @dev Pure mechanics: balance + effects. Pause and role gates are
/// enforced by entrypoint modifiers (`whenNotPaused`,
/// `onlyRole`) on every caller (`burn`, `burnWithMemo`,
/// `burnBlocked`, and `MockB20Security`'s `batchBurn` /
/// `_redeemBurn`); see those functions for the per-caller
/// authorization surface. This helper never authorizes the
/// destruction on its own.
/// `burnBlocked`, and `MockB20Security`'s `_redeemBurn`);
/// see those functions for the per-caller authorization
/// surface. This helper never authorizes the destruction on
/// its own.
function _burnRaw(address from, uint256 amount) internal {
MockB20Storage.Layout storage $ = MockB20Storage.layout();
uint256 fromBalance = $.balances[from];
Expand Down
57 changes: 7 additions & 50 deletions test/lib/mocks/MockB20Security.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,12 @@ import {MockB20RedeemStorage} from "test/lib/mocks/MockB20Storage.sol";
/// `_isPrivileged()` so the factory can stage initial
/// announcements, batched issuance, ratios, identifiers, and
/// minimum-redeemable values during the bootstrap window
/// without first granting itself roles. The two paths that
/// the factory will never legitimately call during bootstrap
/// (`redeem` / `redeemWithMemo`, which are holder-initiated,
/// and `batchBurn`, which is a corporate-actions clawback
/// against existing balances) deliberately do NOT bypass
/// their authorization checks: there is no init-time use case
/// for them, so the bypass would be dead code that widens the
/// without first granting itself roles. `redeem` /
/// `redeemWithMemo` are the only paths the factory will never
/// legitimately call during bootstrap (they are
/// holder-initiated) and deliberately do NOT bypass their
/// authorization checks: there is no init-time use case for
/// them, so the bypass would be dead code that widens the
/// attack surface without buying anything. Token invariants
/// (supply-cap math, balance accounting, share-amount floor
/// on `redeem`) are NOT bypassed anywhere.
Expand All @@ -75,7 +74,6 @@ contract MockB20Security is MockB20, IB20Security {
// ============================================================

bytes32 public constant SECURITY_OPERATOR_ROLE = keccak256("SECURITY_OPERATOR_ROLE");
bytes32 public constant BURN_FROM_ROLE = keccak256("BURN_FROM_ROLE");

bytes32 public constant REDEEM_SENDER_POLICY = keccak256("REDEEM_SENDER_POLICY");

Expand All @@ -96,25 +94,6 @@ contract MockB20Security is MockB20, IB20Security {
return 6;
}

// ============================================================
// MODIFIERS
// ============================================================

/// @dev Like the base `onlyRole` but without the factory bootstrap
/// bypass: the role check is unconditional, even for the
/// factory in the init window. Reserved for variant paths
/// that deliberately reject the bypass — currently just
/// `batchBurn`, the corporate-actions clawback that has no
/// init-time use case (see this contract's natspec). Lives
/// here, not on the base, because no base function needs the
/// stricter gate.
modifier onlyRoleStrict(bytes32 role) {
Copy link
Copy Markdown
Collaborator Author

@ilikesymmetry ilikesymmetry Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we deleting this?

Copy link
Copy Markdown
Collaborator Author

@ilikesymmetry ilikesymmetry Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code after batchBurn removal: onlyRoleStrict had exactly one caller (batchBurn in MockB20Security), and its natspec called that out ("Reserved for variant paths that deliberately reject the bypass — currently just batchBurn"). With batchBurn gone there's no remaining caller and nothing in the security variant needs the strict gate, so I dropped the modifier rather than leaving it as an unused primitive. Easy to restore later if a new function actually needs the bypass-rejecting variant.

if (!hasRole(role, msg.sender)) {
revert AccessControlUnauthorizedAccount(msg.sender, role);
}
_;
}

// ============================================================
// ANNOUNCEMENTS
// ============================================================
Expand Down Expand Up @@ -170,7 +149,7 @@ contract MockB20Security is MockB20, IB20Security {
}

// ============================================================
// BATCHED ISSUANCE / CLAWBACK
// BATCHED ISSUANCE
// ============================================================

/// @dev Pause + role enforced ONCE for the entire batch via the
Expand All @@ -190,28 +169,6 @@ contract MockB20Security is MockB20, IB20Security {
}
}

/// @dev `onlyRoleStrict` (not `onlyRole`): the factory bootstrap
/// bypass is deliberately NOT honored here, per the contract
/// natspec — clawback against existing balances has no init-time
/// use case, so granting the factory a bypass would only widen
/// the attack surface.
function batchBurn(address[] calldata accounts, uint256[] calldata amounts)
external
whenNotPaused(PausableFeature.BURN)
onlyRoleStrict(BURN_FROM_ROLE)
{
if (accounts.length != amounts.length) {
revert LengthMismatch(accounts.length, amounts.length);
}
if (accounts.length == 0) revert EmptyBatch();
for (uint256 i = 0; i < accounts.length; i++) {
// Zero amounts are allowed per ERC-20 conventions (`transfer(0)` is valid);
// `_burnRaw` is a no-op for amount == 0 and emits `Transfer(account, 0, 0)`.
// Callers that want all-non-zero semantics can validate upstream.
_burnRaw(accounts[i], amounts[i]);
}
}

// ============================================================
// REDEMPTION
// ============================================================
Expand Down
Loading
Loading