From f035773910054a07b2e7f30285ab5c1e94332180 Mon Sep 17 00:00:00 2001 From: Elena Nadolinski Date: Tue, 23 Jun 2026 10:29:08 -0700 Subject: [PATCH] feat: emit msg.sender in cert revocation events (I-02) Address BLOCKSEC-5249 finding I-02. Generated with Claude Code Co-Authored-By: Claude --- src/CertManager.sol | 29 ++++++++----- src/ICertManager.sol | 4 +- test/CertManager.t.sol | 53 +++++++++++++++++++++++- test/hinted/HintedNitroAttestation.t.sol | 22 +++++----- 4 files changed, 82 insertions(+), 26 deletions(-) diff --git a/src/CertManager.sol b/src/CertManager.sol index d1d9515..895d39f 100644 --- a/src/CertManager.sol +++ b/src/CertManager.sol @@ -17,9 +17,16 @@ contract CertManager is ICertManager { using LibAsn1Ptr for Asn1Ptr; using LibBytes for bytes; + 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); @@ -78,11 +85,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_) { @@ -107,12 +114,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. @@ -168,13 +175,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; } @@ -195,12 +202,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 { @@ -235,7 +242,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( diff --git a/src/ICertManager.sol b/src/ICertManager.sol index 0c22351..636c3ba 100644 --- a/src/ICertManager.sol +++ b/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); } diff --git a/test/CertManager.t.sol b/test/CertManager.t.sol index cc40cfd..80985c0 100644 --- a/test/CertManager.t.sol +++ b/test/CertManager.t.sol @@ -386,12 +386,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); + } +} diff --git a/test/hinted/HintedNitroAttestation.t.sol b/test/hinted/HintedNitroAttestation.t.sol index 3606bbd..90121ae 100644 --- a/test/hinted/HintedNitroAttestation.t.sol +++ b/test/hinted/HintedNitroAttestation.t.sol @@ -390,10 +390,10 @@ 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); @@ -401,7 +401,7 @@ contract HintedNitroAttestationTest is Test { bytes32 otherCertHash = keccak256("other cert"); vm.prank(address(0xCAFE)); - vm.expectRevert("not revoker"); + vm.expectRevert(CertManager.NotRevoker.selector); certManager.revokeCert(otherCertHash); vm.prank(newRevoker); @@ -409,24 +409,24 @@ contract HintedNitroAttestationTest is Test { 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)); @@ -725,10 +725,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");