Skip to content

Commit

Permalink
Merge branch 'feat/audit-fix-2024-09-registry' of https://github.com/…
Browse files Browse the repository at this point in the history 
…zkemail/ether-email-auth into feat/audit-fix-2024-09-registry
  • Loading branch information
@SoraSuegami
SoraSuegami committed Oct 22, 2024
2 parents 11f33c3 + 19dcbd9 commit 01b3ba8
Show file tree
Hide file tree
Showing 7 changed files with 118 additions and 158 deletions.
10 changes: 9 additions & 1 deletion packages/contracts/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,15 @@ It requires a function `isDKIMPublicKeyHashValid(string domainName, bytes32 publ
One of its implementations is [`ECDSAOwnedDKIMRegistry`](https://github.com/zkemail/ether-email-auth/blob/main/packages/contracts/src/utils/ECDSAOwnedDKIMRegistry.sol).
It stores the Ethereum address `signer` who can update the registry.

We also provide another implementation called [`ForwardDKIMRegistry`](https://github.com/zkemail/ether-email-auth/blob/main/packages/contracts/src/utils/ForwardDKIMRegistry.sol). It stores an address of any internal DKIM registry and forwards its outputs. We can use it to upgrade a proxy of the ECDSAOwnedDKIMRegistry registry to a new DKIM registry with a different storage slots design by 1) upgrading its implementation into ForwardDKIMRegistry and 2) calling `resetStorageForUpgradeFromECDSAOwnedDKIMRegistry` function with an address of the internal DKIM registry.
We also provide another implementation called [`UserOverrideableDKIMRegistry`](https://github.com/zkemail/ether-email-auth/blob/main/packages/contracts/node_modules/@zk-email/contracts/UserOverrideableDKIMRegistry.sol).

Key features of `UserOverrideableDKIMRegistry` include:
1. User-specific public key setting
2. Time-delayed activation of main authorizer's approvals
3. Dual revocation mechanism (user or main authorizer)
4. Reactivation of revoked keys (by users only)

This implementation provides a balance between centralized management and user autonomy in the DKIM registry system.

### `Verifier` Contract
It has the responsibility to verify a ZK proof for the [`email_auth_with_body_parsing_with_qp_encoding.circom` circuit](https://github.com/zkemail/ether-email-auth/blob/main/packages/circuits/src/email_auth_with_body_parsing_with_qp_encoding.circom).
Expand Down
136 changes: 64 additions & 72 deletions packages/contracts/test/DKIMRegistryUpgrade.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,76 +14,68 @@ import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/Own
import {UserOverrideableDKIMRegistry} from "@zk-email/contracts/UserOverrideableDKIMRegistry.sol";

contract DKIMRegistryUpgradeTest is StructHelper {
// function setUp() public override {
// super.setUp();
// vm.startPrank(deployer);
// emailAuth.initialize(deployer, accountSalt, deployer);
// vm.expectEmit(true, false, false, false);
// emit EmailAuth.VerifierUpdated(address(verifier));
// emailAuth.updateVerifier(address(verifier));
// vm.expectEmit(true, false, false, false);
// emit EmailAuth.DKIMRegistryUpdated(address(dkim));
// emailAuth.updateDKIMRegistry(address(dkim));
// UserOverrideableDKIMRegistry overrideableDkimImpl = new UserOverrideableDKIMRegistry();
// ERC1967Proxy overrideableDkimProxy = new ERC1967Proxy(
// address(overrideableDkimImpl),
// abi.encodeCall(
// overrideableDkimImpl.initialize,
// (deployer, deployer, setTimestampDelay)
// )
// );
// UserOverrideableDKIMRegistry overrideableDkim = UserOverrideableDKIMRegistry(
// address(overrideableDkimProxy)
// );
// overrideableDkim.setDKIMPublicKeyHash(
// domainName,
// publicKeyHash,
// deployer,
// new bytes(0)
// );
// dkim.upgradeToAndCall(
// address(overrideableDkimImpl),
// abi.encodeCall(
// forwardDkimImpl
// .resetStorageForUpgradeFromECDSAOwnedDKIMRegistry,
// (address(overrideableDkim))
// )
// );
// vm.stopPrank();
// }
// function testDkimRegistryAddr() public view {
// address dkimAddr = emailAuth.dkimRegistryAddr();
// assertEq(dkimAddr, address(dkim));
// }
// function _testInsertCommandTemplate() private {
// emailAuth.insertCommandTemplate(templateId, commandTemplate);
// string[] memory result = emailAuth.getCommandTemplate(templateId);
// assertEq(result, commandTemplate);
// }
// function testAuthEmail() public {
// vm.startPrank(deployer);
// _testInsertCommandTemplate();
// EmailAuthMsg memory emailAuthMsg = buildEmailAuthMsg();
// vm.stopPrank();
// assertEq(
// emailAuth.usedNullifiers(emailAuthMsg.proof.emailNullifier),
// false
// );
// assertEq(emailAuth.lastTimestamp(), 0);
// vm.startPrank(deployer);
// vm.expectEmit(true, true, true, true);
// emit EmailAuth.EmailAuthed(
// emailAuthMsg.proof.emailNullifier,
// emailAuthMsg.proof.accountSalt,
// emailAuthMsg.proof.isCodeExist,
// emailAuthMsg.templateId
// );
// emailAuth.authEmail(emailAuthMsg);
// vm.stopPrank();
// assertEq(
// emailAuth.usedNullifiers(emailAuthMsg.proof.emailNullifier),
// true
// );
// assertEq(emailAuth.lastTimestamp(), emailAuthMsg.proof.timestamp);
// }
function setUp() public override {
super.setUp();
vm.startPrank(deployer);
emailAuth.initialize(deployer, accountSalt, deployer);
vm.expectEmit(true, false, false, false);
emit EmailAuth.VerifierUpdated(address(verifier));
emailAuth.updateVerifier(address(verifier));
vm.expectEmit(true, false, false, false);
emit EmailAuth.DKIMRegistryUpdated(address(dkim));
emailAuth.updateDKIMRegistry(address(dkim));
UserOverrideableDKIMRegistry overrideableDkimImpl = new UserOverrideableDKIMRegistry();
ERC1967Proxy overrideableDkimProxy = new ERC1967Proxy(
address(overrideableDkimImpl),
abi.encodeCall(
overrideableDkimImpl.initialize,
(deployer, deployer, setTimestampDelay)
)
);
UserOverrideableDKIMRegistry overrideableDkim = UserOverrideableDKIMRegistry(
address(overrideableDkimProxy)
);
overrideableDkim.setDKIMPublicKeyHash(
domainName,
publicKeyHash,
deployer,
new bytes(0)
);
vm.stopPrank();
}
function testDkimRegistryAddr() public view {
address dkimAddr = emailAuth.dkimRegistryAddr();
assertEq(dkimAddr, address(dkim));
}
function _testInsertCommandTemplate() private {
emailAuth.insertCommandTemplate(templateId, commandTemplate);
string[] memory result = emailAuth.getCommandTemplate(templateId);
assertEq(result, commandTemplate);
}
function testAuthEmail() public {
vm.startPrank(deployer);
_testInsertCommandTemplate();
EmailAuthMsg memory emailAuthMsg = buildEmailAuthMsg();
vm.stopPrank();
assertEq(
emailAuth.usedNullifiers(emailAuthMsg.proof.emailNullifier),
false
);
assertEq(emailAuth.lastTimestamp(), 0);
vm.startPrank(deployer);
vm.expectEmit(true, true, true, true);
emit EmailAuth.EmailAuthed(
emailAuthMsg.proof.emailNullifier,
emailAuthMsg.proof.accountSalt,
emailAuthMsg.proof.isCodeExist,
emailAuthMsg.templateId
);
emailAuth.authEmail(emailAuthMsg);
vm.stopPrank();
assertEq(
emailAuth.usedNullifiers(emailAuthMsg.proof.emailNullifier),
true
);
assertEq(emailAuth.lastTimestamp(), emailAuthMsg.proof.timestamp);
}
}
25 changes: 0 additions & 25 deletions packages/contracts/test/EmailAuth.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,31 +58,6 @@ contract EmailAuthTest is StructHelper {
assertEq(emailAuth.dkimRegistryAddr(), address(newDKIM));
}

// function testUpdateDKIMRegistryToForward() public {
// assertEq(emailAuth.dkimRegistryAddr(), address(dkim));

// vm.startPrank(deployer);
// ECDSAOwnedDKIMRegistry dummyDKIM = new ECDSAOwnedDKIMRegistry();
// ForwardDKIMRegistry newDKIM;
// {
// ForwardDKIMRegistry dkimImpl = new ForwardDKIMRegistry();
// ERC1967Proxy dkimProxy = new ERC1967Proxy(
// address(dkimImpl),
// abi.encodeCall(
// dkimImpl.initialize,
// (msg.sender, address(dummyDKIM))
// )
// );
// newDKIM = ForwardDKIMRegistry(address(dkimProxy));
// }
// vm.expectEmit(true, false, false, false);
// emit EmailAuth.DKIMRegistryUpdated(address(newDKIM));
// emailAuth.updateDKIMRegistry(address(newDKIM));
// vm.stopPrank();

// assertEq(emailAuth.dkimRegistryAddr(), address(newDKIM));
// }

function testExpectRevertUpdateDKIMRegistryInvalidDkimRegistryAddress()
public
{
Expand Down
29 changes: 10 additions & 19 deletions packages/contracts/test/IntegrationZKSync.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,20 @@ import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "../src/EmailAuth.sol";
import "../src/utils/Verifier.sol";
import "../src/utils/Groth16Verifier.sol";
import "../src/utils/ForwardDKIMRegistry.sol";
import "./helpers/SimpleWallet.sol";
import "./helpers/RecoveryControllerZKSync.sol";
import "forge-std/console.sol";
import "../src/utils/ZKSyncCreate2Factory.sol";
import {UserOverrideableDKIMRegistry} from "@zk-email/contracts/UserOverrideableDKIMRegistry.sol";
import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol";

contract IntegrationZKSyncTest is Test {
using Strings for *;
using console for *;

EmailAuth emailAuth;
Verifier verifier;
ForwardDKIMRegistry dkim;
UserOverrideableDKIMRegistry dkim;

RecoveryControllerZKSync recoveryControllerZKSync;
SimpleWallet simpleWallet;
Expand Down Expand Up @@ -54,27 +54,18 @@ contract IntegrationZKSyncTest is Test {
// Create DKIM registry
UserOverrideableDKIMRegistry overrideableDkimImpl = new UserOverrideableDKIMRegistry();
{
ForwardDKIMRegistry forwardDkimImpl = new ForwardDKIMRegistry();
ERC1967Proxy forwardDkimProxy = new ERC1967Proxy(
address(forwardDkimImpl),
ERC1967Proxy overrideableDkimProxy = new ERC1967Proxy(
address(overrideableDkimImpl),
abi.encodeCall(
forwardDkimImpl.initializeWithUserOverrideableDKIMRegistry,
(
msg.sender,
address(overrideableDkimImpl),
signer,
setTimeDelay
)
overrideableDkimImpl.initialize,
(msg.sender, signer, setTimeDelay)
)
);
dkim = ForwardDKIMRegistry(address(forwardDkimProxy));
dkim = UserOverrideableDKIMRegistry(address(overrideableDkimProxy));
}
{
UserOverrideableDKIMRegistry overrideableDkimProxy = UserOverrideableDKIMRegistry(
address(dkim.sourceDKIMRegistry())
);
string memory signedMsg = overrideableDkimProxy.computeSignedMsg(
overrideableDkimProxy.SET_PREFIX(),
string memory signedMsg = dkim.computeSignedMsg(
dkim.SET_PREFIX(),
domainName,
publicKeyHash
);
Expand All @@ -83,7 +74,7 @@ contract IntegrationZKSyncTest is Test {
);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(1, digest);
bytes memory signature = abi.encodePacked(r, s, v);
overrideableDkimProxy.setDKIMPublicKeyHash(
dkim.setDKIMPublicKeyHash(
domainName,
publicKeyHash,
signer,
Expand Down
1 change: 0 additions & 1 deletion packages/contracts/test/helpers/DeploymentHelper.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.s

// // FOR_ZKSYNC:START
// import {ZKSyncCreate2Factory} from "../../src/utils/ZKSyncCreate2Factory.sol";
// import "../../src/utils/ForwardDKIMRegistry.sol";
// import {RecoveryControllerZKSync, EmailAccountRecoveryZKSync} from "./RecoveryControllerZKSync.sol";
// // FOR_ZKSYNC:END

Expand Down
4 changes: 0 additions & 4 deletions packages/contracts/test/script/RenounceOwnersScript.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import "forge-std/Test.sol";
import "forge-std/console.sol";

import {Deploy} from "../../script/DeployRecoveryController.s.sol";
// import {Deploy as Deploy2} from "../../script/DeployForwardDKIMRegistry.s.sol";
import {RenounceOwners} from "../../script/RenounceOwners.s.sol";
import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import {StructHelper} from "../helpers/StructHelper.sol";
Expand All @@ -24,9 +23,6 @@ contract RenounceOwnersScriptTest is StructHelper {

Deploy deploy = new Deploy();
deploy.run();
// vm.setEnv("SOURCE_DKIM", vm.toString(vm.envAddress("ECDSA_DKIM")));
// Deploy2 deploy2 = new Deploy2();
// deploy2.run();
RenounceOwners renounceOwners = new RenounceOwners();
renounceOwners.run();
address verifier = vm.envAddress("VERIFIER");
Expand Down
71 changes: 35 additions & 36 deletions packages/contracts/test/script/UpgradesScript.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import "forge-std/Test.sol";
import "forge-std/console.sol";

import {Deploy} from "../../script/DeployRecoveryController.s.sol";
// import {Deploy as Deploy2} from "../../script/DeployForwardDKIMRegistry.s.sol";
import {Upgrades} from "../../script/Upgrades.s.sol";
import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
Expand All @@ -31,43 +30,43 @@ contract UpgradesScriptTest is StructHelper {
// vm.setEnv("SOURCE_DKIM", vm.toString(vm.envAddress("ECDSA_DKIM")));
// Deploy2 deploy2 = new Deploy2();
// deploy2.run();
// address verifier = vm.envAddress("VERIFIER");
// address ecdsaDkimAddr = vm.envAddress("ECDSA_DKIM");
// address dkim = vm.envAddress("DKIM");
// address verifierImpl = _readAddressFromSlot(
// verifier,
// IMPLEMENTATION_SLOT
// );
// address ecdsaDkimImpl = _readAddressFromSlot(
// ecdsaDkimAddr,
// IMPLEMENTATION_SLOT
// );
// address dkimImpl = _readAddressFromSlot(dkim, IMPLEMENTATION_SLOT);
address verifier = vm.envAddress("VERIFIER");
address ecdsaDkimAddr = vm.envAddress("ECDSA_DKIM");
address dkim = vm.envAddress("DKIM");
address verifierImpl = _readAddressFromSlot(
verifier,
IMPLEMENTATION_SLOT
);
address ecdsaDkimImpl = _readAddressFromSlot(
ecdsaDkimAddr,
IMPLEMENTATION_SLOT
);
address dkimImpl = _readAddressFromSlot(dkim, IMPLEMENTATION_SLOT);
Upgrades upgrades = new Upgrades();
upgrades.run();
// assertNotEq(
// verifierImpl,
// _readAddressFromSlot(verifier, IMPLEMENTATION_SLOT)
// );
// assertNotEq(
// ecdsaDkimImpl,
// _readAddressFromSlot(ecdsaDkimAddr, IMPLEMENTATION_SLOT)
// );
// assertNotEq(dkimImpl, _readAddressFromSlot(dkim, IMPLEMENTATION_SLOT));
assertNotEq(
verifierImpl,
_readAddressFromSlot(verifier, IMPLEMENTATION_SLOT)
);
assertNotEq(
ecdsaDkimImpl,
_readAddressFromSlot(ecdsaDkimAddr, IMPLEMENTATION_SLOT)
);
assertNotEq(dkimImpl, _readAddressFromSlot(dkim, IMPLEMENTATION_SLOT));
}

// function _readAddressFromSlot(
// address contractAddress,
// uint256 slot
// ) private view returns (address) {
// address value;
// assembly {
// // Create a pointer to the slot
// let ptr := mload(0x40)
// mstore(ptr, slot)
// // Read the value from the slot
// value := sload(add(ptr, contractAddress))
// }
// return value;
// }
function _readAddressFromSlot(
address contractAddress,
uint256 slot
) private view returns (address) {
address value;
assembly {
// Create a pointer to the slot
let ptr := mload(0x40)
mstore(ptr, slot)
// Read the value from the slot
value := sload(add(ptr, contractAddress))
}
return value;
}
}

0 comments on commit 01b3ba8

Please sign in to comment.