From a3b40b08adfda522e3565283c4ba5ef6ec353c0e Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Fri, 24 May 2019 06:15:24 -0400 Subject: [PATCH 1/5] Added constructors The constructors prevents an exploit in case we forget to initialize the implementation contracts. Ideally, we should allways remember to initialize implementation contracts as well. --- contracts/SecurityTokenRegistry.sol | 5 +++++ contracts/tokens/SecurityToken.sol | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/contracts/SecurityTokenRegistry.sol b/contracts/SecurityTokenRegistry.sol index f16baab30..4c9a104f3 100644 --- a/contracts/SecurityTokenRegistry.sol +++ b/contracts/SecurityTokenRegistry.sol @@ -195,6 +195,11 @@ contract SecurityTokenRegistry is EternalStorage, Proxy { // Initialization ///////////////////////////// + // Constructor + constructor() public { + set(INITIALIZE, true); + } + /** * @notice Initializes instance of STR * @param _polymathRegistry is the address of the Polymath Registry diff --git a/contracts/tokens/SecurityToken.sol b/contracts/tokens/SecurityToken.sol index 65ab132d9..c9c22ca95 100644 --- a/contracts/tokens/SecurityToken.sol +++ b/contracts/tokens/SecurityToken.sol @@ -72,6 +72,11 @@ contract SecurityToken is ERC20, ReentrancyGuard, SecurityTokenStorage, IERC1594 // Emit when the budget allocated to a module is changed event ModuleBudgetChanged(uint8[] _moduleTypes, address _module, uint256 _oldBudget, uint256 _budget); //Event emitted by the tokenLib. + // Constructor + constructor() public { + initialized = true; + } + /** * @notice Initialization function * @dev Expected to be called atomically with the proxy being created, by the owner of the token From b9b42b8e2695306f2b83264f7b5f7553bd1ce736 Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Fri, 24 May 2019 06:34:57 -0400 Subject: [PATCH 2/5] Migration fixed --- migrations/2_deploy_contracts.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migrations/2_deploy_contracts.js b/migrations/2_deploy_contracts.js index a7821aa43..28c63a8aa 100644 --- a/migrations/2_deploy_contracts.js +++ b/migrations/2_deploy_contracts.js @@ -305,7 +305,7 @@ module.exports = function(deployer, network, accounts) { }) .then(() => { // B) Deploy the SecurityTokenLogic Contract - return deployer.deploy(SecurityTokenLogic, "", "", 0, { from: PolymathAccount }); + return deployer.deploy(SecurityTokenLogic, { from: PolymathAccount }); }) .then(() => { // B) Deploy the DataStoreFactory Contract From 351195e5c9c6650a38812bf485b4d7d23e8edfdd Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Fri, 24 May 2019 06:46:31 -0400 Subject: [PATCH 3/5] test fix --- test/helpers/createInstances.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/helpers/createInstances.js b/test/helpers/createInstances.js index 1687b8b65..597a33fd9 100644 --- a/test/helpers/createInstances.js +++ b/test/helpers/createInstances.js @@ -248,7 +248,7 @@ async function deployGTM(account_polymath) { async function deploySTFactory(account_polymath) { I_STGetter = await STGetter.new({from: account_polymath}); - I_SecurityToken = await SecurityTokenLogic.new("", "", 0, {from: account_polymath}); + I_SecurityToken = await SecurityTokenLogic.new({ from: account_polymath }); console.log("STL - " + I_SecurityToken.address); let I_DataStoreLogic = await DataStoreLogic.new({ from: account_polymath }); let I_DataStoreFactory = await DataStoreFactory.new(I_DataStoreLogic.address, { from: account_polymath }); From 825dd4f884d555cd38852e35871062f4a76f7692 Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Fri, 24 May 2019 16:33:35 +0530 Subject: [PATCH 4/5] test fix --- test/u_module_registry_proxy.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/u_module_registry_proxy.js b/test/u_module_registry_proxy.js index e3447bf27..4a198745d 100644 --- a/test/u_module_registry_proxy.js +++ b/test/u_module_registry_proxy.js @@ -140,9 +140,6 @@ contract("ModuleRegistryProxy", async (accounts) => { ); let I_SecurityTokenLogic = await SecurityToken.new( - "", - "", - 0, { from: account_polymath } ); From a5b591f0bf1c8905c221d3a5230b68519f27a974 Mon Sep 17 00:00:00 2001 From: Adam Dossa Date: Tue, 4 Jun 2019 19:14:15 -0400 Subject: [PATCH 5/5] Add the ability to configure the new STR atomically --- contracts/SecurityTokenRegistry.sol | 7 +++- contracts/mocks/MockSTRGetter.sol | 16 ++++++++ contracts/mocks/SecurityTokenRegistryMock.sol | 12 ++++++ test/helpers/encodeCall.js | 6 +++ test/n_security_token_registry.js | 14 ++++++- test/t_security_token_registry_proxy.js | 39 ++++++++++++++++++- 6 files changed, 90 insertions(+), 4 deletions(-) create mode 100644 contracts/mocks/MockSTRGetter.sol diff --git a/contracts/SecurityTokenRegistry.sol b/contracts/SecurityTokenRegistry.sol index 4c9a104f3..d52fc98d3 100644 --- a/contracts/SecurityTokenRegistry.sol +++ b/contracts/SecurityTokenRegistry.sol @@ -162,6 +162,11 @@ contract SecurityTokenRegistry is EternalStorage, Proxy { _; } + modifier onlyOwnerOrSelf() { + require(msg.sender == owner() || msg.sender == address(this), "Only owner or self"); + _; + } + /** * @notice Modifier to make a function callable only when the contract is not paused. */ @@ -293,7 +298,7 @@ contract SecurityTokenRegistry is EternalStorage, Proxy { * @notice Set the getter contract address * @param _getterContract Address of the contract */ - function setGetterRegistry(address _getterContract) public onlyOwner { + function setGetterRegistry(address _getterContract) public onlyOwnerOrSelf { require(_getterContract != address(0)); set(STRGETTER, _getterContract); } diff --git a/contracts/mocks/MockSTRGetter.sol b/contracts/mocks/MockSTRGetter.sol new file mode 100644 index 000000000..464a73847 --- /dev/null +++ b/contracts/mocks/MockSTRGetter.sol @@ -0,0 +1,16 @@ +pragma solidity ^0.5.0; + +import "../STRGetter.sol"; + +/** + * @title Registry contract for issuers to register their security tokens + */ +contract MockSTRGetter is STRGetter { + /// @notice It is a dummy function + /// Alert! Alert! Do NOT use it for the mainnet release + + function newFunction() public pure returns (uint256) { + return 99; + } + +} diff --git a/contracts/mocks/SecurityTokenRegistryMock.sol b/contracts/mocks/SecurityTokenRegistryMock.sol index 2ec9d8edd..e4d5e6ca5 100644 --- a/contracts/mocks/SecurityTokenRegistryMock.sol +++ b/contracts/mocks/SecurityTokenRegistryMock.sol @@ -8,8 +8,20 @@ import "../SecurityTokenRegistry.sol"; contract SecurityTokenRegistryMock is SecurityTokenRegistry { /// @notice It is a dummy function /// Alert! Alert! Do NOT use it for the mainnet release + + modifier onlyOwnerOrSelf() { + require(msg.sender == owner() || msg.sender == address(this), "Only owner or self"); + _; + } + + uint256 public someValue; + function changeTheDeployedAddress(string memory _ticker, address _newSecurityTokenAddress) public { set(Encoder.getKey("tickerToSecurityToken", _ticker), _newSecurityTokenAddress); } + function configure(uint256 _someValue) public onlyOwnerOrSelf { + someValue = _someValue; + } + } diff --git a/test/helpers/encodeCall.js b/test/helpers/encodeCall.js index 7aaa9bbf8..a1a58056b 100644 --- a/test/helpers/encodeCall.js +++ b/test/helpers/encodeCall.js @@ -11,3 +11,9 @@ export function encodeModuleCall(parametersType, values) { const params = abi.rawEncode(parametersType, values).toString("hex"); return "0x" + methodId + params; } + +export function encodeCall(methodName, parametersType, values) { + const methodId = abi.methodID(methodName, parametersType).toString("hex"); + const params = abi.rawEncode(parametersType, values).toString("hex"); + return "0x" + methodId + params; +} diff --git a/test/n_security_token_registry.js b/test/n_security_token_registry.js index 2629f098c..c3c088549 100644 --- a/test/n_security_token_registry.js +++ b/test/n_security_token_registry.js @@ -731,14 +731,24 @@ contract("SecurityTokenRegistry", async (accounts) => { it("Should fail to upgrade the logic contract of the STRProxy -- bad owner", async () => { await I_STRProxied.pause({ from: account_polymath }); + const STRProxyConfigParameters = ["uint256"]; + let bytesProxy = encodeModuleCall(STRProxyConfigParameters, [ + 99 + ]); + await catchRevert( - I_SecurityTokenRegistryProxy.upgradeTo("1.1.0", I_SecurityTokenRegistryV2.address, { from: account_temp }), + I_SecurityTokenRegistryProxy.upgradeToAndCall("1.1.0", I_SecurityTokenRegistryV2.address, bytesProxy, { from: account_temp }), "tx revert -> bad owner" ); }); it("Should upgrade the logic contract into the STRProxy", async () => { - await I_SecurityTokenRegistryProxy.upgradeTo("1.1.0", I_SecurityTokenRegistryV2.address, { from: account_polymath }); + const STRProxyConfigParameters = ["uint256"]; + let bytesProxy = encodeModuleCall(STRProxyConfigParameters, [ + 99 + ]); + + await I_SecurityTokenRegistryProxy.upgradeToAndCall("1.1.0", I_SecurityTokenRegistryV2.address, bytesProxy, { from: account_polymath }); I_STRProxied = await SecurityTokenRegistry.at(I_SecurityTokenRegistryProxy.address); assert.isTrue(await I_STRProxied.getBoolValue.call(web3.utils.soliditySha3("paused")), "Paused value should be false"); }); diff --git a/test/t_security_token_registry_proxy.js b/test/t_security_token_registry_proxy.js index 4a473b5cc..5e8377379 100644 --- a/test/t_security_token_registry_proxy.js +++ b/test/t_security_token_registry_proxy.js @@ -1,11 +1,13 @@ import { duration, promisifyLogWatch, latestBlock } from "./helpers/utils"; -import { encodeProxyCall } from "./helpers/encodeCall"; +import { encodeProxyCall, encodeCall } from "./helpers/encodeCall"; +import { takeSnapshot, increaseTime, revertToSnapshot } from "./helpers/time"; import { catchRevert } from "./helpers/exceptions"; import { setUpPolymathNetwork } from "./helpers/createInstances"; const SecurityTokenRegistry = artifacts.require("./SecurityTokenRegistry.sol"); const SecurityTokenRegistryProxy = artifacts.require("./SecurityTokenRegistryProxy.sol"); const SecurityTokenRegistryMock = artifacts.require("./SecurityTokenRegistryMock.sol"); +const MockSTRGetter = artifacts.require("./MockSTRGetter.sol"); const OwnedUpgradeabilityProxy = artifacts.require("./OwnedUpgradeabilityProxy.sol"); const STFactory = artifacts.require("./STFactory.sol"); const SecurityToken = artifacts.require("./SecurityToken.sol"); @@ -149,6 +151,41 @@ contract("SecurityTokenRegistryProxy", async (accounts) => { web3.utils.toWei("250") ); }); + + it("Upgrade the proxy again and change getter", async () => { + let snapId = await takeSnapshot(); + const I_MockSTRGetter = await MockSTRGetter.new({from: account_polymath}); + const I_MockSecurityTokenRegistry = await SecurityTokenRegistry.new({ from: account_polymath }); + const bytesProxy = encodeCall("setGetterRegistry", ["address"], [I_MockSTRGetter.address]); + console.log("Getter: " + I_MockSTRGetter.address); + console.log("Registry: " + I_MockSecurityTokenRegistry.address); + console.log("STRProxy: " + I_SecurityTokenRegistryProxy.address); + + await I_SecurityTokenRegistryProxy.upgradeToAndCall("2.0.0", I_MockSecurityTokenRegistry.address, bytesProxy, { + from: account_polymath + }); + + let c = await OwnedUpgradeabilityProxy.at(I_SecurityTokenRegistryProxy.address); + assert.equal(await readStorage(c.address, 12), I_MockSecurityTokenRegistry.address.toLowerCase()); + assert.equal( + web3.utils + .toAscii(await readStorage(c.address, 11)) + .replace(/\u0000/g, "") + .replace(/\n/, ""), + "2.0.0" + ); + + const I_MockSecurityTokenRegistryProxy = await SecurityTokenRegistry.at(I_SecurityTokenRegistryProxy.address); + const I_MockSTRGetterProxy = await MockSTRGetter.at(I_SecurityTokenRegistryProxy.address); + await I_MockSecurityTokenRegistryProxy.setProtocolFactory(I_STFactory.address, 3, 1, 0); + await I_MockSecurityTokenRegistryProxy.setLatestVersion(3, 1, 0); + let newValue = await I_MockSTRGetterProxy.newFunction.call(); + assert.equal(newValue.toNumber(), 99); + //assert.isTrue(false); + await revertToSnapshot(snapId); + }); + + }); describe("Feed some data in storage", async () => {