From 8c7a34a604430374becc8367e30af475fdd0f79d Mon Sep 17 00:00:00 2001 From: Marcin Chrzanowski Date: Thu, 24 Oct 2019 19:19:52 +0200 Subject: [PATCH 1/8] Add Freezable contract --- .../protocol/contracts/common/Freezable.sol | 36 ++++++ .../contracts/common/test/FreezableTest.sol | 24 ++++ packages/protocol/test/common/freezable.ts | 106 ++++++++++++++++++ 3 files changed, 166 insertions(+) create mode 100644 packages/protocol/contracts/common/Freezable.sol create mode 100644 packages/protocol/contracts/common/test/FreezableTest.sol create mode 100644 packages/protocol/test/common/freezable.ts diff --git a/packages/protocol/contracts/common/Freezable.sol b/packages/protocol/contracts/common/Freezable.sol new file mode 100644 index 00000000000..e8f25db92eb --- /dev/null +++ b/packages/protocol/contracts/common/Freezable.sol @@ -0,0 +1,36 @@ +pragma solidity ^0.5.3; + + +contract Freezable { + bool public frozen; + address public freezer; + + modifier onlyFreezer() { + require(msg.sender == freezer); + _; + } + + modifier onlyWhenNotFrozen() { + if (!frozen) { + _; + } + } + + modifier onlyWhenNotFrozenOrThrow() { + require(!frozen); + _; + } + + function _setFreezer(address _freezer) internal { + freezer = _freezer; + } + + function freeze() external onlyFreezer { + frozen = true; + } + + function unfreeze() external onlyFreezer { + frozen = false; + } +} + diff --git a/packages/protocol/contracts/common/test/FreezableTest.sol b/packages/protocol/contracts/common/test/FreezableTest.sol new file mode 100644 index 00000000000..21c2efe23e2 --- /dev/null +++ b/packages/protocol/contracts/common/test/FreezableTest.sol @@ -0,0 +1,24 @@ +pragma solidity ^0.5.8; + +import "../Freezable.sol"; + + +contract FreezableTest is Freezable { + event FunctionCalled(); + + function setFreezer(address _freezer) external { + _setFreezer(_freezer); + } + + function freezableFunction() external onlyWhenNotFrozen { + emit FunctionCalled(); + } + + function freezableFunctionWithThrow() external onlyWhenNotFrozenOrThrow { + emit FunctionCalled(); + } + + function nonfreezableFunction() external { + emit FunctionCalled(); + } +} diff --git a/packages/protocol/test/common/freezable.ts b/packages/protocol/test/common/freezable.ts new file mode 100644 index 00000000000..67889ba39fe --- /dev/null +++ b/packages/protocol/test/common/freezable.ts @@ -0,0 +1,106 @@ +import { assertSameAddress, assertLogMatches2, assertRevert } from '@celo/protocol/lib/test-utils' +import { FreezableTestInstance } from 'types' + +contract('Freezable', (accounts: string[]) => { + const FreezableTest = artifacts.require('FreezableTest') + let freezableTest: FreezableTestInstance + + beforeEach(async () => { + freezableTest = await FreezableTest.new() + freezableTest.setFreezer(accounts[0]) + }) + + describe('_setFreezer', () => { + it('should allow owner to change the freezer', async () => { + // _setFreezer is internal in Freezable, FreezableTest wraps around it with setFreezer + await freezableTest.setFreezer(accounts[1]) + const freezer = await freezableTest.freezer() + assertSameAddress(freezer, accounts[1]) + }) + }) + + describe('when the contract is not frozen', () => { + it('should allow freezable functions to be called', async () => { + const resp = await freezableTest.freezableFunction() + assert.equal(resp.logs.length, 1) + const log = resp.logs[0] + assertLogMatches2(log, { + event: 'FunctionCalled', + args: {}, + }) + }) + + it('should allow freezable functions that throw to be called', async () => { + const resp = await freezableTest.freezableFunctionWithThrow() + assert.equal(resp.logs.length, 1) + const log = resp.logs[0] + assertLogMatches2(log, { + event: 'FunctionCalled', + args: {}, + }) + }) + + it('should allow non-freezable functions to be called', async () => { + const resp = await freezableTest.nonfreezableFunction() + assert.equal(resp.logs.length, 1) + const log = resp.logs[0] + assertLogMatches2(log, { + event: 'FunctionCalled', + args: {}, + }) + }) + }) + + describe('freeze', () => { + it('should allow freezer to freeze the contract', async () => { + await freezableTest.freeze() + const frozen = await freezableTest.frozen() + assert.isTrue(frozen) + }) + + it('should not allow a non-freezer to freeze the contract', async () => { + await assertRevert(freezableTest.freeze({ from: accounts[1] })) + }) + }) + + describe('unfreeze', () => { + beforeEach(async () => { + freezableTest.freeze() + }) + + it('should allow freezer to unfreeze the contract', async () => { + await freezableTest.unfreeze() + const frozen = await freezableTest.frozen() + assert.isFalse(frozen) + }) + + it('should not allow a non-freezer to unfreeze the contract', async () => { + await assertRevert(freezableTest.freeze({ from: accounts[1] })) + }) + }) + + describe('when the contract is frozen', () => { + beforeEach(async () => { + await freezableTest.freeze() + }) + + it('should not run a freezable function', async () => { + const resp = await freezableTest.freezableFunction() + assert.equal(resp.logs.length, 0) + }) + + it('should revert a freezable function that throws', async () => { + await assertRevert(freezableTest.freezableFunctionWithThrow()) + }) + + it('should not affect a non-freezable function', async () => { + const resp = await freezableTest.nonfreezableFunction() + assert.equal(resp.logs.length, 1) + const log = resp.logs[0] + assertLogMatches2(log, { + event: 'FunctionCalled', + args: {}, + }) + }) + }) +}) From bf96502f57e77ecda7ff037ff415a088bb61c827 Mon Sep 17 00:00:00 2001 From: Marcin Chrzanowski Date: Thu, 24 Oct 2019 21:13:09 +0200 Subject: [PATCH 2/8] Enable freezing of the exchange --- packages/protocol/contracts/stability/Exchange.sol | 10 +++++++++- .../contracts/stability/interfaces/IExchange.sol | 1 + packages/protocol/migrations/09_exchange.ts | 5 ++++- packages/protocol/test/stability/exchange.ts | 13 +++++++++++++ 4 files changed, 27 insertions(+), 2 deletions(-) diff --git a/packages/protocol/contracts/stability/Exchange.sol b/packages/protocol/contracts/stability/Exchange.sol index 5e0e457422e..0a4b90050bf 100644 --- a/packages/protocol/contracts/stability/Exchange.sol +++ b/packages/protocol/contracts/stability/Exchange.sol @@ -10,6 +10,7 @@ import "./interfaces/IStableToken.sol"; import "../common/FractionUtil.sol"; import "../common/Initializable.sol"; import "../common/FixidityLib.sol"; +import "../common/Freezable.sol"; import "../common/UsingRegistry.sol"; @@ -17,7 +18,7 @@ import "../common/UsingRegistry.sol"; * @title Contract that allows to exchange StableToken for GoldToken and vice versa * using a Constant Product Market Maker Model */ -contract Exchange is IExchange, Initializable, Ownable, UsingRegistry, ReentrancyGuard { +contract Exchange is IExchange, Initializable, Ownable, UsingRegistry, ReentrancyGuard, Freezable { using SafeMath for uint256; using FractionUtil for FractionUtil.Fraction; using FixidityLib for FixidityLib.Fraction; @@ -89,6 +90,7 @@ contract Exchange is IExchange, Initializable, Ownable, UsingRegistry, Reentranc */ function initialize( address registryAddress, + address _freezer, address stableToken, uint256 _spread, uint256 _reserveFraction, @@ -99,6 +101,7 @@ contract Exchange is IExchange, Initializable, Ownable, UsingRegistry, Reentranc initializer { _transferOwnership(msg.sender); + setFreezer(_freezer); setRegistry(registryAddress); setStableToken(stableToken); setSpread(_spread); @@ -123,6 +126,7 @@ contract Exchange is IExchange, Initializable, Ownable, UsingRegistry, Reentranc bool sellGold ) external + onlyWhenNotFrozenOrThrow updateBucketsIfNecessary nonReentrant returns (uint256) @@ -254,6 +258,10 @@ contract Exchange is IExchange, Initializable, Ownable, UsingRegistry, Reentranc emit MinimumReportsSet(newMininumReports); } + function setFreezer(address freezer) public onlyOwner { + _setFreezer(freezer); + } + /** * @notice Allows owner to set the Stable Token address * @param newStableToken The new address for Stable Token diff --git a/packages/protocol/contracts/stability/interfaces/IExchange.sol b/packages/protocol/contracts/stability/interfaces/IExchange.sol index 8be29102eb3..b70d8250ecc 100644 --- a/packages/protocol/contracts/stability/interfaces/IExchange.sol +++ b/packages/protocol/contracts/stability/interfaces/IExchange.sol @@ -4,6 +4,7 @@ pragma solidity ^0.5.3; interface IExchange { function initialize( + address, address, address, uint256, diff --git a/packages/protocol/migrations/09_exchange.ts b/packages/protocol/migrations/09_exchange.ts index 0eedb8069f1..839d9c5c19e 100644 --- a/packages/protocol/migrations/09_exchange.ts +++ b/packages/protocol/migrations/09_exchange.ts @@ -8,8 +8,10 @@ import { import { config } from '@celo/protocol/migrationsConfig' import { toFixed } from '@celo/utils/lib/fixidity' import { ExchangeInstance, ReserveInstance, StableTokenInstance } from 'types' +const truffle = require('@celo/protocol/truffle-config.js') -const initializeArgs = async (): Promise => { +const initializeArgs = async (networkName: string): Promise => { + const network: any = truffle.networks[networkName] const stableToken: StableTokenInstance = await getDeployedProxiedContract( 'StableToken', artifacts @@ -17,6 +19,7 @@ const initializeArgs = async (): Promise => { return [ config.registry.predeployedProxyAddress, stableToken.address, + network.from, toFixed(config.exchange.spread).toString(), toFixed(config.exchange.reserveFraction).toString(), config.exchange.updateFrequency, diff --git a/packages/protocol/test/stability/exchange.ts b/packages/protocol/test/stability/exchange.ts index eddd3b9183a..37b6ca843ed 100644 --- a/packages/protocol/test/stability/exchange.ts +++ b/packages/protocol/test/stability/exchange.ts @@ -119,6 +119,7 @@ contract('Exchange', (accounts: string[]) => { exchange = await Exchange.new() await exchange.initialize( registry.address, + accounts[0], stableToken.address, spread, reserveFraction, @@ -139,6 +140,7 @@ contract('Exchange', (accounts: string[]) => { await assertRevert( exchange.initialize( registry.address, + accounts[0], stableToken.address, spread, reserveFraction, @@ -792,5 +794,16 @@ contract('Exchange', (accounts: string[]) => { }) }) }) + + describe('when the contract is frozen', () => { + beforeEach(async () => { + await exchange.freeze() + }) + + it('should revert', async () => { + await goldToken.approve(exchange.address, 1000) + await assertRevert(exchange.exchange(1000, 1, true)) + }) + }) }) }) From 2975bb1b473053076cc4b1193257281f9313da4a Mon Sep 17 00:00:00 2001 From: Marcin Chrzanowski Date: Thu, 24 Oct 2019 21:25:55 +0200 Subject: [PATCH 3/8] Move Freezable to new baklava directory --- packages/protocol/contracts/{common => baklava}/Freezable.sol | 0 .../contracts/{common => baklava}/test/FreezableTest.sol | 0 packages/protocol/contracts/stability/Exchange.sol | 2 +- packages/protocol/test/{common => baklava}/freezable.ts | 0 4 files changed, 1 insertion(+), 1 deletion(-) rename packages/protocol/contracts/{common => baklava}/Freezable.sol (100%) rename packages/protocol/contracts/{common => baklava}/test/FreezableTest.sol (100%) rename packages/protocol/test/{common => baklava}/freezable.ts (100%) diff --git a/packages/protocol/contracts/common/Freezable.sol b/packages/protocol/contracts/baklava/Freezable.sol similarity index 100% rename from packages/protocol/contracts/common/Freezable.sol rename to packages/protocol/contracts/baklava/Freezable.sol diff --git a/packages/protocol/contracts/common/test/FreezableTest.sol b/packages/protocol/contracts/baklava/test/FreezableTest.sol similarity index 100% rename from packages/protocol/contracts/common/test/FreezableTest.sol rename to packages/protocol/contracts/baklava/test/FreezableTest.sol diff --git a/packages/protocol/contracts/stability/Exchange.sol b/packages/protocol/contracts/stability/Exchange.sol index 0a4b90050bf..cd29f75dbeb 100644 --- a/packages/protocol/contracts/stability/Exchange.sol +++ b/packages/protocol/contracts/stability/Exchange.sol @@ -10,7 +10,7 @@ import "./interfaces/IStableToken.sol"; import "../common/FractionUtil.sol"; import "../common/Initializable.sol"; import "../common/FixidityLib.sol"; -import "../common/Freezable.sol"; +import "../baklava/Freezable.sol"; import "../common/UsingRegistry.sol"; diff --git a/packages/protocol/test/common/freezable.ts b/packages/protocol/test/baklava/freezable.ts similarity index 100% rename from packages/protocol/test/common/freezable.ts rename to packages/protocol/test/baklava/freezable.ts From 4a546f8aaec213637a896eaa8ace8e25001f7964 Mon Sep 17 00:00:00 2001 From: Marcin Chrzanowski Date: Fri, 25 Oct 2019 10:42:14 +0200 Subject: [PATCH 4/8] Add natspecs --- .../protocol/contracts/baklava/Freezable.sol | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/packages/protocol/contracts/baklava/Freezable.sol b/packages/protocol/contracts/baklava/Freezable.sol index e8f25db92eb..79878268257 100644 --- a/packages/protocol/contracts/baklava/Freezable.sol +++ b/packages/protocol/contracts/baklava/Freezable.sol @@ -5,32 +5,47 @@ contract Freezable { bool public frozen; address public freezer; + // onlyFreezer functions can only be called by the specified `freezer` address modifier onlyFreezer() { require(msg.sender == freezer); _; } + // onlyWhenNotFrozen functions can only be called when `frozen` is false, otherwise they are + // noops. modifier onlyWhenNotFrozen() { if (!frozen) { _; } } + // onlyWhenNotFrozenOrThrow functions can only be called when `frozen` is false, otherwise they + // will revert. modifier onlyWhenNotFrozenOrThrow() { require(!frozen); _; } + /** + * @notice Sets the address that is allowed to freeze/unfreeze the contract. + * @param _freezer The address that is allowed to freeze/unfree the contract + * @dev This function is `internal` and leaves its permissioning up to the inheriting contract. + */ function _setFreezer(address _freezer) internal { freezer = _freezer; } + /** + * @notice Freezes the contract, disabling `onlyWhenNotFrozen` functions. + */ function freeze() external onlyFreezer { frozen = true; } + /** + * @notice Unreezes the contract, enabling `onlyWhenNotFrozen` functions. + */ function unfreeze() external onlyFreezer { frozen = false; } } - From 906d0508e391684a61c97d43b73976f5f0db015e Mon Sep 17 00:00:00 2001 From: Marcin Chrzanowski Date: Fri, 25 Oct 2019 10:43:38 +0200 Subject: [PATCH 5/8] Add comment --- packages/protocol/contracts/stability/Exchange.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/protocol/contracts/stability/Exchange.sol b/packages/protocol/contracts/stability/Exchange.sol index cd29f75dbeb..7b02d2d22fd 100644 --- a/packages/protocol/contracts/stability/Exchange.sol +++ b/packages/protocol/contracts/stability/Exchange.sol @@ -119,6 +119,7 @@ contract Exchange is IExchange, Initializable, Ownable, UsingRegistry, Reentranc * transaction to succeed * @param sellGold `true` if gold is the sell token * @return The amount of buyToken that was transfered + * @dev This function can be frozen using the Freezable interface. */ function exchange( uint256 sellAmount, From cf91eebd3f3226893a3a9869e2473ec568245f1f Mon Sep 17 00:00:00 2001 From: Marcin Chrzanowski Date: Fri, 25 Oct 2019 21:54:39 +0200 Subject: [PATCH 6/8] Appease the linter --- .../protocol/contracts/baklava/Freezable.sol | 12 ++++----- .../contracts/baklava/test/FreezableTest.sol | 26 +++++++++---------- .../protocol/contracts/stability/Exchange.sol | 2 +- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/packages/protocol/contracts/baklava/Freezable.sol b/packages/protocol/contracts/baklava/Freezable.sol index 79878268257..16ed12bf405 100644 --- a/packages/protocol/contracts/baklava/Freezable.sol +++ b/packages/protocol/contracts/baklava/Freezable.sol @@ -7,15 +7,15 @@ contract Freezable { // onlyFreezer functions can only be called by the specified `freezer` address modifier onlyFreezer() { - require(msg.sender == freezer); - _; + require(msg.sender == freezer); + _; } // onlyWhenNotFrozen functions can only be called when `frozen` is false, otherwise they are // noops. modifier onlyWhenNotFrozen() { if (!frozen) { - _; + _; } } @@ -32,20 +32,20 @@ contract Freezable { * @dev This function is `internal` and leaves its permissioning up to the inheriting contract. */ function _setFreezer(address _freezer) internal { - freezer = _freezer; + freezer = _freezer; } /** * @notice Freezes the contract, disabling `onlyWhenNotFrozen` functions. */ function freeze() external onlyFreezer { - frozen = true; + frozen = true; } /** * @notice Unreezes the contract, enabling `onlyWhenNotFrozen` functions. */ function unfreeze() external onlyFreezer { - frozen = false; + frozen = false; } } diff --git a/packages/protocol/contracts/baklava/test/FreezableTest.sol b/packages/protocol/contracts/baklava/test/FreezableTest.sol index 21c2efe23e2..2888dcb0824 100644 --- a/packages/protocol/contracts/baklava/test/FreezableTest.sol +++ b/packages/protocol/contracts/baklava/test/FreezableTest.sol @@ -4,21 +4,21 @@ import "../Freezable.sol"; contract FreezableTest is Freezable { - event FunctionCalled(); + event FunctionCalled(); - function setFreezer(address _freezer) external { - _setFreezer(_freezer); - } + function setFreezer(address _freezer) external { + _setFreezer(_freezer); + } - function freezableFunction() external onlyWhenNotFrozen { - emit FunctionCalled(); - } + function freezableFunction() external onlyWhenNotFrozen { + emit FunctionCalled(); + } - function freezableFunctionWithThrow() external onlyWhenNotFrozenOrThrow { - emit FunctionCalled(); - } + function freezableFunctionWithThrow() external onlyWhenNotFrozenOrThrow { + emit FunctionCalled(); + } - function nonfreezableFunction() external { - emit FunctionCalled(); - } + function nonfreezableFunction() external { + emit FunctionCalled(); + } } diff --git a/packages/protocol/contracts/stability/Exchange.sol b/packages/protocol/contracts/stability/Exchange.sol index 7b02d2d22fd..8d816e82a21 100644 --- a/packages/protocol/contracts/stability/Exchange.sol +++ b/packages/protocol/contracts/stability/Exchange.sol @@ -260,7 +260,7 @@ contract Exchange is IExchange, Initializable, Ownable, UsingRegistry, Reentranc } function setFreezer(address freezer) public onlyOwner { - _setFreezer(freezer); + _setFreezer(freezer); } /** From 7df038d4d7ccbc04b713a93ae65710afff67ba6d Mon Sep 17 00:00:00 2001 From: Marcin Chrzanowski Date: Mon, 28 Oct 2019 21:11:19 +0100 Subject: [PATCH 7/8] Order arguments correctly --- packages/protocol/migrations/09_exchange.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/migrations/09_exchange.ts b/packages/protocol/migrations/09_exchange.ts index 96c3ab8888b..b0e1f4b97ef 100644 --- a/packages/protocol/migrations/09_exchange.ts +++ b/packages/protocol/migrations/09_exchange.ts @@ -18,8 +18,8 @@ const initializeArgs = async (networkName: string): Promise => { ) return [ config.registry.predeployedProxyAddress, - stableToken.address, network.from, + stableToken.address, toFixed(config.exchange.spread).toString(), toFixed(config.exchange.reserveFraction).toString(), config.exchange.updateFrequency, From e4b9d6392bb8c1f152401ab66e4386232af6503b Mon Sep 17 00:00:00 2001 From: Marcin Chrzanowski Date: Sat, 2 Nov 2019 00:23:21 +0100 Subject: [PATCH 8/8] Remove non-throwing modifier --- .../protocol/contracts/baklava/Freezable.sol | 12 ++---------- .../contracts/baklava/test/FreezableTest.sol | 4 ---- .../protocol/contracts/stability/Exchange.sol | 2 +- packages/protocol/test/baklava/freezable.ts | 19 ++----------------- 4 files changed, 5 insertions(+), 32 deletions(-) diff --git a/packages/protocol/contracts/baklava/Freezable.sol b/packages/protocol/contracts/baklava/Freezable.sol index 16ed12bf405..2d0a57a5c87 100644 --- a/packages/protocol/contracts/baklava/Freezable.sol +++ b/packages/protocol/contracts/baklava/Freezable.sol @@ -11,17 +11,9 @@ contract Freezable { _; } - // onlyWhenNotFrozen functions can only be called when `frozen` is false, otherwise they are - // noops. + // onlyWhenNotFrozen functions can only be called when `frozen` is false, otherwise they will + // revert. modifier onlyWhenNotFrozen() { - if (!frozen) { - _; - } - } - - // onlyWhenNotFrozenOrThrow functions can only be called when `frozen` is false, otherwise they - // will revert. - modifier onlyWhenNotFrozenOrThrow() { require(!frozen); _; } diff --git a/packages/protocol/contracts/baklava/test/FreezableTest.sol b/packages/protocol/contracts/baklava/test/FreezableTest.sol index 2888dcb0824..23c5baf55fa 100644 --- a/packages/protocol/contracts/baklava/test/FreezableTest.sol +++ b/packages/protocol/contracts/baklava/test/FreezableTest.sol @@ -14,10 +14,6 @@ contract FreezableTest is Freezable { emit FunctionCalled(); } - function freezableFunctionWithThrow() external onlyWhenNotFrozenOrThrow { - emit FunctionCalled(); - } - function nonfreezableFunction() external { emit FunctionCalled(); } diff --git a/packages/protocol/contracts/stability/Exchange.sol b/packages/protocol/contracts/stability/Exchange.sol index 8d816e82a21..0291998717e 100644 --- a/packages/protocol/contracts/stability/Exchange.sol +++ b/packages/protocol/contracts/stability/Exchange.sol @@ -127,7 +127,7 @@ contract Exchange is IExchange, Initializable, Ownable, UsingRegistry, Reentranc bool sellGold ) external - onlyWhenNotFrozenOrThrow + onlyWhenNotFrozen updateBucketsIfNecessary nonReentrant returns (uint256) diff --git a/packages/protocol/test/baklava/freezable.ts b/packages/protocol/test/baklava/freezable.ts index 67889ba39fe..1c7431a80f6 100644 --- a/packages/protocol/test/baklava/freezable.ts +++ b/packages/protocol/test/baklava/freezable.ts @@ -30,16 +30,6 @@ contract('Freezable', (accounts: string[]) => { }) }) - it('should allow freezable functions that throw to be called', async () => { - const resp = await freezableTest.freezableFunctionWithThrow() - assert.equal(resp.logs.length, 1) - const log = resp.logs[0] - assertLogMatches2(log, { - event: 'FunctionCalled', - args: {}, - }) - }) - it('should allow non-freezable functions to be called', async () => { const resp = await freezableTest.nonfreezableFunction() assert.equal(resp.logs.length, 1) @@ -84,13 +74,8 @@ contract('Freezable', (accounts: string[]) => { await freezableTest.freeze() }) - it('should not run a freezable function', async () => { - const resp = await freezableTest.freezableFunction() - assert.equal(resp.logs.length, 0) - }) - - it('should revert a freezable function that throws', async () => { - await assertRevert(freezableTest.freezableFunctionWithThrow()) + it('should revert a freezable function', async () => { + await assertRevert(freezableTest.freezableFunction()) }) it('should not affect a non-freezable function', async () => {