coinbase / coinbase/commerce-onchain-payment-protocol
`Sweepable.setSweeper` NatSpec contradicts the `notZero` modifier
- 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