coinbase / coinbase/commerce-onchain-payment-protocol

`Sweepable.setSweeper` NatSpec contradicts the `notZero` modifier

Open Beginner friendly
#72 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Solidity
Stars
205
Forks
92
PR merge metrics
No merged PRs in 30d

Description

At `contracts/utils/Sweepable.sol` L34-38:

```solidity
// @dev Sets the sweeper
// @notice To remove the sweeper role entirely, set this to the zero address.
function setSweeper(address newSweeper) public virtual onlyOwner notZero(newSweeper) {
_sweeper = newSweeper;
}
```

The NatSpec says passing `address(0)` removes the sweeper role. The `notZero` modifier on L24-27 reverts on `address(0)`. One has to change.

Two options:

1. Drop `notZero` if the doc is right:

```diff
-function setSweeper(address newSweeper) public virtual onlyOwner notZero(newSweeper) {
+function setSweeper(address newSweeper) public virtual onlyOwner {
_sweeper = newSweeper;
}
```

`onlySweeper` on L19-22 still blocks everyone once `_sweeper == address(0)`, since `_msgSender()` is never the zero address.

2. Delete the second comment line if the zero-address ban is intended.

No funds at risk; just a doc/code mismatch.

Contributor guide

No contributing guide indexed for this repository

Research direction

Open contracts/utils/Sweepable.sol and inspect setSweeper at L34-38 alongside the notZero modifier at L24-27 and onlySweeper at L19-22. Decide whether address(0) should remove the sweeper role or remain forbidden. Done means the NatSpec and contract behavior consistently describe the same rule.

Written by the indexing model from the issue text.

Assessment

Tech stack
solidity
Domain
blockchain
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.