Skip to content

Commit

Permalink
Merge 070155c into dca0b4b
Browse files Browse the repository at this point in the history
  • Loading branch information
sohkai committed Apr 5, 2019
2 parents dca0b4b + 070155c commit 2511521
Show file tree
Hide file tree
Showing 7 changed files with 226 additions and 8 deletions.
7 changes: 4 additions & 3 deletions contracts/apps/AragonApp.sol
Expand Up @@ -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";
Expand All @@ -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) {
Expand Down
33 changes: 33 additions & 0 deletions 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);
}
}
1 change: 1 addition & 0 deletions contracts/test/mocks/KeccakConstants.sol
Expand Up @@ -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"));
Expand Down
53 changes: 53 additions & 0 deletions 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");
}
}
10 changes: 10 additions & 0 deletions test/keccak_constants.js
Expand Up @@ -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")
})
})
100 changes: 100 additions & 0 deletions test/reentrancy_guard.js
@@ -0,0 +1,100 @@
const { assertRevert } = require('./helpers/assertThrow')

const ZERO_ADDR = '0x0000000000000000000000000000000000000000'

contract('ReentrancyGuard', accounts => {
let reentrancyMock

async function assertReentrancyGuard(guard, msg) {
assert.equal(
await web3.eth.getStorageAt(reentrancyMock.address, (await reentrancyMock.getReentrancyMutexPosition())),
guard,
msg
)
}

beforeEach(async () => {
reentrancyMock = await artifacts.require('ReentrancyGuardMock').new()
})

it('starts with false mutex', async () => {
await assertReentrancyGuard(false, 're-entrancy mutex 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')
await assertReentrancyGuard(false, 're-entrancy mutex should end false')
})

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')
await assertReentrancyGuard(false, 're-entrancy mutex should end false')
})

context('> Enabled re-entrancy mutex', () => {
beforeEach(async () => {
// Manually set re-entrancy mutex
await reentrancyMock.setReentrancyMutex(true)
})

it('can call re-entrant function if re-entrancy mutex 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 mutex 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

afterEach(async () => {
await assertReentrancyGuard(false, 're-entrancy mutex should end false')
})

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')
})
})
})
})
30 changes: 25 additions & 5 deletions test/unstructured_storage.js
Expand Up @@ -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', () => {
Expand All @@ -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(),
Expand All @@ -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(),
Expand All @@ -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(),
Expand All @@ -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,
Expand All @@ -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(
Expand All @@ -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'
)
})
})
})

0 comments on commit 2511521

Please sign in to comment.