Skip to content

fix(S1): use dedicated blocklist zero-address error#59

Open
liobrasil wants to merge 1 commit intomainfrom
fix/S1-blocklist-batch-zero-address-error
Open

fix(S1): use dedicated blocklist zero-address error#59
liobrasil wants to merge 1 commit intomainfrom
fix/S1-blocklist-batch-zero-address-error

Conversation

@liobrasil
Copy link
Collaborator

Summary

Fixes #32

Aligns blocklist batch-add validation with operation intent by using a dedicated blocklist zero-address error instead of the allowlist-specific error.

Key points:

  • batchAddToBlocklist() no longer reverts with allowlist-specific error naming
  • Added dedicated CannotBlocklistZeroAddress error for clearer admin/operator diagnostics
  • Added regression test to lock expected revert selector for zero-address blocklist inputs

Changes:

  • Added CannotBlocklistZeroAddress custom error
  • Updated batchAddToBlocklist() to revert with CannotBlocklistZeroAddress() on address(0)
  • Added test_BatchAddToBlocklist_RevertZeroAddress()

Test plan

  • env FOUNDRY_OFFLINE=true forge test --match-contract FlowYieldVaultsRequestsTest --threads 1

@claude
Copy link

claude bot commented Feb 20, 2026

PR Review: fix(S1): use dedicated blocklist zero-address error

Summary: This is a correct, minimal fix for a copy-paste bug where batchAddToBlocklist() was reverting with the allowlist-specific CannotAllowlistZeroAddress error instead of a blocklist-appropriate one.


Correctness

The bug is real and the fix is right. batchAddToBlocklist() was clearly scaffolded from batchAddToAllowlist() and the error wasn't updated, causing misleading revert messages for operators/admins. The one-line change at solidity/src/FlowYieldVaultsRequests.sol:595 is exactly the right fix.

Code Quality

  • The new CannotBlocklistZeroAddress error follows the same NatSpec and naming conventions as the existing CannotAllowlistZeroAddress error.
  • Placement in the error block (immediately after its allowlist counterpart) is logical.
  • No unrelated changes — the diff is appropriately scoped.

Test Coverage

The new test test_BatchAddToBlocklist_RevertZeroAddress is correct:

  • Uses vm.prank(c.owner()) appropriately, matching the onlyOwner modifier on batchAddToBlocklist.
  • Verifies the exact error selector (.selector), which locks the ABI-level interface against future regressions — consistent with how similar tests in this file are written (e.g., test_CreateYieldVault_RevertZeroAmount).

One minor observation: there is no equivalent test_BatchAddToAllowlist_RevertZeroAddress test in the suite — the pre-existing CannotAllowlistZeroAddress path is untested at the unit level. That said, this is out of scope for this PR and should be tracked separately.

Security / Performance

No security or performance impact. This is purely a diagnostic fix — error naming only, no behavior change.


Verdict: Looks good to merge. Clean fix, appropriate test, no concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

S1: Blocklist Batch Add Uses Allowlist-Zero Error

1 participant