Fix owneronly for settings#453
Merged
Merged
Conversation
Add a centralized wallet blacklist in Settings and gate the core reward, staking, booster, and channel paths through the shared Settings lookup. RewardsBooster also gains an owner-only accounting removal path so polluted booster balances can be corrected without moving tokens. Constraint: RewardsBooster is close to the EIP-170 size limit, so blacklist reverts are intentionally reasonless and runner checks are not duplicated in spendQueryRewards. Rejected: Per-contract blacklist state | would fragment emergency operations and make removals easy to miss. Rejected: Token-transfer based booster repair | cleanup must correct polluted accounting without touching custody. Confidence: high Scope-risk: moderate Directive: Keep blacklist authority centralized in Settings unless deployment governance changes. Tested: yarn hardhat compile Tested: yarn build:abi Tested: yarn test test/RewardsBooster.test.ts test/Staking.test.ts test/RewardsDistributer.test.ts Not-tested: full yarn test suite
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds owner-managed wallet blacklist in Settings and a reusable SQParameter guard; enforces blacklist checks across channel, rewards, and staking contracts. Updates IRewardsBooster ABI/signature (adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Contract as StateChannel / Staking / Rewards*
participant Settings as Settings
participant Booster as RewardsBooster
participant Distributor as RewardsDistributor
Client->>Contract: call external action (e.g., claim / fund / stake)
Contract->>Settings: isWalletBlacklisted(msg.sender or target)?
Settings-->>Contract: bool (not blacklisted)
alt allowed
Contract->>Distributor: perform reward/claim flows (may call)
Distributor->>Settings: isWalletBlacklisted(runner / sender)?
Distributor-->>Contract: proceed
Contract->>Booster: spendQueryRewards(..., _runner)
Booster->>Settings: isWalletBlacklisted(_runner)?
Booster-->>Contract: reward spend result
Contract-->>Client: success / token transfers
else blocked
Settings-->>Contract: revert (blocked)
Contract-->>Client: revert
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/ConsumerHost.sol (1)
382-387:⚠️ Potential issue | 🔴 CriticalDon't blacklist-gate the
claimed()settlement callback.
claimed()is invoked fromStateChannel._finalize(), so this turns a later blacklist on the consumer into a permanent finalize failure for any already-open channel. That strands locked channel funds instead of just blocking new interactions.Suggested fix
function claimed(uint256 channelId, uint256 amount) external { require(msg.sender == settings.getContractAddress(SQContracts.StateChannel), 'G011'); address consumer = channels[channelId]; - _requireNotBlacklisted(settings, consumer); Consumer storage info = consumers[consumer]; info.balance += amount;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/ConsumerHost.sol` around lines 382 - 387, The claimed() function currently calls _requireNotBlacklisted(settings, consumer), which prevents StateChannel._finalize() from completing for already-open channels; remove that blacklist check so claimed() can always be executed by the StateChannel caller. Update the claimed(uint256 channelId, uint256 amount) implementation to keep the require(msg.sender == settings.getContractAddress(SQContracts.StateChannel), 'G011') guard but delete the _requireNotBlacklisted(settings, consumer) line (or wrap blacklist logic only for new-interaction entrypoints), ensuring channel finalization and Consumer storage updates still occur as before.contracts/RewardsBooster.sol (2)
996-1007:⚠️ Potential issue | 🟠 MajorController callers are still missing a blacklist check here.
collectAllocationReward()blocks a blacklisted_runner, but not a blacklisted controller. If the runner is clean and its controller is blacklisted, the controller can still collect rewards because only the subject address is checked before the RB005 authorization branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/RewardsBooster.sol` around lines 996 - 1007, In collectAllocationReward add a blacklist check for the controller before the authorization require: call _requireNotBlacklisted(settings, controller) (similar to the existing _requireNotBlacklisted(settings, _runner)) prior to the require that compares msg.sender to _runner/controller/stakingAllocation so a blacklisted controller cannot collect rewards; locate this in the collectAllocationReward function where controller is assigned from indexerRegistry.getController(_runner).
124-131:⚠️ Potential issue | 🟠 MajorBlacklisted controllers can still act through
consumerAuthorised.This modifier only checks
consumer. Whenmsg.senderis a controller, a blacklisted controller can still callboostDeploymentFor()andswapBoosterDeployment()on behalf of an unblacklisted consumer.Suggested fix
modifier consumerAuthorised(address consumer) { _requireNotBlacklisted(settings, consumer); + _requireNotBlacklisted(settings, msg.sender); if (msg.sender != consumer) { bool isController = IConsumerRegistry(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/RewardsBooster.sol` around lines 124 - 131, The consumerAuthorised modifier currently only checks that the consumer is not blacklisted via _requireNotBlacklisted(settings, consumer) but allows a blacklisted controller (msg.sender) to act for the consumer; update the modifier so that when msg.sender != consumer and isController(...) returns true you also call _requireNotBlacklisted(settings, msg.sender) (or otherwise reject blacklisted controllers) before allowing the call—this will protect functions like boostDeploymentFor and swapBoosterDeployment from being invoked by blacklisted controllers while preserving valid controller behavior via settings.getContractAddress(...) and IConsumerRegistry.isController.
🧹 Nitpick comments (2)
test/RewardsDistributer.test.ts (1)
642-646: Good regression check; consider adding unblock recovery assertion.Optional: after blacklisting revert, set blacklist back to false and assert claim path succeeds when rewards exist. That guards against sticky-state regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/RewardsDistributer.test.ts` around lines 642 - 646, After asserting that a blacklisted wallet cannot claim, un-blacklist the wallet by calling settings.setWalletBlacklisted(delegator.address, false) and then perform the positive claim path with rewardsDistributor.connect(delegator).claim(runner.address), asserting it succeeds (e.g., does not revert and optionally emits the expected event or updates token/balance state); if the test requires rewards to exist first, ensure you create/distribute rewards (via the existing distribution helper or rewardsDistributor methods) before asserting the successful claim to guard against sticky blacklist state regressions.test/Staking.test.ts (1)
150-154: Test name overstates current coverage.The case currently verifies only
delegate. Either rename it to match scope or add at least one more staking action assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Staking.test.ts` around lines 150 - 154, Test title is too broad for what it asserts; either narrow the name or add another assertion for a different staking action. Option A: rename the it(...) description from 'blacklisted wallet can not use staking actions' to 'blacklisted wallet cannot delegate' to match the single assertion that calls settings.setWalletBlacklisted(...) and stakingManager.connect(delegator).delegate(...). Option B: keep the broader name and add at least one more expect that a blacklisted account is reverted when calling a different staking method (for example stakingManager.connect(delegator).undelegate(...) or stakingManager.connect(delegator).withdraw(...) depending on available API), using the same pattern as the delegate assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/RewardsPool.sol`:
- Line 153: The blacklist checks currently call _requireNotBlacklisted(settings,
runner) but do not block a blacklisted msg.sender from delegating through a
non-blacklisted runner; update each occurrence (including the instances at the
same pattern around lines ~153, 215, 227, 241, 255) to enforce the blacklist for
both parties by calling _requireNotBlacklisted(settings, msg.sender) in addition
to _requireNotBlacklisted(settings, runner) (or replace with a new helper that
validates both addresses) so that both the caller (msg.sender) and the runner
are checked before proceeding.
In `@contracts/RewardsStaking.sol`:
- Around line 254-256: The applyStakeChange function currently only calls
_requireNotBlacklisted for staker but proceeds to modify runner state
(getRewardInfo, claimFrom, _updateTotalStakingAmount, onStakeUpdate); add a
blacklist gate for runner by calling _requireNotBlacklisted(settings, runner) at
the start of applyStakeChange (before any calls to getRewardInfo, claimFrom,
_updateTotalStakingAmount, or onStakeUpdate) so blacklisted runners cannot have
their reward/stake state progressed by this path.
In `@contracts/Staking.sol`:
- Around line 122-123: The expectedBalance invariant currently starts at 0 and
is only updated in transferDelegationTokens(), causing drift and possible
underflow in withdraw/slash; seed expectedBalance to the actual token balance
during the upgrade/migration and update it wherever tokens enter the contract
(not just transferDelegationTokens()), specifically add updates in the reward
restake path (handle inflows from StakingManager.claimForDelegate() and
subsequent addDelegation(...) calls) so every inflow increments expectedBalance
and every outflow (withdraw/slash) decrements it to keep the invariant
consistent.
In `@contracts/StakingManager.sol`:
- Around line 122-124: The added _requireNotBlacklisted(settings, _runner) at
the top of unstake() prevents IndexerRegistry from force-unregistering
blacklisted runners; change the guard so blacklisted checks only apply to the
self-service path: remove the top-level _requireNotBlacklisted(settings,
_runner) and instead call _requireNotBlacklisted(settings, _runner) only inside
the branch where msg.sender == _runner (the self-initiated unstake path),
leaving the msg.sender == IndexerRegistry branch untouched so protocol-driven
unregisters succeed.
---
Outside diff comments:
In `@contracts/ConsumerHost.sol`:
- Around line 382-387: The claimed() function currently calls
_requireNotBlacklisted(settings, consumer), which prevents
StateChannel._finalize() from completing for already-open channels; remove that
blacklist check so claimed() can always be executed by the StateChannel caller.
Update the claimed(uint256 channelId, uint256 amount) implementation to keep the
require(msg.sender == settings.getContractAddress(SQContracts.StateChannel),
'G011') guard but delete the _requireNotBlacklisted(settings, consumer) line (or
wrap blacklist logic only for new-interaction entrypoints), ensuring channel
finalization and Consumer storage updates still occur as before.
In `@contracts/RewardsBooster.sol`:
- Around line 996-1007: In collectAllocationReward add a blacklist check for the
controller before the authorization require: call
_requireNotBlacklisted(settings, controller) (similar to the existing
_requireNotBlacklisted(settings, _runner)) prior to the require that compares
msg.sender to _runner/controller/stakingAllocation so a blacklisted controller
cannot collect rewards; locate this in the collectAllocationReward function
where controller is assigned from indexerRegistry.getController(_runner).
- Around line 124-131: The consumerAuthorised modifier currently only checks
that the consumer is not blacklisted via _requireNotBlacklisted(settings,
consumer) but allows a blacklisted controller (msg.sender) to act for the
consumer; update the modifier so that when msg.sender != consumer and
isController(...) returns true you also call _requireNotBlacklisted(settings,
msg.sender) (or otherwise reject blacklisted controllers) before allowing the
call—this will protect functions like boostDeploymentFor and
swapBoosterDeployment from being invoked by blacklisted controllers while
preserving valid controller behavior via settings.getContractAddress(...) and
IConsumerRegistry.isController.
---
Nitpick comments:
In `@test/RewardsDistributer.test.ts`:
- Around line 642-646: After asserting that a blacklisted wallet cannot claim,
un-blacklist the wallet by calling
settings.setWalletBlacklisted(delegator.address, false) and then perform the
positive claim path with
rewardsDistributor.connect(delegator).claim(runner.address), asserting it
succeeds (e.g., does not revert and optionally emits the expected event or
updates token/balance state); if the test requires rewards to exist first,
ensure you create/distribute rewards (via the existing distribution helper or
rewardsDistributor methods) before asserting the successful claim to guard
against sticky blacklist state regressions.
In `@test/Staking.test.ts`:
- Around line 150-154: Test title is too broad for what it asserts; either
narrow the name or add another assertion for a different staking action. Option
A: rename the it(...) description from 'blacklisted wallet can not use staking
actions' to 'blacklisted wallet cannot delegate' to match the single assertion
that calls settings.setWalletBlacklisted(...) and
stakingManager.connect(delegator).delegate(...). Option B: keep the broader name
and add at least one more expect that a blacklisted account is reverted when
calling a different staking method (for example
stakingManager.connect(delegator).undelegate(...) or
stakingManager.connect(delegator).withdraw(...) depending on available API),
using the same pattern as the delegate assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b43976e-aafe-4cd1-afa6-9d58ef025762
📒 Files selected for processing (24)
contracts/ConsumerHost.solcontracts/RewardsBooster.solcontracts/RewardsDistributor.solcontracts/RewardsPool.solcontracts/RewardsStaking.solcontracts/Settings.solcontracts/Staking.solcontracts/StakingAllocation.solcontracts/StakingManager.solcontracts/StateChannel.solcontracts/interfaces/IRewardsBooster.solcontracts/interfaces/ISettings.solcontracts/utils/SQParameter.solpublish/ABI/RewardsBooster.jsonpublish/ABI/RewardsStaking.jsonpublish/ABI/Settings.jsonpublish/ABI/Staking.jsonpublish/ABI/StakingAllocation.jsonpublish/ABI/StakingManager.jsonpublish/mainnet.jsontest/RewardsBooster.test.tstest/RewardsBooster_migration.test.tstest/RewardsDistributer.test.tstest/Staking.test.ts
Comment on lines
337
to
+339
| function collectAndDistributeRewards(address runner) public { | ||
| _requireNotBlacklisted(settings, runner); | ||
|
|
There was a problem hiding this comment.
Avoid freezing distributor accounting for blacklisted runners.
These two functions are bookkeeping entrypoints, not runner-authenticated actions. Rejecting them on runner means accSQTPerStake never advances for that runner again, so non-blacklisted delegators can get stuck with already-earned rewards and unsettled stake changes.
Suggested fix
function collectAndDistributeRewards(address runner) public {
- _requireNotBlacklisted(settings, runner);
-
// check current era is after lastClaimEra
uint256 currentEra = _getCurrentEra();
require(info[runner].lastClaimEra < currentEra - 1, 'RD003');
collectAndDistributeEraRewards(currentEra, runner);
}
@@
function collectAndDistributeEraRewards(
uint256 currentEra,
address runner
) public returns (uint256) {
- _requireNotBlacklisted(settings, runner);
-
RewardInfo storage rewardInfo = info[runner];
require(rewardInfo.lastClaimEra > 0, 'RD004');Also applies to: 351-356
| * @param amount the labor of services | ||
| */ | ||
| function labor(bytes32 deploymentId, address runner, uint256 amount) external { | ||
| _requireNotBlacklisted(settings, runner); |
There was a problem hiding this comment.
Blacklist enforcement is incomplete: caller bypass remains possible.
These paths only gate runner; they do not gate msg.sender. A blacklisted account can still interact by calling with a non-blacklisted runner.
Suggested patch
function labor(bytes32 deploymentId, address runner, uint256 amount) external {
+ _requireNotBlacklisted(settings, msg.sender);
_requireNotBlacklisted(settings, runner);
...
}
function collect(bytes32 deploymentId, address runner) external {
+ _requireNotBlacklisted(settings, msg.sender);
_requireNotBlacklisted(settings, runner);
...
}
function batchCollect(address runner) external {
+ _requireNotBlacklisted(settings, msg.sender);
_requireNotBlacklisted(settings, runner);
...
}
function collectEra(uint256 era, bytes32 deploymentId, address runner) external {
+ _requireNotBlacklisted(settings, msg.sender);
_requireNotBlacklisted(settings, runner);
...
}
function batchCollectEra(uint256 era, address runner) external {
+ _requireNotBlacklisted(settings, msg.sender);
_requireNotBlacklisted(settings, runner);
...
}Also applies to: 215-215, 227-227, 241-241, 255-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/RewardsPool.sol` at line 153, The blacklist checks currently call
_requireNotBlacklisted(settings, runner) but do not block a blacklisted
msg.sender from delegating through a non-blacklisted runner; update each
occurrence (including the instances at the same pattern around lines ~153, 215,
227, 241, 255) to enforce the blacklist for both parties by calling
_requireNotBlacklisted(settings, msg.sender) in addition to
_requireNotBlacklisted(settings, runner) (or replace with a new helper that
validates both addresses) so that both the caller (msg.sender) and the runner
are checked before proceeding.
Comment on lines
254
to
+256
| function applyStakeChange(address runner, address staker) external { | ||
| _requireNotBlacklisted(settings, staker); | ||
|
|
There was a problem hiding this comment.
Also blacklist-gate runner in applyStakeChange().
This path still settles runner state (getRewardInfo(runner), claimFrom(runner, staker), _updateTotalStakingAmount(..., runner, ...), onStakeUpdate(runner)), but only staker is checked. A runner blacklisted after queuing a stake change can still have rewards/stake state progressed by any caller.
Suggested fix
function applyStakeChange(address runner, address staker) external {
+ _requireNotBlacklisted(settings, runner);
_requireNotBlacklisted(settings, staker);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/RewardsStaking.sol` around lines 254 - 256, The applyStakeChange
function currently only calls _requireNotBlacklisted for staker but proceeds to
modify runner state (getRewardInfo, claimFrom, _updateTotalStakingAmount,
onStakeUpdate); add a blacklist gate for runner by calling
_requireNotBlacklisted(settings, runner) at the start of applyStakeChange
(before any calls to getRewardInfo, claimFrom, _updateTotalStakingAmount, or
onStakeUpdate) so blacklisted runners cannot have their reward/stake state
progressed by this path.
Comment on lines
+122
to
+123
| // Expected token balance: tracks actual transfers in/out for balance invariant check | ||
| uint256 public expectedBalance; |
There was a problem hiding this comment.
expectedBalance will drift from the real token balance.
This new invariant starts at 0, so an on-chain upgrade leaves all pre-existing stake unaccounted for. It also only increments on transferDelegationTokens(), but Staking receives additional inflows via reward restaking in contracts/StakingManager.sol Lines 205-211 (claimForDelegate() followed by addDelegation(...)). After either case, the next expectedBalance -= amount in withdraw/slash can underflow or hit S023 even though the tokens are actually there.
Seed expectedBalance during the upgrade and update it on every inflow path, not just direct deposits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/Staking.sol` around lines 122 - 123, The expectedBalance invariant
currently starts at 0 and is only updated in transferDelegationTokens(), causing
drift and possible underflow in withdraw/slash; seed expectedBalance to the
actual token balance during the upgrade/migration and update it wherever tokens
enter the contract (not just transferDelegationTokens()), specifically add
updates in the reward restake path (handle inflows from
StakingManager.claimForDelegate() and subsequent addDelegation(...) calls) so
every inflow increments expectedBalance and every outflow (withdraw/slash)
decrements it to keep the invariant consistent.
Comment on lines
122
to
+124
| function unstake(address _runner, uint256 _amount) external { | ||
| _requireNotBlacklisted(settings, _runner); | ||
|
|
There was a problem hiding this comment.
Don't block the IndexerRegistry unregister path here.
Per the existing function contract, IndexerRegistry calls unstake() to remove a runner completely. With the new guard at the top, a blacklisted runner can no longer be force-unregistered because the registry call reverts before the msg.sender == IndexerRegistry branch is reached.
A safer pattern is to keep the blacklist check only on the self-service branch, so blacklisted runners cannot call unstake() themselves while the protocol can still clean them up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/StakingManager.sol` around lines 122 - 124, The added
_requireNotBlacklisted(settings, _runner) at the top of unstake() prevents
IndexerRegistry from force-unregistering blacklisted runners; change the guard
so blacklisted checks only apply to the self-service path: remove the top-level
_requireNotBlacklisted(settings, _runner) and instead call
_requireNotBlacklisted(settings, _runner) only inside the branch where
msg.sender == _runner (the self-initiated unstake path), leaving the msg.sender
== IndexerRegistry branch untouched so protocol-driven unregisters succeed.
Comment on lines
+720
to
+723
| function _requireChannelWalletsNotBlacklisted(uint256 channelId) private view { | ||
| _requireNotBlacklisted(settings, channels[channelId].indexer); | ||
| _requireNotBlacklisted(settings, channels[channelId].consumer); | ||
| } |
There was a problem hiding this comment.
This helper is too coarse for channel settlement paths.
Because claim, respond, terminate, and checkpoint all rely on this, blacklisting either stored channel wallet can leave an already-open channel impossible to advance or finalize. Also, for contract consumers this checks only channels[channelId].consumer (the wrapper contract), not the resolved wallet from channelConsumer(channelId).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces comprehensive blacklist checks across multiple smart contracts to enhance security and prevent blacklisted accounts from interacting with protocol functions. The main change is the systematic addition of _requireNotBlacklisted checks for all user-facing and sensitive methods in contracts related to consumers, rewards, and staking. This ensures that blacklisted addresses cannot perform actions such as deposits, withdrawals, claiming rewards, or managing controllers.
Blacklist Enforcement Across Contracts
ConsumerHost, including controller management, deposits, withdrawals, and state channel interactions. This prevents both the caller and affected addresses from being blacklisted. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]RewardsBoosterfor all user-facing methods, including booster management, reward collection, and authorization modifiers. Also added a newadminRemoveBoosterDeploymentfunction for owner-controlled booster removal. [1] [2] [3] [4] [5] [6]RewardsDistributorfor all reward distribution, claim, and agreement-related actions, ensuring neither consumers nor indexers can be blacklisted. [1] [2] [3] [4] [5] [6]RewardsPool, including labor accounting, reward collection, and batch operations. [1] [2] [3] [4] [5]RewardsStakingto inherit fromSQParameterand enforce blacklist restrictions on staking changes and indexer registry events. [1] [2] [3] [4]Summary by CodeRabbit
New Features
Updates