feat: emit msg.sender in cert revocation events (I-02) (#47)

Address BLOCKSEC-5249 finding I-02.

Generated with Claude Code

Co-authored-by: Claude <noreply@anthropic.com>

Author: leanthebean

src/CertManager.sol

@@ -21,10 +21,16 @@ contract CertManager is ICertManager {
     error InvalidBasicConstraints();
     error InvalidSubjectPublicKey();
     error UnsupportedCriticalExtension();
+    error NotOwner();
+    error NotRevoker();
+    error IncompleteCertChain();
+    error DeprecatedEntrypoint();
+    error InvalidOwner();
+    error InvalidRevoker();
 
     event CertVerified(bytes32 indexed certHash);
-    event CertRevoked(bytes32 indexed certHash);
-    event CertUnrevoked(bytes32 indexed certHash);
+    event CertRevoked(bytes32 indexed certHash, address indexed account);
+    event CertUnrevoked(bytes32 indexed certHash, address indexed account);
     event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
     event RevokerUpdated(address indexed previousRevoker, address indexed newRevoker);
 
@@ -83,11 +89,11 @@ contract CertManager is ICertManager {
     }
 
     function _onlyOwner() internal view {
-        require(msg.sender == owner, "not owner");
+        if (msg.sender != owner) revert NotOwner();
     }
 
     function _onlyRevoker() internal view {
-        require(msg.sender == revoker, "not revoker");
+        if (msg.sender != revoker) revert NotRevoker();
     }
 
     constructor(IP384Verifier p384Verifier_) {
@@ -112,12 +118,12 @@ contract CertManager is ICertManager {
     /// @notice DEPRECATED — always reverts. The fully on-chain (non-hinted) path is too expensive
     ///         post-Fusaka and has been removed. Use {verifyCACertWithHints}.
     function verifyCACert(bytes memory, bytes32) external pure returns (bytes32) {
-        revert("use hinted cert verification");
+        revert DeprecatedEntrypoint();
     }
 
     /// @notice DEPRECATED — always reverts. Use {verifyClientCertWithHints}.
     function verifyClientCert(bytes memory, bytes32) external pure returns (VerifiedCert memory) {
-        revert("use hinted cert verification");
+        revert DeprecatedEntrypoint();
     }
 
     /// @notice Verify a CA certificate against its (already-cached) parent and cache the result.
@@ -173,13 +179,13 @@ contract CertManager is ICertManager {
     }
 
     function transferOwnership(address newOwner) external onlyOwner {
-        require(newOwner != address(0), "invalid owner");
+        if (newOwner == address(0)) revert InvalidOwner();
         emit OwnershipTransferred(owner, newOwner);
         owner = newOwner;
     }
 
     function setRevoker(address newRevoker) external onlyOwner {
-        require(newRevoker != address(0), "invalid revoker");
+        if (newRevoker == address(0)) revert InvalidRevoker();
         emit RevokerUpdated(revoker, newRevoker);
         revoker = newRevoker;
     }
@@ -200,12 +206,12 @@ contract CertManager is ICertManager {
 
     function unrevokeCert(bytes32 certId) external onlyOwner {
         revoked[certId] = false;
-        emit CertUnrevoked(certId);
+        emit CertUnrevoked(certId, msg.sender);
     }
 
     function _revokeCert(bytes32 certId) internal {
         revoked[certId] = true;
-        emit CertRevoked(certId);
+        emit CertRevoked(certId, msg.sender);
     }
 
     function _requireCanRevoke(bytes32 certId) internal view {
@@ -240,7 +246,7 @@ contract CertManager is ICertManager {
         // Fail closed: a chain that terminates at bytes32(0) without reaching the pinned root is
         // broken and must not be treated as a verified, non-revoked chain. Reverting here instead
         // of returning silently means revocation safety never depends on upstream guards.
-        revert("incomplete cert chain");
+        revert IncompleteCertChain();
     }
 
     function _verifyCert(

src/ICertManager.sol

@@ -45,9 +45,9 @@ interface ICertManager {
 
     // --- DEPRECATED: these always revert; use the *WithHints variants above. ---
 
-    /// @dev DEPRECATED — always reverts ("use hinted cert verification").
+    /// @dev DEPRECATED — always reverts.
     function verifyCACert(bytes memory cert, bytes32 parentCertHash) external returns (bytes32);
 
-    /// @dev DEPRECATED — always reverts ("use hinted cert verification").
+    /// @dev DEPRECATED — always reverts.
     function verifyClientCert(bytes memory cert, bytes32 parentCertHash) external returns (VerifiedCert memory);
 }

test/CertManager.t.sol

@@ -500,12 +500,61 @@ contract RequireCachedChainNotRevokedTest is Test {
         // verifiedParent[PARENT] is unset (bytes32(0)), so the chain is broken: it can never
         // reach ROOT_CA_CERT_HASH. The fixed function must fail closed instead of returning.
         cm.setParent(CHILD, PARENT);
-        vm.expectRevert("incomplete cert chain");
+        vm.expectRevert(CertManager.IncompleteCertChain.selector);
         cm.requireCachedChainNotRevoked(CHILD);
     }
 
     function test_RevertsOnZeroCertHash() public {
-        vm.expectRevert("incomplete cert chain");
+        vm.expectRevert(CertManager.IncompleteCertChain.selector);
         cm.requireCachedChainNotRevoked(bytes32(0));
     }
 }
+
+/// @dev Regression coverage for BLOCKSEC-5249 finding I-02: `CertRevoked` / `CertUnrevoked` now
+///      include the acting `msg.sender` as an indexed topic so revocation activity is monitorable.
+contract CertRevocationEventTest is Test {
+    CertManager internal cm;
+
+    event CertRevoked(bytes32 indexed certHash, address indexed account);
+    event CertUnrevoked(bytes32 indexed certHash, address indexed account);
+
+    bytes32 internal constant CERT_ID = bytes32(uint256(0xabc));
+
+    function setUp() public {
+        // Deployer is both owner and revoker, so this contract can revoke/unrevoke directly.
+        cm = new CertManager(new P384Verifier());
+    }
+
+    function test_RevokeCertEmitsSender() public {
+        vm.expectEmit(true, true, false, true, address(cm));
+        emit CertRevoked(CERT_ID, address(this));
+        cm.revokeCert(CERT_ID);
+    }
+
+    function test_RevokeCertsEmitsSender() public {
+        bytes32[] memory ids = new bytes32[](1);
+        ids[0] = CERT_ID;
+        vm.expectEmit(true, true, false, true, address(cm));
+        emit CertRevoked(CERT_ID, address(this));
+        cm.revokeCerts(ids);
+    }
+
+    function test_UnrevokeCertEmitsSender() public {
+        cm.revokeCert(CERT_ID);
+        vm.expectEmit(true, true, false, true, address(cm));
+        emit CertUnrevoked(CERT_ID, address(this));
+        cm.unrevokeCert(CERT_ID);
+    }
+
+    /// @dev The recorded account is the actual caller, not the contract: a delegated revoker
+    ///      address shows up in the event topic.
+    function test_RevokeCertRecordsActualCaller() public {
+        address delegatedRevoker = address(0xBEEF);
+        cm.setRevoker(delegatedRevoker);
+
+        vm.expectEmit(true, true, false, true, address(cm));
+        emit CertRevoked(CERT_ID, delegatedRevoker);
+        vm.prank(delegatedRevoker);
+        cm.revokeCert(CERT_ID);
+    }
+}

test/hinted/HintedNitroAttestation.t.sol

@@ -390,43 +390,43 @@ contract HintedNitroAttestationTest is Test {
 
         address newRevoker = address(0xBEEF);
         vm.prank(address(0xCAFE));
-        vm.expectRevert("not owner");
+        vm.expectRevert(CertManager.NotOwner.selector);
         certManager.setRevoker(newRevoker);
 
-        vm.expectRevert("invalid revoker");
+        vm.expectRevert(CertManager.InvalidRevoker.selector);
         certManager.setRevoker(address(0));
 
         certManager.setRevoker(newRevoker);
         assertEq(certManager.revoker(), newRevoker);
 
         bytes32 otherCertHash = keccak256("other cert");
         vm.prank(address(0xCAFE));
-        vm.expectRevert("not revoker");
+        vm.expectRevert(CertManager.NotRevoker.selector);
         certManager.revokeCert(otherCertHash);
 
         vm.prank(newRevoker);
         certManager.revokeCert(otherCertHash);
         assertTrue(certManager.revoked(otherCertHash));
 
         vm.prank(newRevoker);
-        vm.expectRevert("not owner");
+        vm.expectRevert(CertManager.NotOwner.selector);
         certManager.unrevokeCert(otherCertHash);
 
         certManager.unrevokeCert(otherCertHash);
         assertFalse(certManager.revoked(otherCertHash));
 
-        vm.expectRevert("invalid owner");
+        vm.expectRevert(CertManager.InvalidOwner.selector);
         certManager.transferOwnership(address(0));
 
         address newOwner = address(0xA11CE);
         vm.prank(address(0xCAFE));
-        vm.expectRevert("not owner");
+        vm.expectRevert(CertManager.NotOwner.selector);
         certManager.transferOwnership(newOwner);
 
         certManager.transferOwnership(newOwner);
         assertEq(certManager.owner(), newOwner);
 
-        vm.expectRevert("not owner");
+        vm.expectRevert(CertManager.NotOwner.selector);
         certManager.setRevoker(address(0x1234));
 
         vm.prank(newOwner);
@@ -455,7 +455,7 @@ contract HintedNitroAttestationTest is Test {
         bytes32 rootHash = certManager.ROOT_CA_CERT_HASH();
 
         vm.prank(newRevoker);
-        vm.expectRevert("not owner");
+        vm.expectRevert(CertManager.NotOwner.selector);
         certManager.revokeCert(rootHash);
         assertFalse(certManager.revoked(rootHash));
 
@@ -464,7 +464,7 @@ contract HintedNitroAttestationTest is Test {
         certHashes[1] = rootHash;
 
         vm.prank(newRevoker);
-        vm.expectRevert("not owner");
+        vm.expectRevert(CertManager.NotOwner.selector);
         certManager.revokeCerts(certHashes);
         assertFalse(certManager.revoked(certHashes[0]));
         assertFalse(certManager.revoked(rootHash));
@@ -738,10 +738,10 @@ contract HintedNitroAttestationTest is Test {
     }
 
     function test_DeployableCertManagerDisablesUnhintedEntrypoints() public {
-        vm.expectRevert("use hinted cert verification");
+        vm.expectRevert(CertManager.DeprecatedEntrypoint.selector);
         certManager.verifyCACert("", bytes32(0));
 
-        vm.expectRevert("use hinted cert verification");
+        vm.expectRevert(CertManager.DeprecatedEntrypoint.selector);
         certManager.verifyClientCert("", bytes32(0));
 
         vm.expectRevert("use hinted attestation verification");