From 9e9b0e3255db7930f536ddce4e1b2eda61367f04 Mon Sep 17 00:00:00 2001 From: satyam Date: Thu, 27 Sep 2018 20:24:30 +0530 Subject: [PATCH 1/2] overruling the whenNotPaused modifier when msg.sender is owner --- contracts/ModuleRegistry.sol | 10 +++++++--- contracts/SecurityTokenRegistry.sol | 11 ++++++++--- test/k_module_registry.js | 2 +- test/n_security_token_registry.js | 12 ------------ 4 files changed, 16 insertions(+), 19 deletions(-) diff --git a/contracts/ModuleRegistry.sol b/contracts/ModuleRegistry.sol index 94878303d..b9c72c0dd 100644 --- a/contracts/ModuleRegistry.sol +++ b/contracts/ModuleRegistry.sol @@ -70,8 +70,12 @@ contract ModuleRegistry is IModuleRegistry, EternalStorage { * @notice Modifier to make a function callable only when the contract is not paused. */ modifier whenNotPaused() { - require(!getBool(Encoder.getKey("paused")), "Already paused"); - _; + if (msg.sender == getAddress(Encoder.getKey("owner"))) + _; + else { + require(!getBool(Encoder.getKey("paused")), "Already paused"); + _; + } } /** @@ -296,7 +300,7 @@ contract ModuleRegistry is IModuleRegistry, EternalStorage { /** * @notice Called by the owner to pause, triggers stopped state */ - function pause() external whenNotPaused onlyOwner { + function pause() external onlyOwner { set(Encoder.getKey("paused"), true); emit Pause(now); } diff --git a/contracts/SecurityTokenRegistry.sol b/contracts/SecurityTokenRegistry.sol index 9a4e66b0b..495d7c3df 100644 --- a/contracts/SecurityTokenRegistry.sol +++ b/contracts/SecurityTokenRegistry.sol @@ -116,10 +116,15 @@ contract SecurityTokenRegistry is ISecurityTokenRegistry, EternalStorage { * @notice Modifier to make a function callable only when the contract is not paused. */ modifier whenNotPaused() { - require(!getBool(Encoder.getKey("paused")), "Already paused"); - _; + if (msg.sender == getAddress(Encoder.getKey("owner"))) + _; + else { + require(!getBool(Encoder.getKey("paused")), "Already paused"); + _; + } } + /** * @notice Modifier to make a function callable only when the contract is paused. */ @@ -557,7 +562,7 @@ contract SecurityTokenRegistry is ISecurityTokenRegistry, EternalStorage { /** * @notice called by the owner to pause, triggers stopped state */ - function pause() external whenNotPaused onlyOwner { + function pause() external onlyOwner { set(Encoder.getKey("paused"), true); emit Pause(now); } diff --git a/test/k_module_registry.js b/test/k_module_registry.js index 6843db491..55cd836e0 100644 --- a/test/k_module_registry.js +++ b/test/k_module_registry.js @@ -261,7 +261,7 @@ contract('ModuleRegistry', accounts => { await I_MRProxied.pause({from: account_polymath}); let errorThrown = false; try { - let tx = await I_MRProxied.registerModule(I_GeneralTransferManagerFactory.address, {from: account_polymath}); + let tx = await I_MRProxied.registerModule(I_GeneralTransferManagerFactory.address, {from: account_delegate}); } catch(error) { console.log(` tx -> revert because already registered modules are not allowed`); errorThrown = true; diff --git a/test/n_security_token_registry.js b/test/n_security_token_registry.js index a53378c8b..3ccb90f8b 100644 --- a/test/n_security_token_registry.js +++ b/test/n_security_token_registry.js @@ -383,18 +383,6 @@ contract('SecurityTokenRegistry', accounts => { assert.ok(errorThrown, message); }); - it("Should fail to pause if already paused", async() => { - let errorThrown = false; - try { - await I_STRProxied.pause({ from: account_polymath}); - } catch(error) { - console.log(` tx revert -> Registration is already paused`.grey); - errorThrown = true; - ensureException(error); - } - assert.ok(errorThrown, message); - }); - it("Should successfully register ticker if registration is unpaused", async() => { await I_STRProxied.unpause({ from: account_polymath}); await I_PolyToken.approve(I_STRProxied.address, initRegFee, { from: token_owner}); From 825208e8f4ef443560e080cc8dc0875e9e406f63 Mon Sep 17 00:00:00 2001 From: satyam Date: Fri, 28 Sep 2018 13:24:12 +0530 Subject: [PATCH 2/2] some fixes --- contracts/ModuleRegistry.sol | 16 ++++++--- contracts/SecurityTokenRegistry.sol | 36 +++++++++++-------- contracts/mocks/SecurityTokenRegistryMock.sol | 5 ++- test/n_security_token_registry.js | 12 +++++++ 4 files changed, 48 insertions(+), 21 deletions(-) diff --git a/contracts/ModuleRegistry.sol b/contracts/ModuleRegistry.sol index b9c72c0dd..c6b150385 100644 --- a/contracts/ModuleRegistry.sol +++ b/contracts/ModuleRegistry.sol @@ -69,7 +69,7 @@ contract ModuleRegistry is IModuleRegistry, EternalStorage { /** * @notice Modifier to make a function callable only when the contract is not paused. */ - modifier whenNotPaused() { + modifier whenNotPausedOrOwner() { if (msg.sender == getAddress(Encoder.getKey("owner"))) _; else { @@ -78,6 +78,14 @@ contract ModuleRegistry is IModuleRegistry, EternalStorage { } } + /** + * @notice Modifier to make a function callable only when the contract is not paused and ignore is msg.sender is owner. + */ + modifier whenNotPaused() { + require(!getBool(Encoder.getKey("paused")), "Already paused"); + _; + } + /** * @notice Modifier to make a function callable only when the contract is paused. */ @@ -135,7 +143,7 @@ contract ModuleRegistry is IModuleRegistry, EternalStorage { * @notice Called by the ModuleFactory owner to register new modules for SecurityTokens to use * @param _moduleFactory is the address of the module factory to be registered */ - function registerModule(address _moduleFactory) external whenNotPaused { + function registerModule(address _moduleFactory) external whenNotPausedOrOwner { require(getUint(Encoder.getKey('registry', _moduleFactory)) == 0, "Module factory should not be pre-registered"); IModuleFactory moduleFactory = IModuleFactory(_moduleFactory); uint8 moduleType = moduleFactory.getType(); @@ -150,7 +158,7 @@ contract ModuleRegistry is IModuleRegistry, EternalStorage { * @notice Called by the ModuleFactory owner or registry curator to delete a ModuleFactory from the registry * @param _moduleFactory is the address of the module factory to be deleted from the registry */ - function removeModule(address _moduleFactory) external whenNotPaused { + function removeModule(address _moduleFactory) external whenNotPausedOrOwner { uint256 moduleType = getUint(Encoder.getKey('registry', _moduleFactory)); require(moduleType != 0, "Module factory should be registered"); @@ -300,7 +308,7 @@ contract ModuleRegistry is IModuleRegistry, EternalStorage { /** * @notice Called by the owner to pause, triggers stopped state */ - function pause() external onlyOwner { + function pause() external whenNotPaused onlyOwner { set(Encoder.getKey("paused"), true); emit Pause(now); } diff --git a/contracts/SecurityTokenRegistry.sol b/contracts/SecurityTokenRegistry.sol index 7d265d988..1ae6193ff 100644 --- a/contracts/SecurityTokenRegistry.sol +++ b/contracts/SecurityTokenRegistry.sol @@ -115,7 +115,7 @@ contract SecurityTokenRegistry is ISecurityTokenRegistry, EternalStorage { /** * @notice Modifier to make a function callable only when the contract is not paused. */ - modifier whenNotPaused() { + modifier whenNotPausedOrOwner() { if (msg.sender == getAddress(Encoder.getKey("owner"))) _; else { @@ -124,6 +124,14 @@ contract SecurityTokenRegistry is ISecurityTokenRegistry, EternalStorage { } } + /** + * @notice Modifier to make a function callable only when the contract is not paused and ignore is msg.sender is owner. + */ + modifier whenNotPaused() { + require(!getBool(Encoder.getKey("paused")), "Already paused"); + _; + } + /** * @notice Modifier to make a function callable only when the contract is paused. @@ -154,7 +162,7 @@ contract SecurityTokenRegistry is ISecurityTokenRegistry, EternalStorage { */ function initialize(address _polymathRegistry, address _STFactory, uint256 _stLaunchFee, uint256 _tickerRegFee, address _polyToken, address _owner) payable external { require(!getBool(Encoder.getKey("initialised"))); - require(_STFactory != address(0) && _polyToken != address(0) && _owner != address(0) && _polymathRegistry != address(0), "0x address is in-valid"); + require(_STFactory != address(0) && _polyToken != address(0) && _owner != address(0) && _polymathRegistry != address(0), "In-valid address"); require(_stLaunchFee != 0 && _tickerRegFee != 0, "Fees should not be 0"); // address polyToken = _polyToken; set(Encoder.getKey("polyToken"), _polyToken); @@ -180,7 +188,7 @@ contract SecurityTokenRegistry is ISecurityTokenRegistry, EternalStorage { * @param _ticker is unique token ticker * @param _tokenName is the name of the token */ - function registerTicker(address _owner, string _ticker, string _tokenName) external whenNotPaused { + function registerTicker(address _owner, string _ticker, string _tokenName) external whenNotPausedOrOwner { require(_owner != address(0), "Owner should not be 0x"); require(bytes(_ticker).length > 0 && bytes(_ticker).length <= 10, "Ticker length range (0,10]"); // Attempt to charge the reg fee if it is > 0 POLY @@ -210,7 +218,7 @@ contract SecurityTokenRegistry is ISecurityTokenRegistry, EternalStorage { require(bytes(_ticker).length > 0 && bytes(_ticker).length <= 10, "Ticker length range (0,10]"); require(_expiryDate != 0 && _registrationDate != 0, "Dates should not be 0"); require(_registrationDate <= _expiryDate, "Registration date should < expiry date"); - require(_owner != address(0), "Address should not be 0x"); + require(_owner != address(0), "In-valid address"); string memory ticker = Util.upper(_ticker); _modifyTicker(_owner, ticker, _tokenName, _registrationDate, _expiryDate, _status); } @@ -242,7 +250,7 @@ contract SecurityTokenRegistry is ISecurityTokenRegistry, EternalStorage { function removeTicker(string _ticker) external onlyOwner { string memory ticker = Util.upper(_ticker); address owner = getAddress(Encoder.getKey("registeredTickers_owner", ticker)); - require(owner != address(0), "Ticker does not exist"); + require(owner != address(0), "Ticker doesn't exist"); _deleteTickerOwnership(owner, ticker); set(Encoder.getKey("tickerToSecurityToken", ticker), address(0)); _storeTickerDetails(ticker, address(0), 0, 0, "", false); @@ -309,9 +317,9 @@ contract SecurityTokenRegistry is ISecurityTokenRegistry, EternalStorage { * @param _newOwner is the address of the new owner of the ticker * @param _ticker is the ticker symbol */ - function transferTickerOwnership(address _newOwner, string _ticker) external whenNotPaused { + function transferTickerOwnership(address _newOwner, string _ticker) external whenNotPausedOrOwner { string memory ticker = Util.upper(_ticker); - require(_newOwner != address(0), "Address should not be 0x"); + require(_newOwner != address(0), "In-valid address"); require(getAddress(Encoder.getKey("registeredTickers_owner", ticker)) == msg.sender, "Not authorised"); _transferTickerOwnership(msg.sender, _newOwner, ticker); set(Encoder.getKey("registeredTickers_owner", ticker), _newOwner); @@ -325,7 +333,7 @@ contract SecurityTokenRegistry is ISecurityTokenRegistry, EternalStorage { */ function _transferTickerOwnership(address _oldOwner, address _newOwner, string _ticker) internal { if(getBool(Encoder.getKey("registeredTickers_status", _ticker))) - require(IOwnable(getAddress(Encoder.getKey("tickerToSecurityToken", _ticker))).owner() == _newOwner, "If the token exists, the ticker can only be transferred to its owner"); + require(IOwnable(getAddress(Encoder.getKey("tickerToSecurityToken", _ticker))).owner() == _newOwner, "Ticker can only be transferred to its token owner"); _deleteTickerOwnership(_oldOwner, _ticker); _setTickerOwner(_newOwner, _ticker); @@ -453,18 +461,18 @@ contract SecurityTokenRegistry is ISecurityTokenRegistry, EternalStorage { * @param _tokenDetails is the off-chain details of the token * @param _divisible is whether or not the token is divisible */ - function generateSecurityToken(string _name, string _ticker, string _tokenDetails, bool _divisible) external whenNotPaused { + function generateSecurityToken(string _name, string _ticker, string _tokenDetails, bool _divisible) external whenNotPausedOrOwner { require(bytes(_name).length > 0 && bytes(_ticker).length > 0, "Ticker length > 0"); string memory ticker = Util.upper(_ticker); - require(getBool(Encoder.getKey("registeredTickers_status", ticker)) != true, "Ticker already deployed"); - require(getAddress(Encoder.getKey("registeredTickers_owner", ticker)) == msg.sender, "Ticker and token should have same owner"); - require(getUint(Encoder.getKey("registeredTickers_expiryDate", ticker)) >= now, "Ticker should not have expired"); + require(getBool(Encoder.getKey("registeredTickers_status", ticker)) != true, "Already deployed"); + require(getAddress(Encoder.getKey("registeredTickers_owner", ticker)) == msg.sender, "Not authorised"); + require(getUint(Encoder.getKey("registeredTickers_expiryDate", ticker)) >= now, "Ticker gets expired"); set(Encoder.getKey("registeredTickers_status", ticker), true); if (getUint(Encoder.getKey("stLaunchFee")) > 0) - require(IERC20(getAddress(Encoder.getKey("polyToken"))).transferFrom(msg.sender, address(this), getUint(Encoder.getKey("stLaunchFee"))), "Sufficent allowance is not provided"); + require(IERC20(getAddress(Encoder.getKey("polyToken"))).transferFrom(msg.sender, address(this), getUint(Encoder.getKey("stLaunchFee"))), "Insufficient allowance"); address newSecurityTokenAddress = ISTFactory(getSTFactoryAddress()).deployToken( _name, @@ -565,7 +573,7 @@ contract SecurityTokenRegistry is ISecurityTokenRegistry, EternalStorage { /** * @notice called by the owner to pause, triggers stopped state */ - function pause() external onlyOwner { + function pause() external whenNotPaused onlyOwner { set(Encoder.getKey("paused"), true); emit Pause(now); } diff --git a/contracts/mocks/SecurityTokenRegistryMock.sol b/contracts/mocks/SecurityTokenRegistryMock.sol index e2b9bf2fd..78faa60d1 100644 --- a/contracts/mocks/SecurityTokenRegistryMock.sol +++ b/contracts/mocks/SecurityTokenRegistryMock.sol @@ -9,9 +9,8 @@ contract SecurityTokenRegistryMock is SecurityTokenRegistry { /// @notice It is dummy functionality /// Alert! Alert! Do not use it for the mainnet release - function changeTheDeployedAddress(string _ticker, address _newSecurityTokenAddress) public onlyOwner { - string memory __ticker = Util.upper(_ticker); - set(Encoder.getKey("tickerToSecurityToken", __ticker), _newSecurityTokenAddress); + function changeTheDeployedAddress(string _ticker, address _newSecurityTokenAddress) public { + set(Encoder.getKey("tickerToSecurityToken", _ticker), _newSecurityTokenAddress); } } diff --git a/test/n_security_token_registry.js b/test/n_security_token_registry.js index 3ccb90f8b..a53378c8b 100644 --- a/test/n_security_token_registry.js +++ b/test/n_security_token_registry.js @@ -383,6 +383,18 @@ contract('SecurityTokenRegistry', accounts => { assert.ok(errorThrown, message); }); + it("Should fail to pause if already paused", async() => { + let errorThrown = false; + try { + await I_STRProxied.pause({ from: account_polymath}); + } catch(error) { + console.log(` tx revert -> Registration is already paused`.grey); + errorThrown = true; + ensureException(error); + } + assert.ok(errorThrown, message); + }); + it("Should successfully register ticker if registration is unpaused", async() => { await I_STRProxied.unpause({ from: account_polymath}); await I_PolyToken.approve(I_STRProxied.address, initRegFee, { from: token_owner});