From 45a16c8e46f74ebef4f9535922e19bf881e9009c Mon Sep 17 00:00:00 2001 From: "kevin.thizy" Date: Wed, 23 Oct 2024 11:12:21 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(Identity)=20Add=20nonce=20to=20execut?= =?UTF-8?q?ion=20signatures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- contracts/Identity.sol | 24 ++++++-- contracts/interface/IERC734.sol | 1 + contracts/storage/Storage.sol | 9 ++- test/identities/executions.test.ts | 90 +++++++++++++++++++++--------- 4 files changed, 91 insertions(+), 33 deletions(-) diff --git a/contracts/Identity.sol b/contracts/Identity.sol index a7eed56..ce00795 100644 --- a/contracts/Identity.sol +++ b/contracts/Identity.sol @@ -114,13 +114,13 @@ contract Identity is Storage, IIdentity, Version { * @param _keyType The type of key used for the signature, a uint256 for different key types. 1 = ECDSA, 2 = RSA, 3 = P256. * @return executionId to use in the approve function, to approve or reject this execution. */ - function executeSigned(address _to, uint256 _value, bytes memory _data, uint256 _keyType, uint8 v, bytes32 r, bytes32 s) + function executeSigned(address _to, uint256 _value, bytes memory _data, uint256 nonce, uint256 _keyType, uint8 v, bytes32 r, bytes32 s) external delegatedOnly override payable returns (uint256 executionId) { - bytes32 executionSigner = _recoverSignerForExecution(_to, _value, _data, _keyType, v, r, s); + bytes32 executionSigner = _recoverSignerForExecution(_to, _value, _data, nonce, _keyType, v, r, s); uint256 _executionId = _executionNonce; _executions[_executionId].to = _to; @@ -131,9 +131,14 @@ contract Identity is Storage, IIdentity, Version { emit ExecutionRequested(_executionId, _to, _value, _data); if (keyHasPurpose(executionSigner, 1)) { + require(nonce == _operationNonce, "Invalid nonce"); + _operationNonce++; + _approveAndExecute(_executionId, true); - } - else if (_to != address(this) && keyHasPurpose(executionSigner, 2)){ + } else if (_to != address(this) && keyHasPurpose(executionSigner, 2)){ + require(nonce == _operationNonce, "Invalid nonce"); + _operationNonce++; + _approveAndExecute(_executionId, true); } @@ -670,13 +675,14 @@ contract Identity is Storage, IIdentity, Version { address _to, uint256 _value, bytes memory _data, + uint256 _nonce, uint256 _keyType, uint8 v, bytes32 r, bytes32 s ) internal delegatedOnly view returns(bytes32 keyHash) { if (_keyType == 1) { - bytes32 dataHash = keccak256(abi.encode(address(this), _to, _value, _data)); + bytes32 dataHash = keccak256(abi.encode(address(this), _to, _value, _data, _nonce)); bytes32 prefixedHash = keccak256( abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash) ); @@ -715,6 +721,14 @@ contract Identity is Storage, IIdentity, Version { } } + /** + * @notice Return the operation nonce (for signed operations). + * @return The next sequential nonce. + */ + function getNonce() public view virtual returns (uint256) { + return _operationNonce; + } + /** * @notice Computes if the context in which the function is called is a constructor or not. * diff --git a/contracts/interface/IERC734.sol b/contracts/interface/IERC734.sol index da43c31..a813448 100644 --- a/contracts/interface/IERC734.sol +++ b/contracts/interface/IERC734.sol @@ -119,6 +119,7 @@ interface IERC734 { address _to, uint256 _value, bytes calldata _data, + uint256 nonce, uint256 _keyType, uint8 v, bytes32 r, diff --git a/contracts/storage/Storage.sol b/contracts/storage/Storage.sol index 795682c..3533b49 100644 --- a/contracts/storage/Storage.sol +++ b/contracts/storage/Storage.sol @@ -30,9 +30,16 @@ contract Storage is Structs { // status on potential interactions with the contract bool internal _canInteract = false; + /** + * @dev nonce used to validate signature of operations. + * executeSigned must verify that the nonce is the next sequential nonce before calling approveAndExecute. + * Nonce validation is not required for approve function (as the executionNonce already fulfills this role). + */ + uint256 internal _operationNonce = 0; + /** * @dev This empty reserved space is put in place to allow future versions to add new * variables without shifting down storage in the inheritance chain. */ - uint256[49] private __gap; + uint256[48] private __gap; } diff --git a/test/identities/executions.test.ts b/test/identities/executions.test.ts index 9ebd2fa..d49359d 100644 --- a/test/identities/executions.test.ts +++ b/test/identities/executions.test.ts @@ -278,12 +278,12 @@ describe('Identity', () => { }; const signature = await aliceWallet.signMessage(ethers.getBytes(ethers.keccak256(ethers.AbiCoder.defaultAbiCoder().encode( - ['address', 'address', 'uint256', 'bytes'], - [await aliceIdentity.getAddress(), action.to, action.value, action.data], + ['address', 'address', 'uint256', 'bytes', 'uint256'], + [await aliceIdentity.getAddress(), action.to, action.value, action.data, 0], )))); const signatureParsed = ethers.Signature.from(signature); - const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); + const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 0, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); await expect(tx).to.emit(aliceIdentity, 'Approved'); await expect(tx).to.emit(aliceIdentity, 'Executed'); const newBalance = await ethers.provider.getBalance(carolWallet); @@ -309,12 +309,12 @@ describe('Identity', () => { }; const signature = await aliceWallet.signMessage(ethers.getBytes(ethers.keccak256(ethers.AbiCoder.defaultAbiCoder().encode( - ['address', 'address', 'uint256', 'bytes'], - [await aliceIdentity.getAddress(), action.to, action.value, action.data] + ['address', 'address', 'uint256', 'bytes', 'uint256'], + [await aliceIdentity.getAddress(), action.to, action.value, action.data, 0] )))); const signatureParsed = ethers.Signature.from(signature); - const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); + const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 0, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); await expect(tx).to.emit(aliceIdentity, 'Approved'); await expect(tx).to.emit(aliceIdentity, 'ExecutionFailed'); }); @@ -339,12 +339,12 @@ describe('Identity', () => { }; const signature = await aliceWallet.signMessage(ethers.getBytes(ethers.keccak256(ethers.AbiCoder.defaultAbiCoder().encode( - ['address', 'address', 'uint256', 'bytes'], - [await aliceIdentity.getAddress(), action.to, action.value, action.data] + ['address', 'address', 'uint256', 'bytes', 'uint256'], + [await aliceIdentity.getAddress(), action.to, action.value, action.data, 0] )))); const signatureParsed = ethers.Signature.from(signature); - const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); + const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 0, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); await expect(tx).to.emit(aliceIdentity, 'Approved'); await expect(tx).to.emit(aliceIdentity, 'Executed'); @@ -371,12 +371,12 @@ describe('Identity', () => { }; const signature = await aliceWallet.signMessage(ethers.getBytes(ethers.keccak256(ethers.AbiCoder.defaultAbiCoder().encode( - ['address', 'address', 'uint256', 'bytes'], - [await aliceIdentity.getAddress(), action.to, action.value, action.data] + ['address', 'address', 'uint256', 'bytes', 'uint256'], + [await aliceIdentity.getAddress(), action.to, action.value, action.data, 0] )))); const signatureParsed = ethers.Signature.from(signature); - const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); + const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 0, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); await expect(tx).to.emit(aliceIdentity, 'Approved'); await expect(tx).to.emit(aliceIdentity, 'ExecutionFailed'); const newBalance = await ethers.provider.getBalance(carolWallet); @@ -410,12 +410,12 @@ describe('Identity', () => { }; const signature = await carolWallet.signMessage(ethers.getBytes(ethers.keccak256(ethers.AbiCoder.defaultAbiCoder().encode( - ['address', 'uint256', 'bytes'], - [action.to, action.value, action.data] + ['address', 'uint256', 'bytes', 'uint256'], + [action.to, action.value, action.data, 0] )))); const signatureParsed = ethers.Signature.from(signature); - const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); + const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 0, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); await expect(tx).to.emit(aliceIdentity, 'ExecutionRequested'); }); }); @@ -446,12 +446,12 @@ describe('Identity', () => { const previousBalance = await ethers.provider.getBalance(bobIdentity); const signature = await carolWallet.signMessage(ethers.getBytes(ethers.keccak256(ethers.AbiCoder.defaultAbiCoder().encode( - ['address', 'address', 'uint256', 'bytes'], - [await aliceIdentity.getAddress(), action.to, action.value, action.data] + ['address', 'address', 'uint256', 'bytes', 'uint256'], + [await aliceIdentity.getAddress(), action.to, action.value, action.data, 0] )))); const signatureParsed = ethers.Signature.from(signature); - const tx = await aliceIdentity.connect(davidWallet).executeSigned(action.to, action.value, action.data, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); + const tx = await aliceIdentity.connect(davidWallet).executeSigned(action.to, action.value, action.data, 0, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); await expect(tx).to.emit(aliceIdentity, 'Approved'); await expect(tx).to.emit(aliceIdentity, 'ExecutionFailed'); const newBalance = await ethers.provider.getBalance(bobIdentity); @@ -478,18 +478,54 @@ describe('Identity', () => { }; const signature = await carolWallet.signMessage(ethers.getBytes(ethers.keccak256(ethers.AbiCoder.defaultAbiCoder().encode( - ['address', 'address', 'uint256', 'bytes'], - [await aliceIdentity.getAddress(), action.to, action.value, action.data] + ['address', 'address', 'uint256', 'bytes', 'uint256'], + [await aliceIdentity.getAddress(), action.to, action.value, action.data, 0] )))); const signatureParsed = ethers.Signature.from(signature); - const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); + const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 0, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); await expect(tx).to.emit(aliceIdentity, 'Approved'); await expect(tx).to.emit(aliceIdentity, 'Executed'); const newBalance = await ethers.provider.getBalance(davidWallet); expect(newBalance).to.equal(previousBalance + BigInt(action.value)); }); + + describe('when nonce was already used', () => { + it('should failed for nonce already used', async () => { + const {aliceIdentity, aliceWallet, carolWallet, davidWallet, bobIdentity} = await loadFixture(deployIdentityFixture); + + const carolKeyHash = ethers.keccak256( + ethers.AbiCoder.defaultAbiCoder().encode(['address'], [await carolWallet.getAddress()]) + ); + await aliceIdentity.connect(aliceWallet).addKey(carolKeyHash, 2, 1); + + const aliceKeyHash = ethers.keccak256( + ethers.AbiCoder.defaultAbiCoder().encode(['address'], [await aliceWallet.getAddress()]) + ); + + const action = { + to: await bobIdentity.getAddress(), + value: 10, + data: new ethers.Interface(['function addKey(bytes32 key, uint256 purpose, uint256 keyType) returns (bool success)']).encodeFunctionData('addKey', [ + aliceKeyHash, + 3, + 1, + ]), + }; + + const signature = await carolWallet.signMessage(ethers.getBytes(ethers.keccak256(ethers.AbiCoder.defaultAbiCoder().encode( + ['address', 'address', 'uint256', 'bytes', 'uint256'], + [await aliceIdentity.getAddress(), action.to, action.value, action.data, 0] + )))); + const signatureParsed = ethers.Signature.from(signature); + + await aliceIdentity.connect(davidWallet).executeSigned(action.to, action.value, action.data, 0, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); + + await expect(aliceIdentity.connect(davidWallet).executeSigned(action.to, action.value, action.data, 0, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s)).to.be.revertedWith('Invalid nonce'); + + }); + }) }); }); @@ -505,12 +541,12 @@ describe('Identity', () => { }; const signature = await bobWallet.signMessage(ethers.getBytes(ethers.keccak256(ethers.AbiCoder.defaultAbiCoder().encode( - ['address', 'uint256', 'bytes'], - [action.to, action.value, action.data] + ['address', 'uint256', 'bytes', 'uint256'], + [action.to, action.value, action.data, 0] )))); const signatureParsed = ethers.Signature.from(signature); - const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); + const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 0, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); await expect(tx).to.emit(aliceIdentity, 'ExecutionRequested'); const newBalance = await ethers.provider.getBalance(carolWallet); @@ -558,12 +594,12 @@ describe('Identity', () => { data: '0x', }; const signature = await aliceWallet.signMessage(ethers.getBytes(ethers.keccak256(ethers.AbiCoder.defaultAbiCoder().encode( - ['address', 'address', 'uint256', 'bytes'], - [await aliceIdentity.getAddress(), action.to, action.value, action.data], + ['address', 'address', 'uint256', 'bytes', 'uint256'], + [await aliceIdentity.getAddress(), action.to, action.value, action.data, 0], )))); const signatureParsed = ethers.Signature.from(signature); - const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); + const tx = await aliceIdentity.connect(bobWallet).executeSigned(action.to, action.value, action.data, 0, 1, signatureParsed.v, signatureParsed.r, signatureParsed.s); await expect(tx).to.emit(aliceIdentity, 'Approved'); await expect(tx).to.emit(aliceIdentity, 'Executed'); const newBalance = await ethers.provider.getBalance(carolWallet);