diff --git a/.gitignore b/.gitignore index 2ff0a6950..9b491c2bd 100644 --- a/.gitignore +++ b/.gitignore @@ -142,4 +142,4 @@ benchmark.md /userdocs # test temporary folder -/.test \ No newline at end of file +/.test diff --git a/contracts/LSP25ExecuteRelayCall/ILSP25ExecuteRelayCall.sol b/contracts/LSP25ExecuteRelayCall/ILSP25ExecuteRelayCall.sol index b75e053c7..58b13cd68 100644 --- a/contracts/LSP25ExecuteRelayCall/ILSP25ExecuteRelayCall.sol +++ b/contracts/LSP25ExecuteRelayCall/ILSP25ExecuteRelayCall.sol @@ -5,7 +5,18 @@ interface ILSP25ExecuteRelayCall { /** * @notice Reading the latest nonce of address `from` in the channel ID `channelId`. * - * @dev Get the nonce for a specific controller `from` address that can be used for signing relay transactions via {executeRelayCall}. + * @dev Get the nonce for a specific `from` address that can be used for signing relay transactions via {executeRelayCall}. + * + * @custom:hint A signer can choose its channel number arbitrarily. The recommended practice is to: + * - use `channelId == 0` for transactions for which the ordering of execution matters.abi + * + * _Example: you have two transactions A and B, and transaction A must be executed first and complete successfully before + * transaction B should be executed)._ + * + * - use any other `channelId` number for transactions that you want to be order independant (out-of-order execution, execution _"in parallel"_). + * + * _Example: you have two transactions A and B. You want transaction B to be executed a) without having to wait for transaction A to complete, + * or b) regardless if transaction A completed successfully or not. * * @param from The address of the signer of the transaction. * @param channelId The channel id that the signer wants to use for executing the transaction. diff --git a/contracts/LSP25ExecuteRelayCall/LSP25Errors.sol b/contracts/LSP25ExecuteRelayCall/LSP25Errors.sol index 01f249cbc..bac4b0946 100644 --- a/contracts/LSP25ExecuteRelayCall/LSP25Errors.sol +++ b/contracts/LSP25ExecuteRelayCall/LSP25Errors.sol @@ -2,16 +2,10 @@ pragma solidity ^0.8.4; /** - * @dev Reverts when the `signer` address retrieved from the `signature` has an invalid nonce: `invalidNonce`. + * @notice Relay call not valid yet. * - * @param signer The address of the signer - * @param invalidNonce The nonce retrieved for the `signer` address - * @param signature The signature used to retrieve the `signer` address - */ -error InvalidRelayNonce(address signer, uint256 invalidNonce, bytes signature); - -/** - * @dev Reverts when the start timestamp provided to {executeRelayCall} function is bigger than the current timestamp. + * @dev Reverts when the relay call is cannot yet bet executed. + * This mean that the starting timestamp provided to {executeRelayCall} function is bigger than the current timestamp. */ error RelayCallBeforeStartTime(); diff --git a/contracts/LSP25ExecuteRelayCall/LSP25MultiChannelNonce.sol b/contracts/LSP25ExecuteRelayCall/LSP25MultiChannelNonce.sol index be23f67f2..68cb74d3f 100644 --- a/contracts/LSP25ExecuteRelayCall/LSP25MultiChannelNonce.sol +++ b/contracts/LSP25ExecuteRelayCall/LSP25MultiChannelNonce.sol @@ -8,11 +8,7 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {LSP25_VERSION} from "./LSP25Constants.sol"; // errors -import { - InvalidRelayNonce, - RelayCallBeforeStartTime, - RelayCallExpired -} from "./LSP25Errors.sol"; +import {RelayCallBeforeStartTime, RelayCallExpired} from "./LSP25Errors.sol"; /** * @title Implementation of the multi channel nonce and the signature verification defined in the LSP25 standard. @@ -31,29 +27,61 @@ abstract contract LSP25MultiChannelNonce { mapping(address => mapping(uint256 => uint256)) internal _nonceStore; /** - * @dev Validate that the `nonce` given for the `signature` signed and the `payload` to execute is valid - * and conform to the signature format according to the LSP25 standard. + * @dev Read the nonce for a `from` address on a specific `channelId`. + * This will return an `idx`, which is the concatenation of two `uint128` as follow: + * 1. the `channelId` where the nonce was queried for. + * 2. the actual nonce of the given `channelId`. * - * @param signature A valid signature for a signer, generated according to the signature format specified in the LSP25 standard. - * @param nonce The nonce that the signer used to generate the `signature`. - * @param validityTimestamps Two `uint128` concatenated together, where the left-most `uint128` represent the timestamp from which the transaction can be executed, - * and the right-most `uint128` represents the timestamp after which the transaction expire. - * @param callData The abi-encoded function call to execute. + * _Example:_ + * + * Assuming that the latest nonce on `channelId` number `5` is `1`, the `idx` returned by this function will be: + * + * ``` + * // in decimals = 1701411834604692317316873037158841057281 + * idx = 0x0000000000000000000000000000000500000000000000000000000000000001 + * ``` + * + * This idx can be described as follow: + * + * ``` + * channelId => 5 nonce in this channel => 1 + * v------------------------------v-------------------------------v + * 0x0000000000000000000000000000000500000000000000000000000000000001 + * ``` + * + * @param from The address to read the nonce for. + * @param channelId The channel in which to extract the nonce. + * + * @return idx The idx composed of two `uint128`: the channelId + nonce in channel concatenated together in a single `uint256` value. + */ + function _getNonce( + address from, + uint128 channelId + ) internal view virtual returns (uint256 idx) { + uint256 nonceInChannel = _nonceStore[from][channelId]; + return (uint256(channelId) << 128) | nonceInChannel; + } + + /** + * @dev Recover the address of the signer that generated a `signature` using the parameters provided `nonce`, `validityTimestamps`, `msgValue` and `callData`. + * The address of the signer will be recovered using the LSP25 signature format. * - * @return recoveredSignerAddress The address of the signer recovered, for which the signature was validated. + * @param signature A 65 bytes long signature generated according to the signature format specified in the LSP25 standard. + * @param nonce The nonce that the signer used to generate the `signature`. + * @param validityTimestamps The validity timestamp that the signer used to generate the signature (See {_verifyValidityTimestamps} to learn more). + * @param msgValue The amount of native tokens intended to be sent for the relay transaction. + * @param callData The calldata to execute as a relay transaction that the signer signed for. * - * @custom:warning Be aware that this function can also throw an error if the `callData` was signed incorrectly (not conforming to the signature format defined in the LSP25 standard). - * The contract cannot distinguish if the data is signed correctly or not. Instead, it will recover an incorrect signer address from the signature - * and throw an {InvalidRelayNonce} error with the incorrect signer address as the first parameter. + * @return The address that signed, recovered from the `signature`. */ - function _validateExecuteRelayCall( + function _recoverSignerFromLSP25Signature( bytes memory signature, uint256 nonce, uint256 validityTimestamps, uint256 msgValue, bytes calldata callData - ) internal returns (address recoveredSignerAddress) { - bytes memory encodedMessage = abi.encodePacked( + ) internal view returns (address) { + bytes memory lsp25EncodedMessage = abi.encodePacked( LSP25_VERSION, block.chainid, nonce, @@ -62,31 +90,36 @@ abstract contract LSP25MultiChannelNonce { callData ); - recoveredSignerAddress = address(this) - .toDataWithIntendedValidatorHash(encodedMessage) - .recover(signature); + bytes32 eip191Hash = address(this).toDataWithIntendedValidatorHash( + lsp25EncodedMessage + ); - if (!_isValidNonce(recoveredSignerAddress, nonce)) { - revert InvalidRelayNonce(recoveredSignerAddress, nonce, signature); - } + return eip191Hash.recover(signature); + } - // increase nonce after successful verification - _nonceStore[recoveredSignerAddress][nonce >> 128]++; + /** + * @notice Verifying if the current timestamp is within the date and time range provided by `validityTimestamps`. + * + * @dev Verify that the validity timestamp provided is within a valid range compared to the current timestamp. + * + * @param validityTimestamps Two `uint128` concatenated together, where the left-most `uint128` represent the timestamp from which the transaction can be executed, + * and the right-most `uint128` represents the timestamp after which the transaction expire. + */ + function _verifyValidityTimestamps( + uint256 validityTimestamps + ) internal view { + if (validityTimestamps == 0) return; - if (validityTimestamps != 0) { - uint128 startingTimestamp = uint128(validityTimestamps >> 128); - uint128 endingTimestamp = uint128(validityTimestamps); + uint128 startingTimestamp = uint128(validityTimestamps >> 128); + uint128 endingTimestamp = uint128(validityTimestamps); - // solhint-disable not-rely-on-time - if (block.timestamp < startingTimestamp) { - revert RelayCallBeforeStartTime(); - } - if (block.timestamp > endingTimestamp) { - revert RelayCallExpired(); - } + // solhint-disable not-rely-on-time + if (block.timestamp < startingTimestamp) { + revert RelayCallBeforeStartTime(); + } + if (block.timestamp > endingTimestamp) { + revert RelayCallExpired(); } - - return recoveredSignerAddress; } /** @@ -109,6 +142,6 @@ abstract contract LSP25MultiChannelNonce { // Alternatively: // uint256 mask = (1<<128)-1; // uint256 mask = 0xffffffffffffffffffffffffffffffff; - return (idx & mask) == (_nonceStore[from][idx >> 128]); + return (idx & mask) == _nonceStore[from][idx >> 128]; } } diff --git a/contracts/LSP6KeyManager/LSP6Errors.sol b/contracts/LSP6KeyManager/LSP6Errors.sol index 4f1e057b8..9b1c4b8b7 100644 --- a/contracts/LSP6KeyManager/LSP6Errors.sol +++ b/contracts/LSP6KeyManager/LSP6Errors.sol @@ -59,9 +59,9 @@ error InvalidLSP6Target(); * * @dev Reverts when the `signer` address retrieved from the `signature` has an invalid nonce: `invalidNonce`. * - * @param signer The address of the signer - * @param invalidNonce The nonce retrieved for the `signer` address - * @param signature The signature used to retrieve the `signer` address + * @param signer The address of the signer. + * @param invalidNonce The nonce retrieved for the `signer` address. + * @param signature The signature used to retrieve the `signer` address. */ error InvalidRelayNonce(address signer, uint256 invalidNonce, bytes signature); @@ -227,20 +227,6 @@ error CannotSendValueToSetData(); */ error CallingKeyManagerNotAllowed(); -/** - * @notice Relay call not valid yet. - * - * @dev Reverts when the start timestamp provided to {executeRelayCall} function is bigger than the current timestamp. - */ -error RelayCallBeforeStartTime(); - -/** - * @notice The date of the relay call expired. - * - * @dev Reverts when the period to execute the relay call has expired. - */ -error RelayCallExpired(); - /** * @dev reverts when the address of the Key Manager is being set as extensions for lsp20 functions */ diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol index 6f94ae9b6..06599d6a8 100644 --- a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol +++ b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol @@ -43,7 +43,8 @@ import { InvalidPayload, NoPermissionsSet, InvalidERC725Function, - CannotSendValueToSetData + CannotSendValueToSetData, + InvalidRelayNonce } from "./LSP6Errors.sol"; import { @@ -112,16 +113,12 @@ abstract contract LSP6KeyManagerCore is /** * @inheritdoc ILSP25 - * - * @custom:info A signer can choose its channel number arbitrarily. Channel ID = 0 can be used for sequential nonces (transactions - * that are order dependant), any other channel ID for out-of-order execution (= execution in parallel). */ function getNonce( address from, uint128 channelId ) public view virtual returns (uint256) { - uint256 nonceInChannel = _nonceStore[from][channelId]; - return (uint256(channelId) << 128) | nonceInChannel; + return LSP25MultiChannelNonce._getNonce(from, channelId); } /** @@ -386,6 +383,20 @@ abstract contract LSP6KeyManagerCore is return result; } + /** + * @dev Validate that the `nonce` given for the `signature` signed and the `payload` to execute is valid + * and conform to the signature format according to the LSP25 standard. + * + * @param signature A valid signature for a signer, generated according to the signature format specified in the LSP25 standard. + * @param nonce The nonce that the signer used to generate the `signature`. + * @param validityTimestamps Two `uint128` concatenated together, where the left-most `uint128` represent the timestamp from which the transaction can be executed, + * and the right-most `uint128` represents the timestamp after which the transaction expire. + * @param payload The abi-encoded function call to execute. + * + * @custom:warning Be aware that this function can also throw an error if the `callData` was signed incorrectly (not conforming to the signature format defined in the LSP25 standard). + * This is because the contract cannot distinguish if the data is signed correctly or not. Instead, it will recover an incorrect signer address from the signature + * and throw an {InvalidRelayNonce} error with the incorrect signer address as the first parameter. + */ function _executeRelayCall( bytes memory signature, uint256 nonce, @@ -397,13 +408,23 @@ abstract contract LSP6KeyManagerCore is revert InvalidPayload(payload); } - address signer = LSP25MultiChannelNonce._validateExecuteRelayCall( - signature, - nonce, - validityTimestamps, - msgValue, - payload - ); + address signer = LSP25MultiChannelNonce + ._recoverSignerFromLSP25Signature( + signature, + nonce, + validityTimestamps, + msgValue, + payload + ); + + if (!_isValidNonce(signer, nonce)) { + revert InvalidRelayNonce(signer, nonce, signature); + } + + // increase nonce after successful verification + _nonceStore[signer][nonce >> 128]++; + + LSP25MultiChannelNonce._verifyValidityTimestamps(validityTimestamps); bool isSetData = false; if ( diff --git a/contracts/Mocks/LSP25MultiChannelNonceTester.sol b/contracts/Mocks/LSP25MultiChannelNonceTester.sol new file mode 100644 index 000000000..c825ea946 --- /dev/null +++ b/contracts/Mocks/LSP25MultiChannelNonceTester.sol @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.4; + +import { + LSP25MultiChannelNonce +} from "../LSP25ExecuteRelayCall/LSP25MultiChannelNonce.sol"; + +/** + * @dev This contract is used only for testing the internal functions. + */ +contract LSP25MultiChannelNonceTester is LSP25MultiChannelNonce { + function getNonce( + address from, + uint128 channelId + ) public view returns (uint256 idx) { + return _getNonce(from, channelId); + } + + function recoverSignerFromLSP25Signature( + bytes memory signature, + uint256 nonce, + uint256 validityTimestamps, + uint256 msgValue, + bytes calldata callData + ) public view returns (address) { + return + _recoverSignerFromLSP25Signature( + signature, + nonce, + validityTimestamps, + msgValue, + callData + ); + } + + function verifyValidityTimestamps(uint256 validityTimestamps) public view { + return _verifyValidityTimestamps(validityTimestamps); + } + + function isValidNonce( + address from, + uint256 nonce + ) public view returns (bool) { + return _isValidNonce(from, nonce); + } +} diff --git a/package.json b/package.json index cf0245c3d..bcee054f5 100644 --- a/package.json +++ b/package.json @@ -66,6 +66,7 @@ "test:lsp20": "hardhat test --no-compile tests/LSP20CallVerification/LSP6/LSP20WithLSP6.test.ts", "test:lsp20init": "hardhat test --no-compile tests/LSP20CallVerification/LSP6/LSP20WithLSP6Init.test.ts", "test:lsp23": "hardhat test --no-compile tests/LSP23LinkedContractsDeployment/LSP23LinkedContractsDeployment.test.ts", + "test:lsp25": "hardhat test --no-compile tests/LSP25ExecuteRelayCall/LSP25MultiChannelNonce.test.ts", "test:universalfactory": "hardhat test --no-compile tests/LSP16UniversalFactory/LSP16UniversalFactory.test.ts", "test:reentrancy": "hardhat test --no-compile tests/Reentrancy/Reentrancy.test.ts", "test:reentrancyinit": "hardhat test --no-compile tests/Reentrancy/ReentrancyInit.test.ts", diff --git a/tests/LSP25ExecuteRelayCall/LSP25MultiChannelNonce.test.ts b/tests/LSP25ExecuteRelayCall/LSP25MultiChannelNonce.test.ts new file mode 100644 index 000000000..1d3b9c6cd --- /dev/null +++ b/tests/LSP25ExecuteRelayCall/LSP25MultiChannelNonce.test.ts @@ -0,0 +1,154 @@ +import { ethers } from 'hardhat'; +import { expect } from 'chai'; +import { LSP25_VERSION } from '../../constants'; +import { LOCAL_PRIVATE_KEYS } from '../utils/helpers'; +import { EIP191Signer } from '@lukso/eip191-signer.js'; + +import { LSP25MultiChannelNonceTester, LSP25MultiChannelNonceTester__factory } from '../../types'; + +describe('LSP25MultiChannelNonce', () => { + let contract: LSP25MultiChannelNonceTester; + let account; + + const HARDHAT_CHAINID = 31337; + + const eip191Signer = new EIP191Signer(); + const signerPrivateKey = LOCAL_PRIVATE_KEYS.ACCOUNT0; + + before(async () => { + account = (await ethers.getSigners())[0]; + + contract = await new LSP25MultiChannelNonceTester__factory(account).deploy(); + }); + + describe('testing `_isValidNonce`', () => { + it('should return `true` when providing a valid nonce', async () => { + const nonce = await contract.getNonce(account.address, 0); + const result = await contract.isValidNonce(account.address, nonce); + expect(result).to.be.true; + }); + + it('should return `false` if the wrong nonce provided', async () => { + const nonce = await contract.getNonce(account.address, 0); + const invalidNonce = 35; + expect(nonce).to.not.equal(invalidNonce); // sanity check + + const result = await contract.isValidNonce(account.address, invalidNonce); + expect(result).to.be.false; + }); + }); + + describe('testing `_recoverSignerFromLSP25Signature`', () => { + it('should pass and recover the right address if the data was signed with LSP25 signature format', async () => { + const channelId = 0; + + const parameters = { + nonce: await contract.getNonce(account.address, channelId), + validityTimestamps: 0, + valueToSend: 0, + payload: '0xcafecafe', + }; + + const encodedMessage = ethers.utils.solidityPack( + ['uint256', 'uint256', 'uint256', 'uint256', 'uint256', 'bytes'], + [ + LSP25_VERSION, + HARDHAT_CHAINID, + parameters.nonce, + parameters.validityTimestamps, + parameters.valueToSend, + parameters.payload, + ], + ); + + const { signature } = await eip191Signer.signDataWithIntendedValidator( + contract.address, + encodedMessage, + signerPrivateKey, + ); + + const recoveredAddress = await contract.recoverSignerFromLSP25Signature( + signature, + parameters.nonce, + parameters.validityTimestamps, + parameters.valueToSend, + parameters.payload, + ); + expect(recoveredAddress).to.equal(account.address); + }); + + it('should return the wrong address if the data was signed with version different than 25', async () => { + const channelId = 0; + + const parameters = { + nonce: await contract.getNonce(account.address, channelId), + validityTimestamps: 0, + valueToSend: 0, + payload: '0xcafecafe', + }; + + const encodedMessage = ethers.utils.solidityPack( + ['uint256', 'uint256', 'uint256', 'uint256', 'uint256', 'bytes'], + [ + 12345, // incorrect version number + HARDHAT_CHAINID, + parameters.nonce, + parameters.validityTimestamps, + parameters.valueToSend, + parameters.payload, + ], + ); + + const { signature } = await eip191Signer.signDataWithIntendedValidator( + contract.address, + encodedMessage, + signerPrivateKey, + ); + + const recoveredAddress = await contract.recoverSignerFromLSP25Signature( + signature, + parameters.nonce, + parameters.validityTimestamps, + parameters.valueToSend, + parameters.payload, + ); + expect(recoveredAddress).to.not.equal(account.address); + }); + + it('should return the wrong address if the data was signed with an invalid nonce', async () => { + const parameters = { + nonce: 12345, + validityTimestamps: 0, + valueToSend: 0, + payload: '0xcafecafe', + }; + + const encodedMessage = ethers.utils.solidityPack( + ['uint256', 'uint256', 'uint256', 'uint256', 'uint256', 'bytes'], + [ + 12345, // incorrect version number + HARDHAT_CHAINID, + parameters.nonce, + parameters.validityTimestamps, + parameters.valueToSend, + parameters.payload, + ], + ); + + const { signature } = await eip191Signer.signDataWithIntendedValidator( + contract.address, + encodedMessage, + signerPrivateKey, + ); + + const recoveredAddress = await contract.recoverSignerFromLSP25Signature( + signature, + parameters.nonce, + parameters.validityTimestamps, + parameters.valueToSend, + parameters.payload, + ); + expect(recoveredAddress).to.not.equal(account.address); + }); + }); +}); diff --git a/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts b/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts index 896c8468b..d1c8a0207 100644 --- a/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts +++ b/tests/LSP6KeyManager/Relay/ExecuteRelayCall.test.ts @@ -554,8 +554,8 @@ export const shouldBehaveLikeExecuteRelayCall = ( }); describe('When testing `validityTimestamps`', () => { - describe('(invalid timestamps) `startingTimestamp` is greter than `endingTimestamp`', () => { - describe('`now` is equal to `startingTimestamp` and `now` is greter than `endingTimestamp`', () => { + describe('(invalid timestamps) `startingTimestamp` is greater than `endingTimestamp`', () => { + describe('`now` is equal to `startingTimestamp` and `now` is greater than `endingTimestamp`', () => { it('reverts', async () => { const now = await time.latest(); const startingTimestamp = now; diff --git a/tests/LSP6KeyManager/Relay/MultiChannelNonce.test.ts b/tests/LSP6KeyManager/Relay/MultiChannelNonce.test.ts index 228cc35bf..72f2cb0fa 100644 --- a/tests/LSP6KeyManager/Relay/MultiChannelNonce.test.ts +++ b/tests/LSP6KeyManager/Relay/MultiChannelNonce.test.ts @@ -58,7 +58,7 @@ export const shouldBehaveLikeMultiChannelNonce = (buildContext: () => Promise { const channelId = ethers.BigNumber.from(2).pow(129); - await expect(context.keyManager.getNonce(signer.address, channelId)).to.be.revertedWithPanic; + await expect(context.keyManager.getNonce(signer.address, channelId)).to.be.reverted; }); });