Any whitelisted operator can take any holder's badges, and the holder's revocation is unreadable
The badge reported every whitelisted operator as approved for every account, and nothing required that operator to be a contract, so a plain whitelisted address could move any holder's badges without a signature or payment. A holder's explicit refusal was written to storage but never read.
Description
isApprovedForAll returns true for any whitelisted operator against every
account, unconditionally:
function isApprovedForAll(address account, address operator) public view override returns (bool) {return isSettlement[operator] || super.isApprovedForAll(account, operator);}
OpenZeppelin's ERC1155.safeTransferFrom gates on _checkAuthorized(_msgSender(), from),
which consults exactly this override. The walled-garden gate that runs first checks only
the operator's whitelist membership and the token's class bit:
function _checkOperatorMayMove(address operator, uint256 id) internal view {if (!isSettlement[operator]) revert NotSettlementContract(operator);_checkClassPermitted(operator, settlementClassMask[operator], id);}
Neither path checks that from consented. settlementClassMask restricts which class
an operator may move; nothing restricts whose badge. The security model therefore rests
on an unstated property — that the whitelist only ever contains contracts whose own code
enforces consent. PlakxioSettlement does enforce it, through EIP-712 order signatures.
Nothing in PlakxioBadge requires a whitelisted address to.
setSettlementContract accepts a bare address with no code.length check and takes
effect in the same transaction, so the whitelisted address need not be a contract at all.
Because the left operand of the || is already true, a holder calling
setApprovalForAll(operator, false) gets a successful transaction and an ApprovalForAll
event while the contract's answer stays true. The revocation is written to storage and
never read.
Vulnerable Scenario:
- A holder owns badges across classes and pre-emptively calls
setApprovalForAll(grantee, false). - The admin calls
setSettlementContract(grantee, true, CLASS_MASK_ALL)wheregranteeis a plain EOA. isApprovedForAll(holder, grantee)now returnstrue, overriding step 1.granteecallssafeBatchTransferFrom(holder, grantee, [normalId, premiumId, specialId], [1,1,1], ""). Whitelist membership passes, every class bit is set, and_checkAuthorizedpasses on the override.- All three badges move. No order signature is presented and no payment is made.
- The holder cannot move a badge to safety:
safeTransferFromfrom their own address revertsNotSettlementContract, and no burn function exists.
Impact
Every badge holder loses every badge of every class named in the mask, without payment and without any signature of theirs. Special one-of-ones are included, so the ascending-floor mechanic is bypassed entirely for those tokens rather than ratcheted. Holders have no exit and no revocation the contract will honour.
Severity is adjusted one tier down from the matrix lookup because the path requires
DEFAULT_ADMIN_ROLE — a fully-trusted role — to act outside its stated purpose. The trust
assumption being violated is that the whitelist admits only consent-enforcing settlement
contracts, which setSettlementContract does not check and cannot express.
Recommendation
Two options; they are independent and Option B preserves the no-approval seller UX.
Option A — require holder approval as well as the whitelist. The gate becomes a restriction rather than a grant, and a holder can revoke:
function isApprovedForAll(address account, address operator) public view override returns (bool) {- return isSettlement[operator] || super.isApprovedForAll(account, operator);+ return isSettlement[operator] && super.isApprovedForAll(account, operator);}
This is a design change rather than a patch: the current flow relies on sellers never needing an approval, so the change fails 82 tests in the existing suite until the seller-side approval is introduced end to end. Weigh it against Option B accordingly.
Option B — delay the grant and require code. Keeps the UX and turns an unobservable one-transaction confiscation into an announced, cancellable one:
+uint256 public constant ACTIVATION_DELAY = 2 days;+mapping(address account => uint256) public settlementActiveFrom;if (allowed && classMask == 0) revert EmptyClassMask(settlement);+if (allowed && settlement.code.length == 0) revert SettlementNotAContract(settlement);+if (allowed) settlementActiveFrom[settlement] = block.timestamp + ACTIVATION_DELAY;
function _checkOperatorMayMove(address operator, uint256 id) internal view {if (!isSettlement[operator]) revert NotSettlementContract(operator);+ if (block.timestamp < settlementActiveFrom[operator]) revert SettlementNotActiveYet(operator);_checkClassPermitted(operator, settlementClassMask[operator], id);}
Revocation must stay instant — it writes isSettlement = false and zeroes the mask, and no
delay should apply to it.
Independently of either option, isApprovedForAll returning true for an operator whose
class mask excludes the token is misleading to any integrator reading it as the ERC-1155
authority signal. Returning false when settlementClassMask[operator] == 0 costs nothing
and removes the worst of that.
Resolution
Three controls landed: setSettlementContract refuses an address with no code, a settlement
grant waits an activation delay before it may move badges, and a holder's opt-out is now honoured
on the view and on both the single and batch transfer paths. A zero class mask no longer reads
as approved. Each step of the proof-of-concept was re-run against the fix and is closed.
One note for deployment: the activation delay is an immutable constructor argument with no lower bound, and a value of zero reinstates immediate activation on a contract that cannot be replaced without stranding every badge. A floor in the constructor would make the control code-enforced rather than deployment-dependent. Consent also remains opt-out rather than opt-in, so a holder who never refuses is still exposed to a bad whitelist entry — the residual is admin-gated, and the default admin role is itself now two-step and delayed.
Affected files
src/PlakxioBadge.sol#L258-L259andsrc/PlakxioBadge.sol#L300-L303at commit5c38893