Skip to content

Commit 4e6c8d1

Browse files
committed
Apply code quality, documentation, and architecture improvements
1 parent 1ea7bb2 commit 4e6c8d1

20 files changed

Lines changed: 136 additions & 73 deletions

AGENTS.md

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,13 @@ All three share their core logic through `RuleEngineBase` directly or via `RuleE
5555
```
5656
RuleEngineBase (abstract)
5757
├── VersionModule → version() returns "3.0.0"
58-
├── RulesManagementModule → add/remove/set/clear rules
58+
├── RulesManagementModule → add/remove/set/clear rules, maxRules cap
5959
│ ├── AccessControl (OZ)
6060
│ └── RulesManagementModuleInvariantStorage → errors, events, roles
61-
├── ERC3643ComplianceModule → bind/unbind tokens
62-
│ └── IERC3643Compliance
61+
├── ERC3643ComplianceExtendedModule → bind/unbind tokens (extended API)
62+
│ └── ERC3643ComplianceModule → core ERC-3643 compliance
63+
│ ├── IERC3643Compliance
64+
│ └── ERC3643ComplianceModuleInvariantStorage → errors
6365
├── RuleEngineInvariantStorage → errors
6466
└── IRuleEngineERC1404 → CMTAT interface
6567
@@ -87,16 +89,19 @@ Modules define **virtual internal hooks** for access control. Concrete contracts
8789
```solidity
8890
// In RulesManagementModule (abstract):
8991
function _onlyRulesManager() internal virtual;
92+
function _onlyRulesLimitManager() internal virtual; // guards setMaxRules
9093
9194
// In ERC3643ComplianceModule (abstract):
9295
function _onlyComplianceManager() internal virtual;
9396
9497
// RuleEngine overrides with RBAC:
9598
function _onlyRulesManager() internal virtual override onlyRole(RULES_MANAGEMENT_ROLE) {}
99+
function _onlyRulesLimitManager() internal virtual override onlyRole(DEFAULT_ADMIN_ROLE) {}
96100
function _onlyComplianceManager() internal virtual override onlyRole(COMPLIANCE_MANAGER_ROLE) {}
97101
98102
// RuleEngineOwnable overrides with Ownable:
99103
function _onlyRulesManager() internal virtual override onlyOwner {}
104+
function _onlyRulesLimitManager() internal virtual override onlyOwner {}
100105
function _onlyComplianceManager() internal virtual override onlyOwner {}
101106
```
102107

@@ -127,7 +132,7 @@ function _checkRule(address rule_) internal view virtual override {
127132
### Rule Execution Flow
128133

129134
```
130-
Token operation → RuleEngine.transferred(spender, from, to, value) ← used by CMTAT v3.3.0+ for all operations
135+
Token operation → RuleEngine.transferred(spender, from, to, value) ← CMTAT v3.3.0+ primary path
131136
├── onlyBoundToken modifier (caller must be bound)
132137
└── for each rule in _rules:
133138
rule.transferred(spender, from, to, value) // reverts if disallowed
@@ -136,10 +141,20 @@ Token operation → RuleEngine.transferred(spender, from, to, value) ← used
136141
├── onlyBoundToken modifier
137142
└── for each rule in _rules:
138143
rule.transferred(from, to, value)
144+
145+
RuleEngine.created(to, value) ← ERC-3643 mint entry point
146+
├── onlyBoundToken modifier
147+
└── calls _transferred(address(0), to, value)
148+
149+
RuleEngine.destroyed(from, value) ← ERC-3643 burn entry point
150+
├── onlyBoundToken modifier
151+
└── calls _transferred(from, address(0), value)
139152
```
140153

141154
Since CMTAT v3.3.0, mint (`from == address(0)`) and burn (`to == address(0)`) also go through the 4-argument overload with the operator as `spender`. Rules that check `spender` must skip or adapt that check for mint/burn to avoid blocking those operations unintentionally.
142155

156+
`created` and `destroyed` use the 3-argument `_transferred` path (no spender), consistent with the ERC-3643 spec which does not carry a spender for mint/burn.
157+
143158
View path: `detectTransferRestriction()` iterates rules, returns first non-zero code.
144159

145160
### Storage: EnumerableSet

CLAUDE.md

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,13 @@ All three share their core logic through `RuleEngineBase` directly or via `RuleE
5555
```
5656
RuleEngineBase (abstract)
5757
├── VersionModule → version() returns "3.0.0"
58-
├── RulesManagementModule → add/remove/set/clear rules
58+
├── RulesManagementModule → add/remove/set/clear rules, maxRules cap
5959
│ ├── AccessControl (OZ)
6060
│ └── RulesManagementModuleInvariantStorage → errors, events, roles
61-
├── ERC3643ComplianceModule → bind/unbind tokens
62-
│ └── IERC3643Compliance
61+
├── ERC3643ComplianceExtendedModule → bind/unbind tokens (extended API)
62+
│ └── ERC3643ComplianceModule → core ERC-3643 compliance
63+
│ ├── IERC3643Compliance
64+
│ └── ERC3643ComplianceModuleInvariantStorage → errors
6365
├── RuleEngineInvariantStorage → errors
6466
└── IRuleEngineERC1404 → CMTAT interface
6567
@@ -87,16 +89,19 @@ Modules define **virtual internal hooks** for access control. Concrete contracts
8789
```solidity
8890
// In RulesManagementModule (abstract):
8991
function _onlyRulesManager() internal virtual;
92+
function _onlyRulesLimitManager() internal virtual; // guards setMaxRules
9093
9194
// In ERC3643ComplianceModule (abstract):
9295
function _onlyComplianceManager() internal virtual;
9396
9497
// RuleEngine overrides with RBAC:
9598
function _onlyRulesManager() internal virtual override onlyRole(RULES_MANAGEMENT_ROLE) {}
99+
function _onlyRulesLimitManager() internal virtual override onlyRole(DEFAULT_ADMIN_ROLE) {}
96100
function _onlyComplianceManager() internal virtual override onlyRole(COMPLIANCE_MANAGER_ROLE) {}
97101
98102
// RuleEngineOwnable overrides with Ownable:
99103
function _onlyRulesManager() internal virtual override onlyOwner {}
104+
function _onlyRulesLimitManager() internal virtual override onlyOwner {}
100105
function _onlyComplianceManager() internal virtual override onlyOwner {}
101106
```
102107

@@ -127,7 +132,7 @@ function _checkRule(address rule_) internal view virtual override {
127132
### Rule Execution Flow
128133

129134
```
130-
Token operation → RuleEngine.transferred(spender, from, to, value) ← used by CMTAT v3.3.0+ for all operations
135+
Token operation → RuleEngine.transferred(spender, from, to, value) ← CMTAT v3.3.0+ primary path
131136
├── onlyBoundToken modifier (caller must be bound)
132137
└── for each rule in _rules:
133138
rule.transferred(spender, from, to, value) // reverts if disallowed
@@ -136,10 +141,20 @@ Token operation → RuleEngine.transferred(spender, from, to, value) ← used
136141
├── onlyBoundToken modifier
137142
└── for each rule in _rules:
138143
rule.transferred(from, to, value)
144+
145+
RuleEngine.created(to, value) ← ERC-3643 mint entry point
146+
├── onlyBoundToken modifier
147+
└── calls _transferred(address(0), to, value)
148+
149+
RuleEngine.destroyed(from, value) ← ERC-3643 burn entry point
150+
├── onlyBoundToken modifier
151+
└── calls _transferred(from, address(0), value)
139152
```
140153

141154
Since CMTAT v3.3.0, mint (`from == address(0)`) and burn (`to == address(0)`) also go through the 4-argument overload with the operator as `spender`. Rules that check `spender` must skip or adapt that check for mint/burn to avoid blocking those operations unintentionally.
142155

156+
`created` and `destroyed` use the 3-argument `_transferred` path (no spender), consistent with the ERC-3643 spec which does not carry a spender for mint/burn.
157+
143158
View path: `detectTransferRestriction()` iterates rules, returns first non-zero code.
144159

145160
### Storage: EnumerableSet

README.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,7 @@ The toolchain includes the following components, where the versions are the late
279279
- Foundry (forge-std) [v1.14.0](https://github.com/foundry-rs/forge-std/releases/tag/v1.14.0)
280280
- Solidity [0.8.34](https://docs.soliditylang.org/en/v0.8.34/)
281281
- OpenZeppelin Contracts (submodule) [v5.6.1](https://github.com/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.6.1)
282-
- CMTAT [v3.2.0](https://github.com/CMTA/CMTAT/releases/tag/v3.2.0)
282+
- CMTAT [v3.3.0](https://github.com/CMTA/CMTAT/releases/tag/v3.3.0)
283283

284284
### Access Control
285285

@@ -377,8 +377,8 @@ For function signatures, struct arguments are represented with their correspondi
377377
| | `addRule(address rule_)` | public | `IRule rule_` |-|RULES_MANAGEMENT_ROLE|
378378
| | `removeRule(address rule_)` | public | `IRule rule_` |-|RULES_MANAGEMENT_ROLE|
379379
| ERC3643ComplianceModule | | | | | |
380-
| | `bindToken(address token)` | public | `address token` | - | COMPLIANCE_MANAGER_ROLE |
381-
| | `unbindToken(address token)` | public | `address token` | - | COMPLIANCE_MANAGER_ROLE |
380+
| | `bindToken(address token)` | public | `address token` | - | COMPLIANCE_MANAGER_ROLE or approved token self-call |
381+
| | `unbindToken(address token)` | public | `address token` | - | COMPLIANCE_MANAGER_ROLE or approved token self-call |
382382
| ERC3643ComplianceExtendedModule | | | | | |
383383
| | `bindTokens(address[] tokens)` | public | `address[] tokens` | - | COMPLIANCE_MANAGER_ROLE |
384384
| | `unbindTokens(address[] tokens)` | public | `address[] tokens` | - | COMPLIANCE_MANAGER_ROLE |

src/deployment/RuleEngine.sol

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,11 @@ contract RuleEngine is ERC2771ModuleStandalone, RuleEngineBase, AccessControlEnu
3939
/* ============ ACCESS CONTROL ============ */
4040
/**
4141
* @notice Grants `role` to `account`.
42-
* @dev Prevents granting any role to accounts already configured as rules.
42+
* @dev Prevents granting any role to accounts currently configured as rules.
43+
* Note: this check is intentionally one-directional. {addRule} does not verify
44+
* whether the rule address already holds a privileged role, and this function does
45+
* not prevent adding a privileged address as a rule afterwards. Operators are
46+
* responsible for keeping rule contracts and privileged accounts disjoint.
4347
*/
4448
function grantRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) {
4549
if (_rules.contains(account)) {

src/deployment/RuleEngineOwnable2Step.sol

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,10 @@ import {Ownable2StepInterfaceId} from "../modules/library/Ownable2StepInterfaceI
1414
*/
1515
contract RuleEngineOwnable2Step is RuleEngineOwnableShared, Ownable2Step {
1616
/**
17-
* @param owner_ Address of the contract owner (ERC-173)
18-
* @param forwarderIrrevocable Address of the forwarder, required for the gasless support
19-
* @param tokenContract Address of the token contract to bind (can be zero address)
17+
* @notice Deploys a RuleEngine with ERC-173 two-step ownership transfer.
18+
* @param owner_ Address of the initial contract owner (ERC-173).
19+
* @param forwarderIrrevocable Address of the ERC-2771 trusted forwarder (use zero address to disable gasless).
20+
* @param tokenContract Address of the token contract to bind at deployment (use zero address to skip).
2021
*/
2122
constructor(address owner_, address forwarderIrrevocable, address tokenContract)
2223
RuleEngineOwnableShared(forwarderIrrevocable, tokenContract)

src/interfaces/IRulesManagementModule.sol

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ interface IRulesManagementModule {
1414
/**
1515
* @notice Updates the maximum number of rules allowed in the engine.
1616
* @dev Access control is implementation specific (admin/owner).
17+
* Setting a very high cap re-exposes unbounded gas cost for administrative operations
18+
* such as {clearRules} even though normal per-transfer cost remains bounded.
1719
* @param maxRules_ New maximum number of rules.
1820
*/
1921
function setMaxRules(uint256 maxRules_) external;
@@ -60,9 +62,9 @@ interface IRulesManagementModule {
6062
/**
6163
* @notice Removes all configured rules.
6264
* @dev After calling this function, no rules will remain set.
63-
* Developers should keep in mind that this function has an unbounded cost
64-
* and using it may render the function uncallable if the set grows to the point
65-
* where clearing it consumes too much gas to fit in a block.
65+
* Cost is O(n) in the number of configured rules. With the default cap of 10 this is
66+
* negligible, but a high {maxRules} setting re-exposes unbounded cost here even though
67+
* per-transfer cost remains bounded.
6668
*/
6769
function clearRules() external;
6870

@@ -88,5 +90,5 @@ interface IRulesManagementModule {
8890
* @dev Complexity: O(1).
8991
* @return exists True if the rule is present, false otherwise.
9092
*/
91-
function containsRule(IRule rule_) external returns (bool exists);
93+
function containsRule(IRule rule_) external view returns (bool exists);
9294
}

src/mocks/rules/operation/RuleMintAllowance.sol

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ contract RuleMintAllowance is AccessControl, RuleMintAllowanceInvariantStorage,
5353
* @notice Called for transfers where no spender context is available.
5454
* Mint allowance cannot be enforced without a spender; passes through.
5555
*/
56-
function transferred(address from, address to, uint256 value) public view {
56+
function transferred(address from, address to, uint256 value) public {
5757
// no-op: spender unknown, enforcement requires transferred(spender,...)
5858
}
5959

src/mocks/rules/validation/RuleWhitelist.sol

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,8 @@
22

33
pragma solidity ^0.8.20;
44

5-
// forge-lint: disable-next-line(unaliased-plain-import)
6-
import "./abstract/RuleAddressList/RuleAddressList.sol";
7-
// forge-lint: disable-next-line(unaliased-plain-import)
8-
import "./abstract/RuleWhitelistCommon.sol";
5+
import {RuleAddressList} from "./abstract/RuleAddressList/RuleAddressList.sol";
6+
import {RuleWhitelistCommon} from "./abstract/RuleWhitelistCommon.sol";
97
import {AccessControl} from "@openzeppelin/contracts/access/AccessControl.sol";
108
import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
119
import {RuleInterfaceId} from "../../../modules/library/RuleInterfaceId.sol";
@@ -92,12 +90,12 @@ contract RuleWhitelist is RuleAddressList, RuleWhitelistCommon {
9290
}
9391
}
9492

95-
function transferred(address from, address to, uint256 value) public view {
93+
function transferred(address from, address to, uint256 value) public {
9694
uint8 code = detectTransferRestriction(from, to, value);
9795
require(code == uint8(REJECTED_CODE_BASE.TRANSFER_OK), RuleWhitelist_InvalidTransfer(from, to, value, code));
9896
}
9997

100-
function transferred(address spender, address from, address to, uint256 value) public view {
98+
function transferred(address spender, address from, address to, uint256 value) public {
10199
uint8 code = detectTransferRestrictionFrom(spender, from, to, value);
102100
require(code == uint8(REJECTED_CODE_BASE.TRANSFER_OK), RuleWhitelist_InvalidTransfer(from, to, value, code));
103101
}

src/modules/ERC3643ComplianceExtendedModule.sol

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,12 @@ abstract contract ERC3643ComplianceExtendedModule is ERC3643ComplianceModule, IE
1212

1313
mapping(address token => bool approved) private _tokenSelfBindingApproval;
1414

15-
/// @inheritdoc IERC3643ComplianceExtended
15+
/**
16+
* @inheritdoc IERC3643ComplianceExtended
17+
* @custom:security-note See {bindToken} for multi-tenant accounting risks. All tokens bound
18+
* in this batch share the same rule state. Only bind tokens that are equally trusted and
19+
* governed together.
20+
*/
1621
function bindTokens(address[] calldata tokens) public virtual override onlyComplianceManager {
1722
for (uint256 i = 0; i < tokens.length; ++i) {
1823
_bindToken(tokens[i]);

src/modules/ERC3643ComplianceModule.sol

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,23 +7,20 @@ import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet
77
import {Context} from "@openzeppelin/contracts/utils/Context.sol";
88
/* ==== Interface and other library === */
99
import {IERC3643Compliance} from "../interfaces/IERC3643Compliance.sol";
10-
11-
abstract contract ERC3643ComplianceModule is Context, IERC3643Compliance {
10+
import {ERC3643ComplianceModuleInvariantStorage} from "./library/ERC3643ComplianceModuleInvariantStorage.sol";
11+
import {ERC3643ComplianceRolesStorage} from "./library/ERC3643ComplianceRolesStorage.sol";
12+
13+
abstract contract ERC3643ComplianceModule is
14+
Context,
15+
IERC3643Compliance,
16+
ERC3643ComplianceModuleInvariantStorage,
17+
ERC3643ComplianceRolesStorage
18+
{
1219
/* ==== Type declaration === */
1320
using EnumerableSet for EnumerableSet.AddressSet;
1421
/* ==== State Variables === */
1522
// Token binding tracking
1623
EnumerableSet.AddressSet internal _boundTokens;
17-
// Access Control
18-
// Will not be present in the final bytecode if not used
19-
bytes32 public constant COMPLIANCE_MANAGER_ROLE = keccak256("COMPLIANCE_MANAGER_ROLE");
20-
21-
/* ==== Errors === */
22-
error RuleEngine_ERC3643Compliance_InvalidTokenAddress();
23-
error RuleEngine_ERC3643Compliance_TokenAlreadyBound();
24-
error RuleEngine_ERC3643Compliance_TokenNotBound();
25-
error RuleEngine_ERC3643Compliance_UnauthorizedCaller();
26-
error RuleEngine_ERC3643Compliance_OperationNotSuccessful();
2724

2825
/* ==== Modifier === */
2926
modifier onlyBoundToken() {
@@ -46,6 +43,10 @@ abstract contract ERC3643ComplianceModule is Context, IERC3643Compliance {
4643
* @dev Operator warning: "multi-tenant" means one RuleEngine is shared by
4744
* multiple token contracts. In that setup, bind only tokens that are equally
4845
* trusted and governed together.
46+
* @custom:security-note Operation rules (stateful rules such as `RuleConditionalTransferLight`
47+
* or `RuleMintAllowance`) maintain per-address accounting that is shared across all bound tokens.
48+
* Binding tokens from different issuers to the same engine will silently cross-contaminate
49+
* their accounting. Only bind tokens that are equally trusted and governed together.
4950
*/
5051
function bindToken(address token) public virtual override {
5152
_authorizeComplianceBindingChange(token);

0 commit comments

Comments
 (0)