Sip10 delegate approvals audit remediations (#477)

Author: jacko125

contracts/DelegateApprovals.sol

@@ -12,6 +12,14 @@ contract DelegateApprovals is Owned {
     bytes32 public constant EXCHANGE_FOR_ADDRESS = "ExchangeForAddress";
     bytes32 public constant APPROVE_ALL = "ApproveAll";
 
+    bytes32[5] private _delegatableFunctions = [
+        APPROVE_ALL,
+        BURN_FOR_ADDRESS,
+        ISSUE_FOR_ADDRESS,
+        CLAIM_FOR_ADDRESS,
+        EXCHANGE_FOR_ADDRESS
+    ];
+
     /* ========== STATE VARIABLES ========== */
     EternalStorage public eternalStorage;
 
@@ -70,8 +78,11 @@ contract DelegateApprovals is Owned {
         _setApproval(APPROVE_ALL, msg.sender, delegate);
     }
 
+    // Removes all delegate approvals
     function removeAllDelegatePowers(address delegate) external {
-        _withdrawApproval(APPROVE_ALL, msg.sender, delegate);
+        for (uint i = 0; i < _delegatableFunctions.length; i++) {
+            _withdrawApproval(_delegatableFunctions[i], msg.sender, delegate);
+        }
     }
 
     // Burn on behalf
@@ -117,11 +128,15 @@ contract DelegateApprovals is Owned {
     }
 
     function _withdrawApproval(bytes32 action, address authoriser, address delegate) internal {
-        eternalStorage.deleteBooleanValue(_getKey(action, authoriser, delegate));
-        emit WithdrawApproval(authoriser, delegate, action);
+        // Check approval is set otherwise skip deleting approval
+        if (eternalStorage.getBooleanValue(_getKey(action, authoriser, delegate))) {
+            eternalStorage.deleteBooleanValue(_getKey(action, authoriser, delegate));
+            emit WithdrawApproval(authoriser, delegate, action);
+        }
     }
 
     function setEternalStorage(EternalStorage _eternalStorage) external onlyOwner {
+        require(_eternalStorage != address(0), "Can't set eternalStorage to address(0)");
         eternalStorage = _eternalStorage;
         emit EternalStorageUpdated(eternalStorage);
     }

contracts/interfaces/IDelegateApprovals.sol

@@ -2,11 +2,11 @@ pragma solidity 0.4.25;
 
 
 interface IDelegateApprovals {
-    function canBurnFor(address owner, address delegate) external view returns (bool);
+    function canBurnFor(address authoriser, address delegate) external view returns (bool);
 
-    function canIssueFor(address owner, address delegate) external view returns (bool);
+    function canIssueFor(address authoriser, address delegate) external view returns (bool);
 
-    function canClaimFor(address owner, address delegate) external view returns (bool);
+    function canClaimFor(address authoriser, address delegate) external view returns (bool);
 
-    function canExchangeFor(address owner, address delegate) external view returns (bool);
+    function canExchangeFor(address authoriser, address delegate) external view returns (bool);
 }

test/contracts/DelegateApprovals.js

@@ -4,6 +4,7 @@ const {
 	ensureOnlyExpectedMutativeFunctions,
 } = require('../utils/setupUtils');
 const { toBytes32 } = require('../../.');
+const { ZERO_ADDRESS } = require('../utils/testUtils');
 
 require('.'); // import common test scaffolding
 
@@ -46,6 +47,14 @@ contract('DelegateApprovals', async accounts => {
 				newEternalStorage: account1,
 			});
 		});
+		it('reverts if set to ZERO_ADDRESS', async () => {
+			await assert.revert(
+				delegateApprovals.setEternalStorage(ZERO_ADDRESS, {
+					from: owner,
+				}),
+				"Can't set eternalStorage to address(0)"
+			);
+		});
 	});
 
 	it('ensure only known functions are mutative', () => {
@@ -89,27 +98,26 @@ contract('DelegateApprovals', async accounts => {
 			assert.isTrue(result);
 
 			// remove approval
-			await delegateApprovals.removeAllDelegatePowers(delegate, { from: authoriser });
-			const newResult = await delegateApprovals.canBurnFor(authoriser, delegate);
-			assert.isNotTrue(newResult);
-		});
-		it('should add approval and emit an Approval event', async () => {
-			const transaction = await delegateApprovals.approveAllDelegatePowers(delegate, {
+			const transaction = await delegateApprovals.removeAllDelegatePowers(delegate, {
 				from: authoriser,
 			});
 
-			assert.eventEqual(transaction, 'Approval', {
+			// only WithdrawApproval event emitted for ApproveAll
+			assert.eventEqual(transaction, 'WithdrawApproval', {
 				authoriser: account1,
 				delegate: account2,
 				action: toBytes32('ApproveAll'),
 			});
+
+			const newResult = await delegateApprovals.canBurnFor(authoriser, delegate);
+			assert.isNotTrue(newResult);
 		});
-		it('should withdraw approval and emit an WithdrawApproval event', async () => {
-			const transaction = await delegateApprovals.removeAllDelegatePowers(delegate, {
+		it('should add approval and emit an Approval event', async () => {
+			const transaction = await delegateApprovals.approveAllDelegatePowers(delegate, {
 				from: authoriser,
 			});
 
-			assert.eventEqual(transaction, 'WithdrawApproval', {
+			assert.eventEqual(transaction, 'Approval', {
 				authoriser: account1,
 				delegate: account2,
 				action: toBytes32('ApproveAll'),
@@ -141,9 +149,81 @@ contract('DelegateApprovals', async accounts => {
 			assert.isTrue(result);
 
 			// remove approval
-			await delegateApprovals.removeExchangeOnBehalf(delegate, { from: authoriser });
+			const transaction = await delegateApprovals.removeExchangeOnBehalf(delegate, {
+				from: authoriser,
+			});
+
+			assert.eventEqual(transaction, 'WithdrawApproval', {
+				authoriser: account1,
+				delegate: account2,
+				action: toBytes32('ExchangeForAddress'),
+			});
+
 			const newResult = await delegateApprovals.canExchangeFor(authoriser, delegate);
 			assert.isNotTrue(newResult);
 		});
 	});
+
+	describe('when invoking removeAllDelegatePowers', async () => {
+		const authoriser = account1;
+		const delegate = account2;
+
+		beforeEach(async () => {
+			await delegateApprovals.approveExchangeOnBehalf(delegate, { from: authoriser });
+			await delegateApprovals.approveIssueOnBehalf(delegate, { from: authoriser });
+			await delegateApprovals.approveBurnOnBehalf(delegate, { from: authoriser });
+			await delegateApprovals.approveClaimOnBehalf(delegate, { from: authoriser });
+		});
+
+		it('should remove all delegate powers that have been set', async () => {
+			// check approvals is all true
+			assert.isTrue(await delegateApprovals.canExchangeFor(authoriser, delegate));
+			assert.isTrue(await delegateApprovals.canIssueFor(authoriser, delegate));
+			assert.isTrue(await delegateApprovals.canBurnFor(authoriser, delegate));
+			assert.isTrue(await delegateApprovals.canClaimFor(authoriser, delegate));
+
+			// invoke removeAllDelegatePowers
+			await await delegateApprovals.removeAllDelegatePowers(delegate, { from: authoriser });
+
+			// each delegations revoked
+			assert.isNotTrue(await delegateApprovals.canExchangeFor(authoriser, delegate));
+			assert.isNotTrue(await delegateApprovals.canIssueFor(authoriser, delegate));
+			assert.isNotTrue(await delegateApprovals.canBurnFor(authoriser, delegate));
+			assert.isNotTrue(await delegateApprovals.canClaimFor(authoriser, delegate));
+		});
+
+		it('should withdraw approval and emit an WithdrawApproval event for each withdrawn delegation', async () => {
+			const transaction = await delegateApprovals.removeAllDelegatePowers(delegate, {
+				from: authoriser,
+			});
+
+			assert.eventsEqual(
+				transaction,
+				'WithdrawApproval',
+				{
+					authoriser: account1,
+					delegate: account2,
+					action: toBytes32('BurnForAddress'),
+				},
+				'WithdrawApproval',
+				{
+					authoriser: account1,
+					delegate: account2,
+					action: toBytes32('IssueForAddress'),
+				},
+				'WithdrawApproval',
+				{
+					authoriser: account1,
+					delegate: account2,
+					action: toBytes32('ClaimForAddress'),
+				},
+				'WithdrawApproval',
+				{
+					authoriser: account1,
+					delegate: account2,
+					action: toBytes32('ExchangeForAddress'),
+				}
+			);
+		});
+	});
 });