Code4rena · S-43 · Jan 2025
Liquid Ron
// Inverted Boolean Logic in onlyOperator Modifier Causes Permanent DoS for All Operators
EXECUTIVE SUMMARY
ROOT CAUSE ANALYSIS
The condition `msg.sender != owner() || operator[msg.sender]` evaluates to true (revert) for any authorized operator who is not also the owner. The || operator makes the second condition (operator[msg.sender] == true) trigger the revert path instead of the allow path.
modifier onlyOperator() {
// WRONG: || causes inversion — operators always revert
if (msg.sender != owner() || operator[msg.sender])
revert ErrInvalidOperator();
_;
}
// Truth table:
// sender=owner, operator[x]=false → NOT revert ✓ (owner OK)
// sender=owner, operator[x]=true → NOT revert ✓ (owner OK)
// sender=operator, operator[x]=true → REVERTS ✗ (should pass!)
// sender=random, operator[x]=false → REVERTS ✓ (correctly blocked)
// Test proof — fails with ErrInvalidOperator():
// liquidRon.updateOperator(protocolAdmin, true);
// vm.prank(protocolAdmin);
// liquidRon.harvest(0, consensusAddrs); // ← REVERT▸ INCORRECT ASSUMPTIONS
- —The || condition correctly allows both owner and operator to pass
- —operator[msg.sender] == true means the sender is authorized (it actually triggers revert)
- —Access control tests covered the non-owner-operator path — they did not
▸ AFFECTED CONTRACTS
- LiquidRon.sol — all functions guarded by onlyOperator (harvest, etc.)
ATTACK SCENARIO
Protocol admin deploys LiquidRon and calls updateOperator(protocolAdmin, true) to grant operator access.
↓Admin calls harvest() or any onlyOperator-protected function from the protocolAdmin address.
↓Modifier evaluates: (protocolAdmin != owner() → true) || (operator[protocolAdmin] → true) → condition is true → ErrInvalidOperator revert.
↓All operator-gated protocol operations are permanently inaccessible to all granted operators. Only the owner can call these functions — the operator role provides no access whatsoever.
IMPACT ASSESSMENT
Access Control Failure
The operator role is entirely non-functional. updateOperator(x, true) effectively revokes access rather than granting it. No granted operator can call any protected function.
Denial of Service
All operator-gated protocol operations (harvest, etc.) are permanently unavailable to operators. Critical protocol maintenance functions require owner-key availability at all times.
Operational Risk
If the owner key is in cold storage or a multisig with slow signing, time-sensitive operator operations (like harvest) stall indefinitely with no fallback.
MITIGATION
Replace || with && and flip the operator condition. The correct logic: revert only if the sender is NEITHER the owner NOR an authorized operator.
modifier onlyOperator() {
if (msg.sender != owner() || operator[msg.sender])
revert ErrInvalidOperator();
_;
}modifier onlyOperator() {
// allow: owner OR authorized operator
if (msg.sender != owner() && !operator[msg.sender])
revert ErrInvalidOperator();
_;
}▸ SECURITY CONSIDERATIONS
- 01.Add an explicit test: grant operator role to a non-owner address, call every onlyOperator function, assert no revert
- 02.Consider OpenZeppelin AccessControl to eliminate custom modifier boolean logic entirely
- 03.Write the truth table as a comment above any custom access modifier for future maintainers
RESEARCHER NOTES
This is a pure logic bug hidden in a three-word modifier. It looks plausible at first glance — 'if sender is not owner OR is an operator, revert' almost reads correctly until you trace the truth table explicitly.
My approach for custom access modifiers: always write the full truth table before trusting the condition. For onlyOperator with two state variables (owner match + operator map), there are four cases. One read-through of the table showed the inversion immediately.
The most dangerous aspect is how long this could go undetected. If the owner handles all operator-gated calls during early deployment (common), the bug only surfaces when someone first tries to use a granted operator role — potentially long after launch.
This reinforced a personal rule: for every permission-granting function, verify that granting the permission actually enables access. updateOperator(x, true) should make x able to call protected functions. Verifying this trivially catches the entire class of inverted-logic access control bugs.
