Skip to content

Commit

Permalink
✨(Identity) Add nonce to execution signatures
Browse files Browse the repository at this point in the history
  • Loading branch information
@Nakasar
Nakasar committed Oct 23, 2024
1 parent 9eac916 commit 45a16c8
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 33 deletions.
24 changes: 19 additions & 5 deletions contracts/Identity.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Check failure on line 117 in contracts/Identity.sol

View workflow job for this annotation

GitHub Actions / Lint (16.x) 

Line length must be no more than 130 but current length is 139
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;
Expand All @@ -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);
}

Expand Down Expand Up @@ -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)
);
Expand Down Expand Up @@ -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) {

Check failure on line 728 in contracts/Identity.sol

View workflow job for this annotation

GitHub Actions / Lint (16.x) 

Function order is incorrect, public view function can not go after internal view function (line 699)
return _operationNonce;
}

/**
* @notice Computes if the context in which the function is called is a constructor or not.
*
Expand Down
1 change: 1 addition & 0 deletions contracts/interface/IERC734.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ interface IERC734 {
address _to,
uint256 _value,
bytes calldata _data,
uint256 nonce,
uint256 _keyType,
uint8 v,
bytes32 r,
Expand Down
9 changes: 8 additions & 1 deletion contracts/storage/Storage.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
90 changes: 63 additions & 27 deletions test/identities/executions.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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');
});
Expand All @@ -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');

Expand All @@ -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);
Expand Down Expand Up @@ -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');
});
});
Expand Down Expand Up @@ -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);
Expand All @@ -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');

});
})
});
});

Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 45a16c8

Please sign in to comment.