feat: add ReentrancyGuard

contracts/apps/AragonApp.sol

@@ -6,6 +6,7 @@ pragma solidity ^0.4.24;
 
 import "./AppStorage.sol";
 import "../common/Autopetrified.sol";
+import "../common/ReentrancyGuard.sol";
 import "../common/VaultRecoverable.sol";
 import "../evmscript/EVMScriptRunner.sol";
 import "../acl/ACLSyntaxSugar.sol";
@@ -14,9 +15,9 @@ import "../acl/ACLSyntaxSugar.sol";
 // Contracts inheriting from AragonApp are, by default, immediately petrified upon deployment so
 // that they can never be initialized.
 // Unless overriden, this behaviour enforces those contracts to be usable only behind an AppProxy.
-// ACLSyntaxSugar and EVMScriptRunner are not directly used by this contract, but are included so
-// that they are automatically usable by subclassing contracts
-contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, EVMScriptRunner, ACLSyntaxSugar {
+// ReentrancyGuard, EVMScriptRunner, and ACLSyntaxSugar are not directly used by this contract, but
+// are included so that they are automatically usable by subclassing contracts
+contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, ReentrancyGuard, EVMScriptRunner, ACLSyntaxSugar {
     string private constant ERROR_AUTH_FAILED = "APP_AUTH_FAILED";
 
     modifier auth(bytes32 _role) {

contracts/common/ReentrancyGuard.sol

@@ -0,0 +1,33 @@
+/*
+ * SPDX-License-Identitifer:    MIT
+ */
+
+pragma solidity ^0.4.24;
+
+import "../common/UnstructuredStorage.sol";
+
+
+contract ReentrancyGuard {
+    using UnstructuredStorage for bytes32;
+
+    /* Hardcoded constants to save gas
+    bytes32 internal constant REENTRANCY_MUTEX_POSITION = keccak256("aragonOS.reentrancyGuard.mutex");
+    */
+    bytes32 private constant REENTRANCY_MUTEX_POSITION = 0xe855346402235fdd185c890e68d2c4ecad599b88587635ee285bce2fda58dacb;
+
+    string private constant ERROR_REENTRANT = "REENTRANCY_REENTRANT_CALL";
+
+    modifier nonReentrant() {
+        // Ensure mutex is unlocked
+        require(!REENTRANCY_MUTEX_POSITION.getStorageBool(), ERROR_REENTRANT);
+
+        // Lock mutex before function call
+        REENTRANCY_MUTEX_POSITION.setStorageBool(true);
+
+        // Perform function call
+        _;
+
+        // Unlock mutex after function call
+        REENTRANCY_MUTEX_POSITION.setStorageBool(false);
+    }
+}

contracts/test/mocks/KeccakConstants.sol

@@ -49,6 +49,7 @@ contract KeccakConstants {
     // Unstructured storage
     bytes32 public constant initializationBlockPosition = keccak256(abi.encodePacked("aragonOS.initializable.initializationBlock"));
     bytes32 public constant depositablePosition = keccak256(abi.encodePacked("aragonOS.depositableStorage.depositable"));
+    bytes32 public constant reentrancyGuardPosition = keccak256(abi.encodePacked("aragonOS.reentrancyGuard.mutex"));
     bytes32 public constant kernelPosition = keccak256(abi.encodePacked("aragonOS.appStorage.kernel"));
     bytes32 public constant appIdPosition = keccak256(abi.encodePacked("aragonOS.appStorage.appId"));
     bytes32 public constant pinnedCodePosition = keccak256(abi.encodePacked("aragonOS.appStorage.pinnedCode"));

contracts/test/mocks/ReentrancyGuardMock.sol

@@ -0,0 +1,53 @@
+pragma solidity 0.4.24;
+
+import "../../common/ReentrancyGuard.sol";
+import "../../common/UnstructuredStorage.sol";
+
+
+contract ReentrantActor {
+    bool reenterNonReentrant;
+
+    constructor(bool _reenterNonReentrant) public {
+        reenterNonReentrant = _reenterNonReentrant;
+    }
+
+    function reenter(ReentrancyGuardMock _mock) public {
+        // Set the reentrancy target to 0 so we don't infinite loop
+        ReentrantActor reentrancyTarget = ReentrantActor(0);
+
+        if (reenterNonReentrant) {
+            _mock.nonReentrantCall(reentrancyTarget);
+        } else {
+            _mock.reentrantCall(reentrancyTarget);
+        }
+    }
+}
+
+
+contract ReentrancyGuardMock is ReentrancyGuard {
+    using UnstructuredStorage for bytes32;
+
+    uint256 public callCounter;
+
+    function nonReentrantCall(ReentrantActor _target) public nonReentrant {
+        callCounter++;
+        if (_target != address(0)) {
+            _target.reenter(this);
+        }
+    }
+
+    function reentrantCall(ReentrantActor _target) public {
+        callCounter++;
+        if (_target != address(0)) {
+            _target.reenter(this);
+        }
+    }
+
+    function setReentrancyMutex(bool _mutex) public {
+        getReentrancyMutexPosition().setStorageBool(_mutex);
+    }
+
+    function getReentrancyMutexPosition() public pure returns (bytes32) {
+        return keccak256("aragonOS.reentrancyGuard.mutex");
+    }
+}

test/keccak_constants.js

@@ -115,4 +115,14 @@ contract('Constants', accounts => {
     const initializableMock = await getContract('InitializableStorageMock').new()
     assert.equal(await initializableMock.getInitializationBlockPosition(), await keccakConstants.initializationBlockPosition(), "initializationBlockPosition doesn't match")
   })
+
+  it('checks ReentrancyGuard unstructured storage constants', async () => {
+    const reentrancyGuardMock = await getContract('ReentrancyGuardMock').new()
+    // Note that this is a bit of a roundabout test for this unstructured storage slot. Since the
+    // position is declared as private in the base ReentrancyGuard contract, we redefine in the
+    // mock.
+    // This test therefore also relies on the ReentrancyGuard's own tests to make sure we've
+    // redefined the storage position correctly in the mock.
+    assert.equal(await reentrancyGuardMock.getReentrancyMutexPosition(), await keccakConstants.reentrancyGuardPosition(), "reentrancyGuardPosition doesn't match")
+  })
 })

test/reentrancy_guard.js

@@ -0,0 +1,90 @@
+const { assertRevert } = require('./helpers/assertThrow')
+
+const ZERO_ADDR = '0x0000000000000000000000000000000000000000'
+
+contract('ReentrancyGuard', accounts => {
+  let reentrancyMock
+
+  beforeEach(async () => {
+    reentrancyMock = await artifacts.require('ReentrancyGuardMock').new()
+  })
+
+  it('starts with false mutex', async () => {
+    assert.equal(
+      await web3.eth.getStorageAt(reentrancyMock.address, (await reentrancyMock.getReentrancyMutexPosition())),
+      false,
+      're-entrancy guard should start false'
+    )
+  })
+
+  it('starts with no calls', async () => {
+    assert.equal((await reentrancyMock.callCounter()).toString(), 0, 'should start with no calls')
+  })
+
+  it('can call re-entrant function normally', async () => {
+    await reentrancyMock.reentrantCall(ZERO_ADDR)
+    assert.equal((await reentrancyMock.callCounter()).toString(), 1, 'should have registered one call')
+  })
+
+  it('can call non-re-entrant function normally', async () => {
+    await reentrancyMock.nonReentrantCall(ZERO_ADDR)
+    assert.equal((await reentrancyMock.callCounter()).toString(), 1, 'should have registered one call')
+  })
+
+  context('> Enabled re-entrancy guard', () => {
+    beforeEach(async () => {
+      // Manually set re-entrancy guard
+      await reentrancyMock.setReentrancyMutex(true)
+    })
+
+    it('can call re-entrant function if re-entrancy guard is enabled', async () => {
+      await reentrancyMock.reentrantCall(ZERO_ADDR)
+      assert.equal((await reentrancyMock.callCounter()).toString(), 1, 'should have called')
+    })
+
+    it('can not call non-re-entrant function if re-entrancy guard is enabled', async () => {
+      await assertRevert(async () => {
+        await reentrancyMock.nonReentrantCall(ZERO_ADDR)
+      })
+      assert.equal((await reentrancyMock.callCounter()).toString(), 0, 'should not have called')
+    })
+  })
+
+  context('> Re-entering through contract', async () => {
+    let reentrantActor
+
+    context('> Re-enters re-entrant call', async () => {
+      before(async () => {
+        reentrantActor = await artifacts.require('ReentrantActor').new(false)
+      })
+
+      it('allows re-entering re-entrant call', async () => {
+        await reentrancyMock.reentrantCall(reentrantActor.address)
+        assert.equal((await reentrancyMock.callCounter()).toString(), 2, 'should have called twice')
+      })
+
+      it('allows entering non-re-entrant call from re-entrant call', async () => {
+        await reentrancyMock.nonReentrantCall(reentrantActor.address)
+        assert.equal((await reentrancyMock.callCounter()).toString(), 2, 'should have called twice')
+      })
+    })
+
+    context('> Re-enters non-reentrant call', async () => {
+      before(async () => {
+        reentrantActor = await artifacts.require('ReentrantActor').new(true)
+      })
+
+      it('disallows re-entering non-re-entrant call', async () => {
+        await assertRevert(async () => {
+          await reentrancyMock.nonReentrantCall(reentrantActor.address)
+        })
+        assert.equal((await reentrancyMock.callCounter()).toString(), 0, 'should not have completed any calls')
+      })
+
+      it('allows entering non-entrant call from re-entrant call', async () => {
+        await reentrancyMock.reentrantCall(reentrantActor.address)
+        assert.equal((await reentrancyMock.callCounter()).toString(), 2, 'should have called twice')
+      })
+    })
+  })
+})

test/unstructured_storage.js

@@ -7,6 +7,7 @@ const AppProxyPinnedStorageMock = artifacts.require('AppProxyPinnedStorageMock')
 const DepositableStorageMock = artifacts.require('DepositableStorageMock')
 const InitializableStorageMock = artifacts.require('InitializableStorageMock')
 const KernelPinnedStorageMock = artifacts.require('KernelPinnedStorageMock')
+const ReentrancyGuardMock = artifacts.require('ReentrancyGuardMock')
 
 contract('Unstructured storage', accounts => {
   context('> AppStorage', () => {
@@ -19,7 +20,7 @@ contract('Unstructured storage', accounts => {
     it('tests Kernel storage', async () => {
       const kernel = await Kernel.new(true)
       await appStorage.setKernelExt(kernel.address)
-      //checks
+      // checks
       assert.equal(
         await web3.eth.getStorageAt(appStorage.address, (await appStorage.getKernelPosition())),
         (await appStorage.kernel()).toString(),
@@ -35,7 +36,7 @@ contract('Unstructured storage', accounts => {
     it('tests appID storage', async () => {
       const appId = '0x1234000000000000000000000000000000000000000000000000000000000000'
       await appStorage.setAppIdExt(appId)
-      //checks
+      // checks
       assert.equal(
         await web3.eth.getStorageAt(appStorage.address, (await appStorage.getAppIdPosition())),
         (await appStorage.appId()).toString(),
@@ -61,7 +62,7 @@ contract('Unstructured storage', accounts => {
     it('tests pinnedCode storage', async () => {
       const pinnedCode = '0x1200000000000000000000000000000000005678'
       await appPinned.setPinnedCodeExt(pinnedCode)
-      //checks
+      // checks
       assert.equal(
         await web3.eth.getStorageAt(appPinned.address, (await appPinned.getPinnedCodePosition())),
         (await appPinned.pinnedCodeExt()).toString(),
@@ -85,7 +86,7 @@ contract('Unstructured storage', accounts => {
     it('tests depositable', async () => {
       // set values
       await depositableMock.setDepositableExt(true)
-      //checks
+      // checks
       assert.equal(
         await web3.eth.getStorageAt(depositableMock.address, (await depositableMock.getDepositablePosition())),
         true,
@@ -105,7 +106,7 @@ contract('Unstructured storage', accounts => {
       // set values
       await initializableMock.initialize()
       const blockNumber = web3.eth.blockNumber
-      //checks
+      // checks
       assert.equal(
         parseInt(
           await web3.eth.getStorageAt(
@@ -130,4 +131,23 @@ contract('Unstructured storage', accounts => {
       )
     })
   })
+
+  context('> ReentrancyGuard', () => {
+    let reentrancyGuardMock
+
+    beforeEach(async () => {
+      reentrancyGuardMock = await ReentrancyGuardMock.new()
+    })
+
+    it('tests reentrancy mutex', async () => {
+      // set values
+      await reentrancyGuardMock.setReentrancyMutex(true)
+      // checks
+      assert.equal(
+        await web3.eth.getStorageAt(reentrancyGuardMock.address, (await reentrancyGuardMock.getReentrancyMutexPosition())),
+        true,
+        'Reentrancy mutex should match'
+      )
+    })
+  })
 })