Vulnerability Report: Missing Ownership Verification in PolicyClientRegistry.registerClient

gistfile1.txt

Vulnerability Report: Missing Ownership Verification in PolicyClientRegistry.registerClient

Executive Summary

The PolicyClientRegistry.registerClient function lacks verification that the caller is the actual owner of the policy client being registered. This allows any address to register as the "registry owner" of any INewtonPolicyClient contract, enabling them to control the client's active status and block identity linking operations in the dependent IdentityRegistry.

Vulnerability Details

Location

File: PolicyClientRegistry.sol
Function: registerClient(address client)
Lines: 37-50

Vulnerable Code

solidity


function registerClient(
    address client
) external {
    require(
        IERC165(client).supportsInterface(type(INewtonPolicyClient).interfaceId),
        NotPolicyClient(client)
    );
    require(_clients[client].registeredAt == 0, ClientAlreadyRegistered(client));

    _clients[client] =
        ClientRecord({owner: msg.sender, active: true, registeredAt: uint64(block.timestamp)});
    _ownerClients[msg.sender].add(client);

    emit ClientRegistered(client, msg.sender);
}
Root Cause Analysis

The function performs only two checks before registration:

ERC165 Interface Check: Verifies the client implements INewtonPolicyClient
Duplication Check: Ensures the client hasn't been registered before
Missing: Verification that msg.sender is authorized to register the client. The INewtonPolicyClient interface exposes a getOwner() function, but it is never called during registration.

The consequence is that whoever calls registerClient first becomes the "registry owner" stored in _clients[client].owner, regardless of their relationship to the actual policy client contract.

Impact Analysis

Direct Impact

Once an attacker becomes the registry owner, they gain control over:

deactivateClient(client) - Sets active = false
activateClient(client) - Sets active = true
setClientOwner(client, newOwner) - Transfers registry ownership
Downstream Impact on IdentityRegistry

The IdentityRegistry contract depends on PolicyClientRegistry.isRegisteredClient() to gate identity linking operations:

solidity


function isRegisteredClient(
    address client
) external view returns (bool) {
    return _clients[client].registeredAt != 0 && _clients[client].active;
}
When an attacker calls deactivateClient(), this function returns false, causing all identity linking operations to revert at the check:

solidity


require(
    IPolicyClientRegistry(policyClientRegistry).isRegisteredClient(_policyClient),
    // error
);
Severity Justification

Factor	Assessment	Rationale
Impact	Medium	Causes denial-of-service for identity linking; no direct fund loss
Likelihood	High	Permissionless function; front-running is standard and trivial
Overall	HIGH	Combination of feasibility and significant operational disruption
Exploitation Scenarios

Scenario 1: Front-Running Registration

Preconditions:

A legitimate policy client C is deployed and implements INewtonPolicyClient
The rightful owner submits a public mempool transaction to registerClient(C)
Attacker monitors the mempool
Attack Steps:

Attacker observes pending registerClient(C) transaction in mempool
Attacker submits identical registerClient(C) call with higher gas price
Attacker's transaction executes first, storing attacker as _clients[C].owner
Rightful owner's transaction reverts with ClientAlreadyRegistered(C)
Attacker calls deactivateClient(C)
All IdentityRegistry linking operations for client C now revert
Result: Complete denial-of-service for identity linking to client C.

Scenario 2: Preemptive Registration

Preconditions:

A legitimate policy client C is deployed but not yet registered
Attacker discovers C's address before the rightful owner registers
Attack Steps:

Attacker calls registerClient(C) before the rightful owner
Attacker becomes _clients[C].owner
Attacker immediately calls deactivateClient(C)
Rightful owner cannot register (already registered) or activate (not registry owner)
Result: Policy client C is permanently blocked from IdentityRegistry integration without attacker cooperation.

Scenario 3: Silent Squatting with Delayed Activation

Preconditions:

Attacker successfully registers C first (via Scenario 1 or 2)
Attacker keeps active = true initially
Attack Progression:

Attacker registers C and leaves it active
Users begin linking identities through client C
At a strategically chosen time, attacker calls deactivateClient(C)
All new identity linking operations fail
Attacker may demand off-chain concessions to reactivate or transfer ownership
Result: Operational disruption with potential extortion vector.

Recovery Limitations

The vulnerable contract provides no recovery mechanism for hijacked registry records:

No admin override function to reassign ownership
No governance capability to invalidate malicious registrations
The rightful owner cannot fix this on-chain without attacker cooperation
The only recourse would be:

Protocol upgrade/migration to a new registry
Off-chain negotiation with the attacker
Deploying a new policy client contract and registering it correctly
Recommended Fix

Primary Fix: Verify Caller Against Policy Client Owner

Add ownership verification by calling INewtonPolicyClient(client).getOwner():

solidity


function registerClient(
    address client
) external {
    require(
        IERC165(client).supportsInterface(type(INewtonPolicyClient).interfaceId),
        NotPolicyClient(client)
    );
    require(_clients[client].registeredAt == 0, ClientAlreadyRegistered(client));
    require(
        INewtonPolicyClient(client).getOwner() == msg.sender, 
        NotClientOwner(client, msg.sender)
    );

    _clients[client] =
        ClientRecord({owner: msg.sender, active: true, registeredAt: uint64(block.timestamp)});
    _ownerClients[msg.sender].add(client);

    emit ClientRegistered(client, msg.sender);
}
Secondary Fix: Update _requireClientOwner for Consistency

Ensure ongoing ownership checks also verify against the policy client's actual owner:

solidity


function _requireClientOwner(
    address client
) private view {
    require(_clients[client].registeredAt != 0, ClientNotRegistered(client));
    require(
        INewtonPolicyClient(client).getOwner() == msg.sender, 
        NotClientOwner(client, msg.sender)
    );
}
This ensures that if the policy client's on-chain owner changes, the registry operations follow the actual ownership rather than stale registry data.

Additional Considerations

Synchronization Between Registry and Policy Client Ownership

The secondary fix introduces a design consideration: should registry ownership be:

Static at registration time (current design intent) - The registry owner is set once and can only be changed via setClientOwner
Dynamic based on policy client owner (secondary fix approach) - Registry operations always defer to INewtonPolicyClient(client).getOwner()
Trade-offs:

Approach	Pros	Cons
Static	Gas efficient for repeated operations; clear ownership model	Registry can become out-of-sync with actual client ownership
Dynamic	Always reflects true ownership	Additional external call per operation; dependency on policy client implementation
Open Questions

Should setClientOwner be removed if using dynamic ownership? If the registry always checks INewtonPolicyClient.getOwner(), the registry's owner field becomes redundant.
What happens if INewtonPolicyClient.getOwner() reverts? The fix assumes this function is always callable. If the policy client can be in a state where getOwner() reverts, all registry operations would also revert.
Should there be an admin recovery mechanism? Consider adding a protocol-owned override for cases where legitimate recovery is needed (e.g., compromised owner keys on the policy client).
Conclusion

The missing ownership verification in registerClient creates a high-severity vulnerability allowing unauthorized parties to control policy client registry records and deny service to identity linking operations. The recommended fix is straightforward: verify msg.sender against INewtonPolicyClient(client).getOwner() before allowing registration.