Merge pull request #121 from mysteriumnetwork/code-review-fixes

Fixes after code review

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;
     }
 
 }

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);

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

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)
     })

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)

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