From 46f4f94ecffb12237f93ee1bb0d7353bc254df96 Mon Sep 17 00:00:00 2001 From: Jaro Date: Mon, 28 Sep 2020 18:33:33 +0300 Subject: [PATCH 1/3] We don't need ownership denouncing --- contracts/Ownable.sol | 5 ----- 1 file changed, 5 deletions(-) diff --git a/contracts/Ownable.sol b/contracts/Ownable.sol index f3101bd..2a58f07 100644 --- a/contracts/Ownable.sol +++ b/contracts/Ownable.sol @@ -15,11 +15,6 @@ contract Ownable { _; } - function renounceOwnership() public virtual onlyOwner { - emit OwnershipTransferred(_owner, address(0)); - _owner = address(0); - } - function transferOwnership(address newOwner) public virtual onlyOwner { require(newOwner != address(0), "Ownable: new owner is the zero address"); emit OwnershipTransferred(_owner, newOwner); From 0882e2ef4d032f4424bc0ff3c1780d1970408170 Mon Sep 17 00:00:00 2001 From: Jaro Date: Mon, 28 Sep 2020 22:20:26 +0300 Subject: [PATCH 2/3] Reply protection for update hermes url --- contracts/Registry.sol | 3 ++- test/registry.js | 3 ++- test/utils/client.js | 5 +++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/contracts/Registry.sol b/contracts/Registry.sol index d844abe..306ea50 100644 --- a/contracts/Registry.sol +++ b/contracts/Registry.sol @@ -15,6 +15,7 @@ contract Registry is FundsRecovery { using ECDSA for bytes32; using SafeMath for uint256; + uint256 internal lastNonce; address payable public dex; // Any uniswap v2 compatible DEX router address uint256 public minimalHermesStake; @@ -145,7 +146,7 @@ contract Registry is FundsRecovery { require(isActiveHermes(_hermesId), "Registry: provided hermes has to be active"); // Check if given signature is valid - address _operator = keccak256(abi.encodePacked(address(this), _hermesId, _url)).recover(_signature); + address _operator = keccak256(abi.encodePacked(address(this), _hermesId, _url, lastNonce++)).recover(_signature); require(_operator == hermeses[_hermesId].operator, "wrong signature"); // Update URL diff --git a/test/registry.js b/test/registry.js index fffd9c2..bedecfe 100644 --- a/test/registry.js +++ b/test/registry.js @@ -56,7 +56,8 @@ contract('Registry', ([txMaker, minter, fundsDestination, ...otherAccounts]) => it('should be possible to change hermes URL', async () => { const newURL = 'https://test.hermes/api/v2' - const signature = signUrlUpdate(registry.address, hermesId, newURL, operator) + const nonce = new BN(0) + const signature = signUrlUpdate(registry.address, hermesId, newURL, nonce, operator) await registry.updateHermesURL(hermesId, Buffer.from(newURL), signature) expect(Buffer.from((await registry.getHermesURL(hermesId)).slice(2), 'hex').toString()).to.be.equal(newURL) diff --git a/test/utils/client.js b/test/utils/client.js index 096dcbf..29e2d06 100644 --- a/test/utils/client.js +++ b/test/utils/client.js @@ -352,11 +352,12 @@ function signIdentityRegistration(registryAddress, hermesId, stake, fee, benefic return signature } -function signUrlUpdate(registryAddress, hermesId, url, identity) { +function signUrlUpdate(registryAddress, hermesId, url, nonce, identity) { const message = Buffer.concat([ Buffer.from(registryAddress.slice(2), 'hex'), Buffer.from(hermesId.slice(2), 'hex'), - Buffer.from(url) + Buffer.from(url), + toBytes32Buffer(nonce) ]) // sign and verify the signature From 836f12dd7d131149a91de418b976c6b3bbe22087 Mon Sep 17 00:00:00 2001 From: Jaro Date: Mon, 28 Sep 2020 22:40:56 +0300 Subject: [PATCH 3/3] Prevent signed message replay on other channels of the same operator --- contracts/ChannelImplementation.sol | 9 ++++----- test/fundsRecoveryByCheque.js | 13 +++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/contracts/ChannelImplementation.sol b/contracts/ChannelImplementation.sol index 53cb6a0..f4c5dc9 100644 --- a/contracts/ChannelImplementation.sol +++ b/contracts/ChannelImplementation.sol @@ -185,17 +185,16 @@ contract ChannelImplementation is FundsRecovery { // Setting new destination of funds recovery. string constant FUNDS_DESTINATION_PREFIX = "Set funds destination:"; - function setFundsDestinationByCheque(address payable _newDestination, uint256 _nonce, bytes memory _signature) public { + function setFundsDestinationByCheque(address payable _newDestination, bytes memory _signature) public { require(_newDestination != address(0)); - require(_nonce > lastNonce, "nonce have to be bigger than last one"); - address _signer = keccak256(abi.encodePacked(FUNDS_DESTINATION_PREFIX, _newDestination, _nonce)).recover(_signature); - require(_signer == operator, "have to be signed by proper identity"); + address _channelId = address(this); + address _signer = keccak256(abi.encodePacked(FUNDS_DESTINATION_PREFIX, _channelId, _newDestination, lastNonce++)).recover(_signature); + require(_signer == operator, "Channel: have to be signed by proper identity"); emit DestinationChanged(fundsDestination, _newDestination); fundsDestination = _newDestination; - lastNonce = _nonce; } } diff --git a/test/fundsRecoveryByCheque.js b/test/fundsRecoveryByCheque.js index 8e99426..520eb99 100644 --- a/test/fundsRecoveryByCheque.js +++ b/test/fundsRecoveryByCheque.js @@ -21,10 +21,11 @@ const Zero = new BN(0) const ZeroAddress = '0x0000000000000000000000000000000000000000' const hermesURL = Buffer.from('http://test.hermes') -function createCheque(signer, destination, nonce) { +function createCheque(signer, channelId, destination, nonce) { const PREFIX = Buffer.from("Set funds destination:") const message = Buffer.concat([ PREFIX, + Buffer.from(channelId.slice(2), 'hex'), Buffer.from(destination.slice(2), 'hex'), toBytes32Buffer(nonce) ]) @@ -88,16 +89,16 @@ contract('Full path (in channel using cheque) test for funds recovery', ([txMake }) it('should set funds destination using checque', async () => { - const nonce = new BN(1) - const signature = createCheque(identity, fundsDestination, nonce) - await channel.setFundsDestinationByCheque(fundsDestination, nonce, signature).should.be.fulfilled + const nonce = new BN(0) + const signature = createCheque(identity, channel.address, fundsDestination, nonce) + await channel.setFundsDestinationByCheque(fundsDestination, signature).should.be.fulfilled expect(await channel.getFundsDestination()).to.be.equal(fundsDestination) }) it('should fail setting funds destination using wrong identity', async () => { const secondIdentity = wallet.generateAccount() - const nonce = new BN(2) - const signature = createCheque(secondIdentity, otherAccounts[1], nonce) + const nonce = new BN(1) + const signature = createCheque(secondIdentity, channel.address, otherAccounts[1], nonce) await channel.setFundsDestinationByCheque(fundsDestination, nonce, signature).should.be.rejected expect(await channel.getFundsDestination()).to.be.equal(fundsDestination) })