Merge pull request #67 from rocket-pool/contract-optimisations

Backup address fixes

Author: darcius

contracts/RocketUser.sol

@@ -174,7 +174,7 @@ contract RocketUser is RocketBase {
         // Get an instance of that pool contract
         rocketPoolMini = RocketPoolMini(_miniPoolAddress);       
         // Got the users address, now check to see if this is a user withdrawing to their backup address, if so, we need to update the users minipool account
-        if (rocketPoolMini.getUserBackupAddressExists(_userAddress)) {
+        if (!rocketPoolMini.getUserExists(_userAddress) && rocketPoolMini.getUserBackupAddressExists(_userAddress)) {
             // Get the original deposit address now
             // This will update the users account to match the backup address, but only after many checks and balances
             // It will fail if the user can't use their backup address to withdraw at this point or its not their nominated backup address trying
@@ -270,10 +270,17 @@ contract RocketUser is RocketBase {
     /// @param _miniPoolAddress The address of the mini pool they the supplied user account is in.
     /// @param _newUserAddressUsedForDeposit The address the user wishes to make their backup withdrawal address
     function userSetWithdrawalDepositAddress(address _newUserAddressUsedForDeposit, address _miniPoolAddress) public returns(bool) {
+        // Check backup withdrawal address is valid
+        require(_newUserAddressUsedForDeposit != 0);
         // Get an instance of that pool contract
         rocketPoolMini = RocketPoolMini(_miniPoolAddress);       
+        // Check user exists in minipool
+        require(rocketPoolMini.getUserExists(msg.sender));
+        // Check backup withdrawal address is not already in use
+        require(!rocketPoolMini.getUserExists(_newUserAddressUsedForDeposit));
+        require(!rocketPoolMini.getUserBackupAddressExists(_newUserAddressUsedForDeposit));
         // User can only set this backup address before deployment to casper, also partners cannot set this address to their own to prevent them accessing the users funds after the set withdrawal backup period expires
-        if ((rocketPoolMini.getStatus() == 0 || rocketPoolMini.getStatus() == 1) && _newUserAddressUsedForDeposit != 0 && rocketPoolMini.getUserPartner(msg.sender) != _newUserAddressUsedForDeposit) {
+        if ((rocketPoolMini.getStatus() == 0 || rocketPoolMini.getStatus() == 1) && rocketPoolMini.getUserPartner(msg.sender) != _newUserAddressUsedForDeposit) {
             if (rocketPoolMini.setUserAddressBackupWithdrawal(msg.sender, _newUserAddressUsedForDeposit)) {
                 // Fire the event
                 emit UserSetBackupWithdrawalAddress(msg.sender, _newUserAddressUsedForDeposit, _miniPoolAddress, now);

test/rocket-user/rocket-user-tests.js

@@ -271,20 +271,49 @@ export default function({owner}) {
 
             // User cannot set a backup withdrawal address to an invalid address
             it(printTitle('user', 'cannot set a backup withdrawal address to an invalid address'), async () => {
-
-                // Register withdrawal address
-                let result = await scenarioRegisterWithdrawalAddress({
+                await assertThrows(scenarioRegisterWithdrawalAddress({
                     withdrawalAddress: '0x0000000000000000000000000000000000000000',
                     miniPool: miniPools.first,
                     fromAddress: userFirst,
                     gas: 550000,
                     checkLogs: false,
-                });
+                }), 'Backup withdrawal address was set');
+            });
+
+
+            // User cannot set a backup withdrawal address to their deposit address
+            it(printTitle('user', 'cannot set a backup withdrawal address to their deposit address'), async () => {
+                await assertThrows(scenarioRegisterWithdrawalAddress({
+                    withdrawalAddress: userFirst,
+                    miniPool: miniPools.first,
+                    fromAddress: userFirst,
+                    gas: 550000,
+                    checkLogs: false,
+                }), 'Backup withdrawal address was set');
+            });
 
-                // Assert UserSetBackupWithdrawalAddress event was not logged
-                let log = result.logs.find(({ event }) => event == 'UserSetBackupWithdrawalAddress');
-                assert.equal(log, undefined, 'UserSetBackupWithdrawalAddress event was logged');
 
+            // User cannot set a backup withdrawal address to an existing deposit address
+            it(printTitle('user', 'cannot set a backup withdrawal address to an existing deposit address'), async () => {
+                await assertThrows(scenarioRegisterWithdrawalAddress({
+                    withdrawalAddress: userSecond,
+                    miniPool: miniPools.first,
+                    fromAddress: userFirst,
+                    gas: 550000,
+                    checkLogs: false,
+                }), 'Backup withdrawal address was set');
+            });
+
+
+            // Random account cannot set a backup withdrawal address
+            it(printTitle('random account', 'cannot set a backup withdrawal address'), async () => {
+                await assertThrows(scenarioRegisterWithdrawalAddress({
+                    withdrawalAddress: userFirstBackupAddress,
+                    miniPool: miniPools.first,
+                    fromAddress: accounts[9],
+                    gas: 550000,
+                    checkLogs: false,
+                }), 'Backup withdrawal address was set');
             });