Skip to content
This repository was archived by the owner on May 9, 2024. It is now read-only.

Commit fe8628b

Browse files
nmlinaricnmlinaric
authored
Implement EIP712 for signing (#628)
1 parent 076fc37 commit fe8628b

File tree

13 files changed

+351
-231
lines changed

13 files changed

+351
-231
lines changed

‎contracts/Bridge.sol‎

Lines changed: 50 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,10 @@ pragma solidity 0.8.11;
33

44
import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
55
import "@openzeppelin/contracts/utils/Context.sol";
6+
import "@openzeppelin/contracts/utils/cryptography/draft-EIP712.sol";
67
import "./utils/Pausable.sol";
78

9+
810
import "./interfaces/IDepositExecute.sol";
911
import "./interfaces/IERCHandler.sol";
1012
import "./interfaces/IGenericHandler.sol";
@@ -15,9 +17,15 @@ import "./interfaces/IAccessControlSegregator.sol";
1517
@title Facilitates deposits and creation of deposit proposals, and deposit executions.
1618
@author ChainSafe Systems.
1719
*/
18-
contract Bridge is Pausable, Context {
20+
contract Bridge is Pausable, Context, EIP712 {
1921
using ECDSA for bytes32;
2022

23+
bytes32 private constant _PROPOSALS_TYPEHASH =
24+
keccak256("Proposals(Proposal[] proposals)Proposal(uint8 originDomainID,uint64 depositNonce,bytes32 resourceID,bytes data)");
25+
bytes32 private constant _PROPOSAL_TYPEHASH =
26+
keccak256("Proposal(uint8 originDomainID,uint64 depositNonce,bytes32 resourceID,bytes data)");
27+
28+
2129
uint8 public immutable _domainID;
2230
address public _MPCAddress;
2331

@@ -96,7 +104,7 @@ contract Bridge is Pausable, Context {
96104
@param domainID ID of chain the Bridge contract exists on.
97105
@param accessControl Address of access control contract.
98106
*/
99-
constructor (uint8 domainID, address accessControl) public {
107+
constructor (uint8 domainID, address accessControl) EIP712("Bridge", "3.1.0") public {
100108
_domainID = domainID;
101109
_accessControl = IAccessControlSegregator(accessControl);
102110

@@ -271,37 +279,28 @@ contract Bridge is Pausable, Context {
271279

272280
/**
273281
@notice Executes a deposit proposal using a specified handler contract (only if signature is signed by MPC).
274-
@param originDomainID ID of chain deposit originated from.
275-
@param resourceID ResourceID to be used when making deposits.
276-
@param depositNonce ID of deposit generated by origin Bridge contract.
277-
@param data Data originally provided when deposit was made.
282+
@notice Failed executeProposal from handler don't revert, emits {FailedHandlerExecution} event.
283+
@param proposal Proposal which consists of:
284+
- originDomainID ID of chain deposit originated from.
285+
- resourceID ResourceID to be used when making deposits.
286+
- depositNonce ID of deposit generated by origin Bridge contract.
287+
- data Data originally provided when deposit was made.
278288
@param signature bytes memory signature composed of MPC key shares
279289
@notice Emits {ProposalExecution} event.
280290
@notice Behaviour of this function is different for {GenericHandler} and other specific ERC handlers.
281291
In the case of ERC handler, when execution fails, the handler will terminate the function with revert.
282292
In the case of {GenericHandler}, when execution fails, the handler will emit a failure event and terminate the function normally.
283293
*/
284-
function executeProposal(uint8 originDomainID, uint64 depositNonce, bytes calldata data, bytes32 resourceID, bytes calldata signature) public whenNotPaused {
285-
require(isProposalExecuted(originDomainID, depositNonce) != true, "Deposit with provided nonce already executed");
286-
287-
address signer = keccak256(abi.encode(originDomainID, _domainID, depositNonce, data, resourceID)).recover(signature);
288-
require(signer == _MPCAddress, "Invalid message signer");
289-
290-
address handler = _resourceIDToHandlerAddress[resourceID];
291-
bytes32 dataHash = keccak256(abi.encodePacked(handler, data));
294+
function executeProposal(Proposal memory proposal, bytes calldata signature) public {
295+
Proposal[] memory proposalArray = new Proposal[](1);
296+
proposalArray[0] = proposal;
292297

293-
IDepositExecute depositHandler = IDepositExecute(handler);
294-
295-
usedNonces[originDomainID][depositNonce / 256] |= 1 << (depositNonce % 256);
296-
297-
// Reverts for every handler except GenericHandler
298-
depositHandler.executeProposal(resourceID, data);
299-
300-
emit ProposalExecution(originDomainID, depositNonce, dataHash);
298+
executeProposals(proposalArray, signature);
301299
}
302300

303301
/**
304302
@notice Executes a batch of deposit proposals using a specified handler contract for each proposal (only if signature is signed by MPC).
303+
@notice If executeProposals fails it doesn't revert, emits {FailedHandlerExecution} event.
305304
@param proposals Array of Proposal which consists of:
306305
- originDomainID ID of chain deposit originated from.
307306
- resourceID ResourceID to be used when making deposits.
@@ -313,11 +312,9 @@ contract Bridge is Pausable, Context {
313312
In the case of ERC handler, when execution fails, the handler will terminate the function with revert.
314313
In the case of {GenericHandler}, when execution fails, the handler will emit a failure event and terminate the function normally.
315314
*/
316-
function executeProposals(Proposal[] memory proposals, bytes memory signature) public whenNotPaused {
315+
function executeProposals(Proposal[] memory proposals, bytes calldata signature) public whenNotPaused {
317316
require(proposals.length > 0, "Proposals can't be an empty array");
318-
319-
address signer = keccak256(abi.encode(proposals, _domainID)).recover(signature);
320-
require(signer == _MPCAddress, "Invalid message signer");
317+
require(verify(proposals, signature), "Invalid proposal signer");
321318

322319
for (uint256 i = 0; i < proposals.length; i++) {
323320
if(isProposalExecuted(proposals[i].originDomainID, proposals[i].depositNonce)) {
@@ -399,4 +396,31 @@ contract Bridge is Pausable, Context {
399396
function isProposalExecuted(uint8 domainID, uint256 depositNonce) public view returns (bool) {
400397
return usedNonces[domainID][depositNonce / 256] & (1 << (depositNonce % 256)) != 0;
401398
}
399+
400+
/**
401+
@notice Verifies that proposal data is signed by MPC address.
402+
@param proposals array of Proposals.
403+
@param signature signature bytes memory signature composed of MPC key shares.
404+
@return Boolean value depending if signer is vaild or not.
405+
*/
406+
function verify(Proposal[] memory proposals, bytes calldata signature) public view returns (bool) {
407+
bytes32[] memory keccakData = new bytes32[](proposals.length);
408+
for (uint256 i = 0; i < proposals.length; i++) {
409+
keccakData[i] = keccak256(
410+
abi.encode(
411+
_PROPOSAL_TYPEHASH,
412+
proposals[i].originDomainID,
413+
proposals[i].depositNonce,
414+
proposals[i].resourceID,
415+
keccak256(proposals[i].data)
416+
)
417+
);
418+
}
419+
420+
address signer = _hashTypedDataV4(
421+
keccak256(abi.encode(
422+
_PROPOSALS_TYPEHASH, keccak256(abi.encodePacked(keccakData))))
423+
).recover(signature);
424+
return signer == _MPCAddress;
425+
}
402426
}

‎test/contractBridge/executeProposal.js‎

Lines changed: 33 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ contract('Bridge - [execute proposal]', async (accounts) => {
3939

4040
let data = '';
4141
let dataHash = '';
42+
let proposal;
4243

4344
beforeEach(async () => {
4445
await Promise.all([
@@ -71,6 +72,13 @@ contract('Bridge - [execute proposal]', async (accounts) => {
7172
depositProposalData = Helpers.createERCDepositData(depositAmount, 20, recipientAddress)
7273
depositProposalDataHash = Ethers.utils.keccak256(ERC20HandlerInstance.address + depositProposalData.substr(2));
7374

75+
proposal = {
76+
originDomainID: originDomainID,
77+
depositNonce: expectedDepositNonce,
78+
resourceID: resourceID,
79+
data: depositProposalData
80+
};
81+
7482
// set MPC address to unpause the Bridge
7583
await BridgeInstance.endKeygen(Helpers.mpcAddress);
7684
});
@@ -79,10 +87,10 @@ contract('Bridge - [execute proposal]', async (accounts) => {
7987
const destinationDomainID = await BridgeInstance._domainID();
8088

8189
assert.isFalse(await BridgeInstance.isProposalExecuted(destinationDomainID, expectedDepositNonce));
82-
});
90+
});
8391

84-
it('should create and execute executeProposal successfully', async () => {
85-
const proposalSignedData = await Helpers.signDataWithMpc(originDomainID, destinationDomainID, expectedDepositNonce, depositProposalData, resourceID);
92+
it('should create and execute executeProposal successfully', async () => {
93+
const proposalSignedData = await Helpers.signTypedProposal(BridgeInstance.address, [proposal]);
8694

8795
// depositorAddress makes initial deposit of depositAmount
8896
assert.isFalse(await BridgeInstance.paused());
@@ -95,12 +103,9 @@ contract('Bridge - [execute proposal]', async (accounts) => {
95103
));
96104

97105
await TruffleAssert.passes(BridgeInstance.executeProposal(
98-
originDomainID,
99-
expectedDepositNonce,
100-
depositProposalData,
101-
resourceID,
102-
proposalSignedData,
103-
{ from: relayer1Address }
106+
proposal,
107+
proposalSignedData,
108+
{ from: relayer1Address }
104109
));
105110

106111
// check that deposit nonce has been marked as used in bitmap
@@ -111,8 +116,8 @@ contract('Bridge - [execute proposal]', async (accounts) => {
111116
assert.strictEqual(recipientBalance.toNumber(), depositAmount);
112117
});
113118

114-
it('should fail to executeProposal if deposit nonce is already used', async () => {
115-
const proposalSignedData = await Helpers.signDataWithMpc(originDomainID, destinationDomainID, expectedDepositNonce, depositProposalData, resourceID);
119+
it('should skip executing proposal if deposit nonce is already used', async () => {
120+
const proposalSignedData = await Helpers.signTypedProposal(BridgeInstance.address, [proposal]);
116121

117122
// depositorAddress makes initial deposit of depositAmount
118123
assert.isFalse(await BridgeInstance.paused());
@@ -125,26 +130,23 @@ contract('Bridge - [execute proposal]', async (accounts) => {
125130
));
126131

127132
await TruffleAssert.passes(BridgeInstance.executeProposal(
128-
originDomainID,
129-
expectedDepositNonce,
130-
depositProposalData,
131-
resourceID,
133+
proposal,
132134
proposalSignedData,
133135
{ from: relayer1Address }
134136
));
135137

136-
await TruffleAssert.reverts(BridgeInstance.executeProposal(
137-
originDomainID,
138-
expectedDepositNonce,
139-
depositProposalData,
140-
resourceID,
141-
proposalSignedData,
142-
{ from: relayer1Address }
143-
), "Deposit with provided nonce already executed");
138+
const skipExecuteTx = await BridgeInstance.executeProposal(
139+
proposal,
140+
proposalSignedData,
141+
{ from: relayer1Address }
142+
);
143+
144+
// check that no ProposalExecution events are emitted
145+
assert.equal(skipExecuteTx.logs.length, 0);
144146
});
145147

146148
it('executeProposal event should be emitted with expected values', async () => {
147-
const proposalSignedData = await Helpers.signDataWithMpc(originDomainID, destinationDomainID, expectedDepositNonce, depositProposalData, resourceID);
149+
const proposalSignedData = await Helpers.signTypedProposal(BridgeInstance.address, [proposal]);
148150

149151
// depositorAddress makes initial deposit of depositAmount
150152
assert.isFalse(await BridgeInstance.paused());
@@ -157,13 +159,10 @@ contract('Bridge - [execute proposal]', async (accounts) => {
157159
));
158160

159161
const proposalTx = await BridgeInstance.executeProposal(
160-
originDomainID,
161-
expectedDepositNonce,
162-
depositProposalData,
163-
resourceID,
162+
proposal,
164163
proposalSignedData,
165-
{ from: relayer1Address }
166-
);
164+
{ from: relayer1Address }
165+
);
167166

168167
TruffleAssert.eventEmitted(proposalTx, 'ProposalExecution', (event) => {
169168
return event.originDomainID.toNumber() === originDomainID &&
@@ -179,8 +178,8 @@ contract('Bridge - [execute proposal]', async (accounts) => {
179178
assert.strictEqual(recipientBalance.toNumber(), depositAmount);
180179
});
181180

182-
it('should fail to executeProposal if signed destinationDomainID in not the domain on which proposal should be executed', async () => {
183-
const proposalSignedData = await Helpers.signDataWithMpc(originDomainID, invalidDestinationDomainID, expectedDepositNonce, depositProposalData, resourceID);
181+
it('should fail to executeProposal if signed Proposal has different chainID than the one on which it should be executed', async () => {
182+
const proposalSignedData = await Helpers.mockSignTypedProposalWithInvalidChainID(BridgeInstance.address, [proposal]);
184183

185184
// depositorAddress makes initial deposit of depositAmount
186185
assert.isFalse(await BridgeInstance.paused());
@@ -193,12 +192,9 @@ contract('Bridge - [execute proposal]', async (accounts) => {
193192
));
194193

195194
await TruffleAssert.reverts(BridgeInstance.executeProposal(
196-
originDomainID,
197-
expectedDepositNonce,
198-
depositProposalData,
199-
resourceID,
195+
proposal,
200196
proposalSignedData,
201197
{ from: relayer1Address }
202-
), "Invalid message signer");
198+
), "Invalid proposal signer");
203199
});
204200
});

‎test/contractBridge/executeProposals.js‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@
126126
});
127127

128128
it('should create and execute executeProposal successfully', async () => {
129-
const proposalSignedData = await Helpers.signArrayOfDataWithMpc(proposalsForExecution, destinationDomainID);
129+
const proposalSignedData = await Helpers.signTypedProposal(BridgeInstance.address, proposalsForExecution);
130130

131131
// depositorAddress makes initial deposit of depositAmount
132132
assert.isFalse(await BridgeInstance.paused());
@@ -180,7 +180,7 @@
180180
});
181181

182182
it('should skip executing proposal if deposit nonce is already used', async () => {
183-
const proposalSignedData = await Helpers.signArrayOfDataWithMpc(proposalsForExecution, destinationDomainID);
183+
const proposalSignedData = await Helpers.signTypedProposal(BridgeInstance.address, proposalsForExecution);
184184

185185
// depositorAddress makes initial deposit of depositAmount
186186
assert.isFalse(await BridgeInstance.paused());
@@ -245,7 +245,7 @@
245245
});
246246

247247
it('should fail executing proposals if empty array is passed for execution', async () => {
248-
const proposalSignedData = await Helpers.signArrayOfDataWithMpc(proposalsForExecution, destinationDomainID);
248+
const proposalSignedData = await Helpers.signTypedProposal(BridgeInstance.address, proposalsForExecution);
249249

250250
await TruffleAssert.reverts(BridgeInstance.executeProposals(
251251
[],
@@ -255,7 +255,7 @@
255255
});
256256

257257
it('executeProposal event should be emitted with expected values', async () => {
258-
const proposalSignedData = await Helpers.signArrayOfDataWithMpc(proposalsForExecution, destinationDomainID);
258+
const proposalSignedData = await Helpers.signTypedProposal(BridgeInstance.address, proposalsForExecution);
259259

260260
// depositorAddress makes initial deposit of depositAmount
261261
assert.isFalse(await BridgeInstance.paused());
@@ -322,8 +322,8 @@
322322
assert.strictEqual(recipientERC1155Balance.toNumber(), depositAmount);
323323
});
324324

325-
it('should fail to executeProposals if signed destinationDomainID in not the domain on which proposal should be executed', async () => {
326-
const proposalSignedData = await Helpers.signArrayOfDataWithMpc(proposalsForExecution, invalidDestinationDomainID);
325+
it('should fail to executeProposals if signed Proposal has different chainID than the one on which it should be executed', async () => {
326+
const proposalSignedData = await Helpers.mockSignTypedProposalWithInvalidChainID(BridgeInstance.address, proposalsForExecution);
327327

328328
// depositorAddress makes initial deposit of depositAmount
329329
assert.isFalse(await BridgeInstance.paused());
@@ -355,6 +355,6 @@
355355
proposalsForExecution,
356356
proposalSignedData,
357357
{ from: relayer1Address }
358-
), "Invalid message signer");
358+
), "Invalid proposal signer");
359359
});
360360
});

0 commit comments

Comments
 (0)