From 8e2b1e400dc27bf1a886a0cbda7bd691a428eb63 Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Wed, 13 Mar 2019 17:44:56 +0530 Subject: [PATCH 01/10] Require signed user ack --- contracts/tokens/SecurityToken.sol | 27 ++++++++++++----------- contracts/tokens/SecurityTokenStorage.sol | 9 ++++---- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/contracts/tokens/SecurityToken.sol b/contracts/tokens/SecurityToken.sol index ba9ffc719..eceff5305 100644 --- a/contracts/tokens/SecurityToken.sol +++ b/contracts/tokens/SecurityToken.sol @@ -12,12 +12,12 @@ import "../interfaces/token/IERC1594.sol"; import "../interfaces/token/IERC1643.sol"; import "../interfaces/token/IERC1644.sol"; import "../interfaces/IModuleRegistry.sol"; -import "../interfaces/IFeatureRegistry.sol"; import "../interfaces/ITransferManager.sol"; import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; import "openzeppelin-solidity/contracts/utils/ReentrancyGuard.sol"; import "openzeppelin-solidity/contracts/token/ERC20/ERC20.sol"; import "openzeppelin-solidity/contracts/token/ERC20/ERC20Detailed.sol"; +import "openzeppelin-solidity/contracts/cryptography/ECDSA.sol"; /** * @title Security Token contract @@ -32,6 +32,7 @@ import "openzeppelin-solidity/contracts/token/ERC20/ERC20Detailed.sol"; contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, SecurityTokenStorage, IERC1594, IERC1643, IERC1644, Proxy { using SafeMath for uint256; + using ECDSA for bytes32; // Emit at the time when module get added event ModuleAdded( @@ -115,11 +116,6 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi _; } - modifier isEnabled(string memory _nameKey) { - require(IFeatureRegistry(featureRegistry).getFeatureStatus(_nameKey)); - _; - } - /** * @notice constructor * @param _name Name of the SecurityToken @@ -141,7 +137,7 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi ) public ERC20Detailed(_name, _symbol, _decimals) - { + { _zeroAddressCheck(_polymathRegistry); _zeroAddressCheck(_delegate); polymathRegistry = _polymathRegistry; @@ -298,7 +294,7 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi /** * @notice Allows to change the treasury wallet address - * @param _wallet Ethereum address of the treasury wallet + * @param _wallet Ethereum address of the treasury wallet */ function changeTreasuryWallet(address _wallet) external onlyOwner { _zeroAddressCheck(_wallet); @@ -485,7 +481,10 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi * @notice Permanently freeze issuance of this security token. * @dev It MUST NOT be possible to increase `totalSuppy` after this function is called. */ - function freezeIssuance() external isIssuanceAllowed isEnabled("freezeIssuanceAllowed") onlyOwner { + function freezeIssuance(bytes calldata _signature) external onlyOwner { + // keccack256("I acknowledge that freezing Issuance is a permanent and irrevocable change"); + bytes32 hash = 0x3cebd417af2bef7b3e07b77448684b22aedaf0f43dbc106b1eda6599190f6a6d; + require(owner() == hash.toEthSignedMessageHash().recover(_signature), "Owner did not sign"); issuance = false; /*solium-disable-next-line security/no-block-members*/ emit FreezeIssuance(); @@ -507,8 +506,8 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi ) public isIssuanceAllowed - { - _onlyModuleOrOwner(MINT_KEY); + { + _onlyModuleOrOwner(MINT_KEY); // Add a function to validate the `_data` parameter _isValidTransfer(_updateTransfer(address(0), _tokenHolder, _value, _data)); _mint(_tokenHolder, _value); @@ -603,7 +602,10 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi * @notice Used by the issuer to permanently disable controller functionality * @dev enabled via feature switch "disableControllerAllowed" */ - function disableController() external isEnabled("disableControllerAllowed") onlyOwner { + function disableController(bytes calldata _signature) external onlyOwner { + // keccack256("I acknowledge that disabling controller is a permanent and irrevocable change"); + bytes32 hash = 0x6c33f5d82a4088dba7f7969350798c7a2900b5a3689f123d0630d513d05e5611; + require(owner() == hash.toEthSignedMessageHash().recover(_signature), "Owner did not sign"); require(_isControllable()); controllerDisabled = true; delete controller; @@ -766,7 +768,6 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi function updateFromRegistry() public onlyOwner { moduleRegistry = PolymathRegistry(polymathRegistry).getAddress("ModuleRegistry"); securityTokenRegistry = PolymathRegistry(polymathRegistry).getAddress("SecurityTokenRegistry"); - featureRegistry = PolymathRegistry(polymathRegistry).getAddress("FeatureRegistry"); polyToken = PolymathRegistry(polymathRegistry).getAddress("PolyToken"); } } diff --git a/contracts/tokens/SecurityTokenStorage.sol b/contracts/tokens/SecurityTokenStorage.sol index ba958eac8..4b33047cc 100644 --- a/contracts/tokens/SecurityTokenStorage.sol +++ b/contracts/tokens/SecurityTokenStorage.sol @@ -10,7 +10,7 @@ contract SecurityTokenStorage { uint8 constant DATA_KEY = 6; bytes32 internal constant INVESTORSKEY = 0xdf3a8dd24acdd05addfc6aeffef7574d2de3f844535ec91e8e0f3e45dba96731; //keccak256(abi.encodePacked("INVESTORS")) bytes32 internal constant TREASURY = 0xaae8817359f3dcb67d050f44f3e49f982e0359d90ca4b5f18569926304aaece6; //keccak256(abi.encodePacked("TREASURY_WALLET")) - + ////////////////////////// /// Document datastructure ////////////////////////// @@ -53,7 +53,6 @@ contract SecurityTokenStorage { address public polymathRegistry; address public moduleRegistry; address public securityTokenRegistry; - address public featureRegistry; address public polyToken; address public delegate; // Address of the data store used to store shared data @@ -79,12 +78,12 @@ contract SecurityTokenStorage { // Variable which tells whether issuance is ON or OFF forever // Implementers need to implement one more function to reset the value of `issuance` variable // to false. That function is not a part of the standard (EIP-1594) as it is depend on the various factors - // issuer, followed compliance rules etc. So issuers have the choice how they want to close the issuance. + // issuer, followed compliance rules etc. So issuers have the choice how they want to close the issuance. bool internal issuance = true; // Array use to store all the document name present in the contracts bytes32[] _docNames; - + // Times at which each checkpoint was created uint256[] checkpointTimes; @@ -110,4 +109,4 @@ contract SecurityTokenStorage { // mapping to store the document name indexes mapping(bytes32 => uint256) internal _docIndexes; -} \ No newline at end of file +} From 3987327e4513d082ffd00345f8034606f9f7c4ba Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Wed, 13 Mar 2019 18:20:05 +0530 Subject: [PATCH 02/10] Added test --- test/o_security_token.js | 62 +++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/test/o_security_token.js b/test/o_security_token.js index ed83ce50c..d2f93d08f 100644 --- a/test/o_security_token.js +++ b/test/o_security_token.js @@ -2,6 +2,7 @@ import latestTime from "./helpers/latestTime"; import { duration, ensureException, promisifyLogWatch, latestBlock } from "./helpers/utils"; import { takeSnapshot, increaseTime, revertToSnapshot } from "./helpers/time"; import { encodeProxyCall, encodeModuleCall } from "./helpers/encodeCall"; +import { pk } from "./helpers/testprivateKey"; import { catchRevert } from "./helpers/exceptions"; import { setUpPolymathNetwork, @@ -29,6 +30,9 @@ contract("SecurityToken", async (accounts) => { let account_investor1; let account_issuer; let token_owner; + let token_owner_pk; + let disableControllerAckHash; + let freezeIssuanceAckHash; let account_investor2; let account_investor3; let account_affiliate1; @@ -130,6 +134,10 @@ contract("SecurityToken", async (accounts) => { account_investor1 = accounts[9]; token_owner = account_issuer; + token_owner_pk = pk.account_1; + disableControllerAckHash = "0x6c33f5d82a4088dba7f7969350798c7a2900b5a3689f123d0630d513d05e5611"; //without prefix + freezeIssuanceAckHash = "0xfb587c87f76761d656905891c2a6b65a7e364703852e964d8e914be92d02fa28"; //with prefix + account_controller = account_temp; // Step:1 Create the polymath ecosystem contract instances @@ -300,30 +308,21 @@ contract("SecurityToken", async (accounts) => { assert.isTrue(await I_SecurityToken.isIssuable.call()); }) - it("Should finish the minting -- fail because feature is not activated", async () => { - await catchRevert(I_SecurityToken.freezeIssuance({ from: token_owner })); - }); - - it("Should finish the minting -- fail to activate the feature because msg.sender is not polymath", async () => { - await catchRevert(I_FeatureRegistry.setFeatureStatus("freezeIssuanceAllowed", true, { from: token_owner })); - }); - - it("Should finish the minting -- successfully activate the feature", async () => { - await catchRevert(I_FeatureRegistry.setFeatureStatus("freezeIssuanceAllowed", false, { from: account_polymath })); - assert.equal(false, await I_FeatureRegistry.getFeatureStatus("freezeIssuanceAllowed", { from: account_temp })); - await I_FeatureRegistry.setFeatureStatus("freezeIssuanceAllowed", true, { from: account_polymath }); - assert.equal(true, await I_FeatureRegistry.getFeatureStatus("freezeIssuanceAllowed", { from: account_temp })); - - await catchRevert(I_FeatureRegistry.setFeatureStatus("freezeIssuanceAllowed", true, { from: account_polymath })); + it("Should finish the minting -- fail because owner didn't sign correct acknowledegement", async () => { + let signature = (web3.eth.accounts.sign("F O'Brien is the best", token_owner_pk)).signature; + await catchRevert(I_SecurityToken.freezeIssuance(signature, { from: token_owner })); }); it("Should finish the minting -- fail because msg.sender is not the owner", async () => { - await catchRevert(I_SecurityToken.freezeIssuance({ from: account_temp })); + let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, token_owner_pk)).signature; + await catchRevert(I_SecurityToken.freezeIssuance(signature, { from: account_temp })); }); it("Should finish minting & restrict the further minting", async () => { let id = await takeSnapshot(); - await I_SecurityToken.freezeIssuance({ from: token_owner }); + let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, token_owner_pk)).signature; + console.log(signature); + await I_SecurityToken.freezeIssuance(signature, { from: token_owner }); assert.isFalse(await I_SecurityToken.isIssuable.call()); await catchRevert(I_SecurityToken.issue(account_affiliate1, new BN(100).mul(new BN(10).pow(new BN(18))), "0x0", { from: token_owner, gas: 500000 })); await revertToSnapshot(id); @@ -392,7 +391,8 @@ contract("SecurityToken", async (accounts) => { it("Should fail to issue tokens while STO attached after freezeMinting called", async () => { let id = await takeSnapshot(); - await I_SecurityToken.freezeIssuance({ from: token_owner }); + let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, token_owner_pk)).signature; + await I_SecurityToken.freezeIssuance(signature, { from: token_owner }); await catchRevert(I_SecurityToken.issue(account_affiliate1, new BN(100).mul(new BN(10).pow(new BN(18))), "0x0", { from: token_owner })); await revertToSnapshot(id); @@ -819,7 +819,8 @@ contract("SecurityToken", async (accounts) => { it("STO should fail to issue tokens after minting is frozen", async () => { let id = await takeSnapshot(); - await I_SecurityToken.freezeIssuance({ from: token_owner }); + let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, token_owner_pk)).signature; + await I_SecurityToken.freezeIssuance(signature, { from: token_owner }); await catchRevert( web3.eth.sendTransaction({ @@ -1217,23 +1218,19 @@ contract("SecurityToken", async (accounts) => { assert.equal(new BN(web3.utils.toWei("10", "ether")).toString(), eventTransfer.args.value.toString(), "Event not emitted as expected"); }); - it("Should fail to freeze controller functionality because not owner", async () => { - await catchRevert(I_SecurityToken.disableController({ from: account_investor1 })); + it("Should fail to freeze controller functionality because not owner", async () => { + let signature = (web3.eth.accounts.sign(disableControllerAckHash, token_owner_pk)).signature; + await catchRevert(I_SecurityToken.disableController(signature, { from: account_investor1 })); }); - it("Should fail to freeze controller functionality because disableControllerAllowed not activated", async () => { - await catchRevert(I_SecurityToken.disableController({ from: token_owner })); + it("Should fail to freeze controller functionality because proper acknowledgement not signed by owner", async () => { + let signature = (web3.eth.accounts.sign("He truely is", token_owner_pk)).signature; + await catchRevert(I_SecurityToken.disableController(signature, { from: token_owner })); }); it("Should successfully freeze controller functionality", async () => { - let tx1 = await I_FeatureRegistry.setFeatureStatus("disableControllerAllowed", true, { from: account_polymath }); - - // check event - assert.equal("disableControllerAllowed", tx1.logs[0].args._nameKey, "Event not emitted as expected"); - assert.equal(true, tx1.logs[0].args._newStatus, "Event not emitted as expected"); - - let tx2 = await I_SecurityToken.disableController({ from: token_owner }); - + let signature = (web3.eth.accounts.sign(disableControllerAckHash, token_owner_pk)).signature; + await I_SecurityToken.disableController(signature, { from: token_owner }); // check state assert.equal(address_zero, await I_SecurityToken.controller.call(), "State not changed"); assert.equal(true, await I_SecurityToken.controllerDisabled.call(), "State not changed"); @@ -1244,7 +1241,8 @@ contract("SecurityToken", async (accounts) => { }); it("Should fail to freeze controller functionality because already frozen", async () => { - await catchRevert(I_SecurityToken.disableController({ from: token_owner })); + let signature = (web3.eth.accounts.sign(disableControllerAckHash, token_owner_pk)).signature; + await catchRevert(I_SecurityToken.disableController(signature, { from: token_owner })); }); it("Should fail to set controller because controller functionality frozen", async () => { From 1b98f86e2931c30359d021dde4a253d258d29fc9 Mon Sep 17 00:00:00 2001 From: Adam Dossa Date: Wed, 13 Mar 2019 15:29:47 -0400 Subject: [PATCH 03/10] Small fix --- test/o_security_token.js | 81 ++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 45 deletions(-) diff --git a/test/o_security_token.js b/test/o_security_token.js index d2f93d08f..3039d51be 100644 --- a/test/o_security_token.js +++ b/test/o_security_token.js @@ -136,7 +136,7 @@ contract("SecurityToken", async (accounts) => { token_owner = account_issuer; token_owner_pk = pk.account_1; disableControllerAckHash = "0x6c33f5d82a4088dba7f7969350798c7a2900b5a3689f123d0630d513d05e5611"; //without prefix - freezeIssuanceAckHash = "0xfb587c87f76761d656905891c2a6b65a7e364703852e964d8e914be92d02fa28"; //with prefix + freezeIssuanceAckHash = "0x3cebd417af2bef7b3e07b77448684b22aedaf0f43dbc106b1eda6599190f6a6d"; //with prefix account_controller = account_temp; @@ -301,7 +301,7 @@ contract("SecurityToken", async (accounts) => { assert.equal(balance1.div(new BN(10).pow(new BN(18))).toNumber(), 200); let balance2 = await I_SecurityToken.balanceOf(account_affiliate2); assert.equal(balance2.div(new BN(10).pow(new BN(18))).toNumber(), 110); - + }); it("Should ST be issuable", async() => { @@ -309,22 +309,23 @@ contract("SecurityToken", async (accounts) => { }) it("Should finish the minting -- fail because owner didn't sign correct acknowledegement", async () => { - let signature = (web3.eth.accounts.sign("F O'Brien is the best", token_owner_pk)).signature; + let signature = (web3.eth.accounts.sign("F O'Brien is the best", "0x" + token_owner_pk)).signature; await catchRevert(I_SecurityToken.freezeIssuance(signature, { from: token_owner })); }); it("Should finish the minting -- fail because msg.sender is not the owner", async () => { - let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, token_owner_pk)).signature; + let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, "0x" + token_owner_pk)).signature; await catchRevert(I_SecurityToken.freezeIssuance(signature, { from: account_temp })); }); it("Should finish minting & restrict the further minting", async () => { let id = await takeSnapshot(); - let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, token_owner_pk)).signature; + let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, "0x" + token_owner_pk)).signature; console.log(signature); await I_SecurityToken.freezeIssuance(signature, { from: token_owner }); assert.isFalse(await I_SecurityToken.isIssuable.call()); await catchRevert(I_SecurityToken.issue(account_affiliate1, new BN(100).mul(new BN(10).pow(new BN(18))), "0x0", { from: token_owner, gas: 500000 })); + // assert.isTrue(false); await revertToSnapshot(id); }); @@ -384,14 +385,14 @@ contract("SecurityToken", async (accounts) => { it("Should successfully issue tokens while STO attached", async () => { await I_SecurityToken.issue(account_affiliate1, new BN(100).mul(new BN(10).pow(new BN(18))), "0x0", { from: token_owner }); - + let balance = await I_SecurityToken.balanceOf(account_affiliate1); assert.equal(balance.div(new BN(10).pow(new BN(18))).toNumber(), 300); }); it("Should fail to issue tokens while STO attached after freezeMinting called", async () => { let id = await takeSnapshot(); - let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, token_owner_pk)).signature; + let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, "0x" + token_owner_pk)).signature; await I_SecurityToken.freezeIssuance(signature, { from: token_owner }); await catchRevert(I_SecurityToken.issue(account_affiliate1, new BN(100).mul(new BN(10).pow(new BN(18))), "0x0", { from: token_owner })); @@ -463,13 +464,13 @@ contract("SecurityToken", async (accounts) => { assert.equal(tx.logs[0].args._module, I_GeneralTransferManager.address); await I_SecurityToken.issue(account_investor1, new BN(web3.utils.toWei("500")), "0x0", { from: token_owner }); let _canTransfer = await I_SecurityToken.canTransfer.call(account_investor2, new BN(web3.utils.toWei("200")), "0x0", {from: account_investor1}); - + assert.isTrue(_canTransfer[0]); assert.equal(_canTransfer[1], 0x51); assert.equal(_canTransfer[2], empty_hash); - + await I_SecurityToken.transfer(account_investor2, new BN(web3.utils.toWei("200")), { from: account_investor1 }); - + assert.equal((await I_SecurityToken.balanceOf(account_investor2)).div(new BN(10).pow(new BN(18))).toNumber(), 200); await revertToSnapshot(key); }); @@ -615,7 +616,7 @@ contract("SecurityToken", async (accounts) => { it("Should Fail in transferring the token from one whitelist investor 1 to non whitelist investor 2", async () => { let _canTransfer = await I_SecurityToken.canTransfer.call(account_investor2, new BN(10).mul(new BN(10).pow(new BN(18))), "0x0", {from: account_investor1}); - + assert.isFalse(_canTransfer[0]); assert.equal(_canTransfer[1], 0x50); @@ -643,7 +644,7 @@ contract("SecurityToken", async (accounts) => { it("Should activate allow All Transfer", async () => { ID_snap = await takeSnapshot(); await I_GeneralTransferManager.modifyTransferRequirementsMulti( - [0, 1, 2], + [0, 1, 2], [false, false, false], [false, false, false], [false, false, false], @@ -710,7 +711,7 @@ contract("SecurityToken", async (accounts) => { it("Should activate allow All Whitelist Transfers", async () => { ID_snap = await takeSnapshot(); await I_GeneralTransferManager.modifyTransferRequirementsMulti( - [0, 1, 2], + [0, 1, 2], [true, false, true], [true, true, false], [false, false, false], @@ -778,7 +779,7 @@ contract("SecurityToken", async (accounts) => { assert.equal(balance1.div(new BN(10).pow(new BN(18))).toNumber(), 500); let balance2 = await I_SecurityToken.balanceOf(account_affiliate2); assert.equal(balance2.div(new BN(10).pow(new BN(18))).toNumber(), 220); - + }); it("Should provide more permissions to the delegate", async () => { @@ -819,7 +820,7 @@ contract("SecurityToken", async (accounts) => { it("STO should fail to issue tokens after minting is frozen", async () => { let id = await takeSnapshot(); - let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, token_owner_pk)).signature; + let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, "0x" + token_owner_pk)).signature; await I_SecurityToken.freezeIssuance(signature, { from: token_owner }); await catchRevert( @@ -943,7 +944,7 @@ contract("SecurityToken", async (accounts) => { it("Should force burn the tokens - value too high", async () => { await I_GeneralTransferManager.modifyTransferRequirementsMulti( - [0, 1, 2], + [0, 1, 2], [true, false, false], [true, true, false], [true, false, false], @@ -1218,18 +1219,18 @@ contract("SecurityToken", async (accounts) => { assert.equal(new BN(web3.utils.toWei("10", "ether")).toString(), eventTransfer.args.value.toString(), "Event not emitted as expected"); }); - it("Should fail to freeze controller functionality because not owner", async () => { - let signature = (web3.eth.accounts.sign(disableControllerAckHash, token_owner_pk)).signature; + it("Should fail to freeze controller functionality because not owner", async () => { + let signature = (web3.eth.accounts.sign(disableControllerAckHash, "0x" + token_owner_pk)).signature; await catchRevert(I_SecurityToken.disableController(signature, { from: account_investor1 })); }); it("Should fail to freeze controller functionality because proper acknowledgement not signed by owner", async () => { - let signature = (web3.eth.accounts.sign("He truely is", token_owner_pk)).signature; + let signature = (web3.eth.accounts.sign("He truely is", "0x" + token_owner_pk)).signature; await catchRevert(I_SecurityToken.disableController(signature, { from: token_owner })); }); it("Should successfully freeze controller functionality", async () => { - let signature = (web3.eth.accounts.sign(disableControllerAckHash, token_owner_pk)).signature; + let signature = (web3.eth.accounts.sign(disableControllerAckHash, "0x" + token_owner_pk)).signature; await I_SecurityToken.disableController(signature, { from: token_owner }); // check state assert.equal(address_zero, await I_SecurityToken.controller.call(), "State not changed"); @@ -1241,7 +1242,7 @@ contract("SecurityToken", async (accounts) => { }); it("Should fail to freeze controller functionality because already frozen", async () => { - let signature = (web3.eth.accounts.sign(disableControllerAckHash, token_owner_pk)).signature; + let signature = (web3.eth.accounts.sign(disableControllerAckHash, "0x" + token_owner_pk)).signature; await catchRevert(I_SecurityToken.disableController(signature, { from: token_owner })); }); @@ -1398,78 +1399,68 @@ contract("SecurityToken", async (accounts) => { web3.utils.toChecksumAddress(await readStorage(I_SecurityToken.address, 10)) ); - console.log(` - FeatureRegistry address from the contract: ${await stGetter.featureRegistry.call()} - FeatureRegistry address from the storage: ${await readStorage(I_SecurityToken.address, 11)} - `) - - assert.equal( - await stGetter.featureRegistry.call(), - web3.utils.toChecksumAddress(await readStorage(I_SecurityToken.address, 11)) - ); - console.log(` PolyToken address from the contract: ${await stGetter.polyToken.call()} - PolyToken address from the storage: ${await readStorage(I_SecurityToken.address, 12)} + PolyToken address from the storage: ${await readStorage(I_SecurityToken.address, 11)} `) assert.equal( await stGetter.polyToken.call(), - web3.utils.toChecksumAddress(await readStorage(I_SecurityToken.address, 12)) + web3.utils.toChecksumAddress(await readStorage(I_SecurityToken.address, 11)) ); console.log(` Delegate address from the contract: ${await stGetter.delegate.call()} - Delegate address from the storage: ${await readStorage(I_SecurityToken.address, 13)} + Delegate address from the storage: ${await readStorage(I_SecurityToken.address, 12)} `) assert.equal( await stGetter.delegate.call(), - web3.utils.toChecksumAddress(await readStorage(I_SecurityToken.address, 13)) + web3.utils.toChecksumAddress(await readStorage(I_SecurityToken.address, 12)) ); console.log(` Datastore address from the contract: ${await stGetter.dataStore.call()} - Datastore address from the storage: ${await readStorage(I_SecurityToken.address, 14)} + Datastore address from the storage: ${await readStorage(I_SecurityToken.address, 13)} `) assert.equal( await stGetter.dataStore.call(), - web3.utils.toChecksumAddress(await readStorage(I_SecurityToken.address, 14)) + web3.utils.toChecksumAddress(await readStorage(I_SecurityToken.address, 13)) ); console.log(` Granularity value from the contract: ${await stGetter.granularity.call()} - Granularity value from the storage: ${(web3.utils.toBN(await readStorage(I_SecurityToken.address, 15))).toString()} + Granularity value from the storage: ${(web3.utils.toBN(await readStorage(I_SecurityToken.address, 14))).toString()} `) assert.equal( web3.utils.fromWei(await stGetter.granularity.call()), - web3.utils.fromWei((web3.utils.toBN(await readStorage(I_SecurityToken.address, 15))).toString()) + web3.utils.fromWei((web3.utils.toBN(await readStorage(I_SecurityToken.address, 14))).toString()) ); console.log(` Current checkpoint ID from the contract: ${await stGetter.currentCheckpointId.call()} - Current checkpoint ID from the storage: ${(web3.utils.toBN(await readStorage(I_SecurityToken.address, 16))).toString()} + Current checkpoint ID from the storage: ${(web3.utils.toBN(await readStorage(I_SecurityToken.address, 15))).toString()} `) assert.equal( await stGetter.currentCheckpointId.call(), - (web3.utils.toBN(await readStorage(I_SecurityToken.address, 16))).toString() + (web3.utils.toBN(await readStorage(I_SecurityToken.address, 15))).toString() ); console.log(` TokenDetails from the contract: ${await stGetter.tokenDetails.call()} - TokenDetails from the storage: ${(web3.utils.toUtf8((await readStorage(I_SecurityToken.address, 17)).substring(0, 60)))} + TokenDetails from the storage: ${(web3.utils.toUtf8((await readStorage(I_SecurityToken.address, 16)).substring(0, 60)))} `) assert.equal( await stGetter.tokenDetails.call(), - (web3.utils.toUtf8((await readStorage(I_SecurityToken.address, 17)).substring(0, 60))).replace(/\u0000/g, "") + (web3.utils.toUtf8((await readStorage(I_SecurityToken.address, 16)).substring(0, 60))).replace(/\u0000/g, "") ); }); }); - + describe(`Test cases for the ERC1643 contract\n`, async () => { describe(`Test cases for the setDocument() function of the ERC1643\n`, async() => { @@ -1561,7 +1552,7 @@ contract("SecurityToken", async (accounts) => { }); it("\tShould succssfully remove the document from the contract which is present in the last index of the `_docsName` and check the params of the `DocumentRemoved` event\n", async() => { - // first add the new document + // first add the new document await I_SecurityToken.setDocument(web3.utils.utf8ToHex("doc3"), "https://www.bts.l", "0x0", {from: token_owner}); // as this will be last in the array so remove this let tx = await I_SecurityToken.removeDocument(web3.utils.utf8ToHex("doc3"), {from: token_owner}); From bceac7e538ba7e54c405eb8a85814e3e36303a1f Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Thu, 14 Mar 2019 16:01:13 +0530 Subject: [PATCH 04/10] WIP --- contracts/libraries/TokenLib.sol | 92 ++++++++++++++++++++++++++++++ contracts/tokens/SecurityToken.sol | 15 +++-- package.json | 1 + test/helpers/signData.js | 89 ++++++++++++++++++++++++++++- test/o_security_token.js | 44 +++++++------- yarn.lock | 30 ++++++++++ 6 files changed, 241 insertions(+), 30 deletions(-) diff --git a/contracts/libraries/TokenLib.sol b/contracts/libraries/TokenLib.sol index 91135dac6..6b90ffb59 100644 --- a/contracts/libraries/TokenLib.sol +++ b/contracts/libraries/TokenLib.sol @@ -12,6 +12,24 @@ library TokenLib { using SafeMath for uint256; + struct EIP712Domain { + string name; + uint256 chainId; + address verifyingContract; + } + + struct Acknowledgment { + string text; + } + + bytes32 constant EIP712DOMAIN_TYPEHASH = keccak256( + "EIP712Domain(string name,uint256 chainId,address verifyingContract)" + ); + + bytes32 constant ACK_TYPEHASH = keccak256( + "Acknowledgment(string text)" + ); + bytes32 internal constant WHITELIST = "WHITELIST"; bytes32 internal constant INVESTORSKEY = 0xdf3a8dd24acdd05addfc6aeffef7574d2de3f844535ec91e8e0f3e45dba96731; //keccak256(abi.encodePacked("INVESTORS")) @@ -26,6 +44,80 @@ library TokenLib { // Emit when the budget allocated to a module is changed event ModuleBudgetChanged(uint8[] _moduleTypes, address _module, uint256 _oldBudget, uint256 _budget); + function hash(EIP712Domain memory _eip712Domain) internal pure returns (bytes32) { + return keccak256( + abi.encode( + EIP712DOMAIN_TYPEHASH, + keccak256(bytes(_eip712Domain.name)), + _eip712Domain.chainId, + _eip712Domain.verifyingContract + ) + ); + } + + function hash(Acknowledgment memory _ack) internal pure returns (bytes32) { + return keccak256(abi.encode(keccak256(bytes(_ack.text)))); + } + + function recoverFreezeIssuanceAckSigner(bytes memory _signature) public view returns (address) { + Acknowledgment memory ack = Acknowledgment("I acknowledge that freezing Issuance is a permanent and irrevocable change"); + return extractSigner(ack, _signature); + } + + function recoverDisableControllerAckSigner(bytes memory _signature) public view returns (address) { + Acknowledgment memory ack = Acknowledgment("I acknowledge that disabling controller is a permanent and irrevocable change"); + return extractSigner(ack, _signature); + } + + function extractSigner(Acknowledgment memory _ack, bytes memory _signature) internal view returns (address) { + bytes32 r; + bytes32 s; + uint8 v; + + // Check the signature length + if (_signature.length != 65) { + return (address(0)); + } + + // Divide the signature in r, s and v variables + // ecrecover takes the signature parameters, and the only way to get them + // currently is to use assembly. + // solhint-disable-next-line no-inline-assembly + assembly { + r := mload(add(_signature, 0x20)) + s := mload(add(_signature, 0x40)) + v := byte(0, mload(add(_signature, 0x60))) + } + + // Version of signature should be 27 or 28, but 0 and 1 are also possible versions + if (v < 27) { + v += 27; + } + + // If the version is correct return the signer address + if (v != 27 && v != 28) { + return (address(0)); + } + + bytes32 DOMAIN_SEPARATOR = hash( + EIP712Domain( + { + name: "Polymath", + chainId: 1, + verifyingContract: address(this) + } + ) + ); + + // Note: we need to use `encodePacked` here instead of `encode`. + bytes32 digest = keccak256(abi.encodePacked( + "\x19\x01", + DOMAIN_SEPARATOR, + hash(_ack) + )); + return ecrecover(digest, v, r, s); + } + /** * @notice Archives a module attached to the SecurityToken * @param _moduleData Storage data diff --git a/contracts/tokens/SecurityToken.sol b/contracts/tokens/SecurityToken.sol index eceff5305..976f86ce8 100644 --- a/contracts/tokens/SecurityToken.sol +++ b/contracts/tokens/SecurityToken.sol @@ -51,6 +51,8 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi event GranularityChanged(uint256 _oldGranularity, uint256 _newGranularity); // Emit when is permanently frozen by the issuer event FreezeIssuance(); + + event AddressEvent(address sig); // Emit when transfers are frozen or unfrozen event FreezeTransfers(bool _status); // Emit when Module get archived from the securityToken @@ -477,14 +479,17 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi return issuance; } + + /** * @notice Permanently freeze issuance of this security token. * @dev It MUST NOT be possible to increase `totalSuppy` after this function is called. */ function freezeIssuance(bytes calldata _signature) external onlyOwner { - // keccack256("I acknowledge that freezing Issuance is a permanent and irrevocable change"); - bytes32 hash = 0x3cebd417af2bef7b3e07b77448684b22aedaf0f43dbc106b1eda6599190f6a6d; - require(owner() == hash.toEthSignedMessageHash().recover(_signature), "Owner did not sign"); + emit AddressEvent(address(this)); + address ad = TokenLib.recoverFreezeIssuanceAckSigner(_signature); + emit AddressEvent(ad); + require(owner() == ad, "Owner did not sign"); issuance = false; /*solium-disable-next-line security/no-block-members*/ emit FreezeIssuance(); @@ -603,9 +608,7 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi * @dev enabled via feature switch "disableControllerAllowed" */ function disableController(bytes calldata _signature) external onlyOwner { - // keccack256("I acknowledge that disabling controller is a permanent and irrevocable change"); - bytes32 hash = 0x6c33f5d82a4088dba7f7969350798c7a2900b5a3689f123d0630d513d05e5611; - require(owner() == hash.toEthSignedMessageHash().recover(_signature), "Owner did not sign"); + require(owner() == TokenLib.recoverDisableControllerAckSigner(_signature), "Owner did not sign"); require(_isControllable()); controllerDisabled = true; delete controller; diff --git a/package.json b/package.json index 3cc06302e..a6315a6ff 100644 --- a/package.json +++ b/package.json @@ -57,6 +57,7 @@ }, "homepage": "https://github.com/PolymathNetwork/polymath-core#readme", "dependencies": { + "eth-sig-util": "^2.1.1", "truffle": "^5.0.4", "truffle-hdwallet-provider": "^1.0.4", "web3-provider-engine": "^14.1.0" diff --git a/test/helpers/signData.js b/test/helpers/signData.js index f0d0a09bb..399e45bf4 100644 --- a/test/helpers/signData.js +++ b/test/helpers/signData.js @@ -1,4 +1,5 @@ const Web3 = require("web3"); +const sigUtil = require('eth-sig-util') let BN = Web3.utils.BN; function getSignSTMData(tmAddress, nonce, validFrom, expiry, fromAddress, toAddress, amount, pk) { @@ -32,6 +33,90 @@ function getSignSTMData(tmAddress, nonce, validFrom, expiry, fromAddress, toAddr return data; } +async function getFreezeIssuanceAck(stAddress, from) { + const typedData = { + types: { + EIP712Domain: [ + { name: 'name', type: 'string' }, + { name: 'chainId', type: 'uint256' }, + { name: 'verifyingContract', type: 'address' } + ], + Acknowledgment: [ + { name: 'text', type: 'string' } + ], + }, + primaryType: 'Acknowledgment', + domain: { + name: 'Polymath', + chainId: 1, + verifyingContract: stAddress + }, + message: { + text: 'I acknowledge that freezing Issuance is a permanent and irrevocable change', + }, + }; + const result = await new Promise((resolve, reject) => { + web3.currentProvider.send( + { + method: 'eth_signTypedData', + params: [from, typedData] + }, + (err, result) => { + if (err) { + return reject(err); + } + resolve(result.result); + } + ); + }); + // console.log('signed by', from); + // const recovered = sigUtil.recoverTypedSignature({ + // data: typedData, + // sig: result + // }) + // console.log('recovered', recovered); + return result; +} + +async function getDisableControllerAck(stAddress, from) { + const typedData = { + types: { + EIP712Domain: [ + { name: 'name', type: 'string' }, + { name: 'chainId', type: 'uint256' }, + { name: 'verifyingContract', type: 'address' } + ], + Acknowledgment: [ + { name: 'text', type: 'string' } + ], + }, + primaryType: 'Acknowledgment', + domain: { + name: 'Polymath', + chainId: 1, + verifyingContract: stAddress + }, + message: { + text: 'I acknowledge that disabling controller is a permanent and irrevocable change', + }, + }; + const result = await new Promise((resolve, reject) => { + web3.currentProvider.send( + { + method: 'eth_signTypedData', + params: [from, typedData] + }, + (err, result) => { + if (err) { + return reject(err); + } + resolve(result.result); + } + ); + }); + return result; +} + function getSignGTMData(tmAddress, investorAddress, fromTime, toTime, expiryTime, validFrom, validTo, nonce, pk) { let hash = web3.utils.soliditySha3({ t: 'address', @@ -109,5 +194,7 @@ module.exports = { getSignSTMData, getSignGTMData, getSignGTMTransferData, - getMultiSignGTMData + getMultiSignGTMData, + getFreezeIssuanceAck, + getDisableControllerAck }; \ No newline at end of file diff --git a/test/o_security_token.js b/test/o_security_token.js index 3039d51be..75061b6a5 100644 --- a/test/o_security_token.js +++ b/test/o_security_token.js @@ -1,5 +1,6 @@ import latestTime from "./helpers/latestTime"; import { duration, ensureException, promisifyLogWatch, latestBlock } from "./helpers/utils"; +import { getFreezeIssuanceAck, getDisableControllerAck } from "./helpers/signData"; import { takeSnapshot, increaseTime, revertToSnapshot } from "./helpers/time"; import { encodeProxyCall, encodeModuleCall } from "./helpers/encodeCall"; import { pk } from "./helpers/testprivateKey"; @@ -135,8 +136,6 @@ contract("SecurityToken", async (accounts) => { token_owner = account_issuer; token_owner_pk = pk.account_1; - disableControllerAckHash = "0x6c33f5d82a4088dba7f7969350798c7a2900b5a3689f123d0630d513d05e5611"; //without prefix - freezeIssuanceAckHash = "0x3cebd417af2bef7b3e07b77448684b22aedaf0f43dbc106b1eda6599190f6a6d"; //with prefix account_controller = account_temp; @@ -203,6 +202,9 @@ contract("SecurityToken", async (accounts) => { assert.equal(await stGetter.getTreasuryWallet.call(), token_owner, "Incorrect wallet set") const log = (await I_SecurityToken.getPastEvents('ModuleAdded', {filter: {transactionHash: tx.transactionHash}}))[0]; + disableControllerAckHash = "0x6c33f5d82a4088dba7f7969350798c7a2900b5a3689f123d0630d513d05e5611"; + + // Verify that GeneralTransferManager module get added successfully or not assert.equal(log.args._types[0].toNumber(), transferManagerKey); assert.equal(web3.utils.toUtf8(log.args._name), "GeneralTransferManager"); @@ -309,20 +311,20 @@ contract("SecurityToken", async (accounts) => { }) it("Should finish the minting -- fail because owner didn't sign correct acknowledegement", async () => { - let signature = (web3.eth.accounts.sign("F O'Brien is the best", "0x" + token_owner_pk)).signature; - await catchRevert(I_SecurityToken.freezeIssuance(signature, { from: token_owner })); + let fakeHash = await getFreezeIssuanceAck(I_SecurityToken.address, account_investor2); + await catchRevert(I_SecurityToken.freezeIssuance(fakeHash, { from: token_owner })); }); it("Should finish the minting -- fail because msg.sender is not the owner", async () => { - let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, "0x" + token_owner_pk)).signature; - await catchRevert(I_SecurityToken.freezeIssuance(signature, { from: account_temp })); + freezeIssuanceAckHash = await getFreezeIssuanceAck(I_SecurityToken.address, token_owner); + await catchRevert(I_SecurityToken.freezeIssuance(freezeIssuanceAckHash, { from: account_temp })); }); it("Should finish minting & restrict the further minting", async () => { let id = await takeSnapshot(); - let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, "0x" + token_owner_pk)).signature; - console.log(signature); - await I_SecurityToken.freezeIssuance(signature, { from: token_owner }); + console.log(freezeIssuanceAckHash, token_owner); + + await I_SecurityToken.freezeIssuance(freezeIssuanceAckHash, { from: token_owner }); assert.isFalse(await I_SecurityToken.isIssuable.call()); await catchRevert(I_SecurityToken.issue(account_affiliate1, new BN(100).mul(new BN(10).pow(new BN(18))), "0x0", { from: token_owner, gas: 500000 })); // assert.isTrue(false); @@ -392,8 +394,7 @@ contract("SecurityToken", async (accounts) => { it("Should fail to issue tokens while STO attached after freezeMinting called", async () => { let id = await takeSnapshot(); - let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, "0x" + token_owner_pk)).signature; - await I_SecurityToken.freezeIssuance(signature, { from: token_owner }); + await I_SecurityToken.freezeIssuance(freezeIssuanceAckHash, { from: token_owner }); await catchRevert(I_SecurityToken.issue(account_affiliate1, new BN(100).mul(new BN(10).pow(new BN(18))), "0x0", { from: token_owner })); await revertToSnapshot(id); @@ -820,8 +821,7 @@ contract("SecurityToken", async (accounts) => { it("STO should fail to issue tokens after minting is frozen", async () => { let id = await takeSnapshot(); - let signature = (web3.eth.accounts.sign(freezeIssuanceAckHash, "0x" + token_owner_pk)).signature; - await I_SecurityToken.freezeIssuance(signature, { from: token_owner }); + await I_SecurityToken.freezeIssuance(freezeIssuanceAckHash, { from: token_owner }); await catchRevert( web3.eth.sendTransaction({ @@ -1219,19 +1219,18 @@ contract("SecurityToken", async (accounts) => { assert.equal(new BN(web3.utils.toWei("10", "ether")).toString(), eventTransfer.args.value.toString(), "Event not emitted as expected"); }); - it("Should fail to freeze controller functionality because not owner", async () => { - let signature = (web3.eth.accounts.sign(disableControllerAckHash, "0x" + token_owner_pk)).signature; - await catchRevert(I_SecurityToken.disableController(signature, { from: account_investor1 })); + it("Should fail to freeze controller functionality because proper acknowledgement not signed by owner", async () => { + let fakeHash = await getDisableControllerAck(I_SecurityToken.address, account_investor2); + await catchRevert(I_SecurityToken.disableController(fakeHash, { from: token_owner })); }); - it("Should fail to freeze controller functionality because proper acknowledgement not signed by owner", async () => { - let signature = (web3.eth.accounts.sign("He truely is", "0x" + token_owner_pk)).signature; - await catchRevert(I_SecurityToken.disableController(signature, { from: token_owner })); + it("Should fail to freeze controller functionality because not owner", async () => { + disableControllerAckHash = await getDisableControllerAck(I_SecurityToken.address, token_owner); + await catchRevert(I_SecurityToken.disableController(disableControllerAckHash, { from: account_investor1 })); }); it("Should successfully freeze controller functionality", async () => { - let signature = (web3.eth.accounts.sign(disableControllerAckHash, "0x" + token_owner_pk)).signature; - await I_SecurityToken.disableController(signature, { from: token_owner }); + await I_SecurityToken.disableController(disableControllerAckHash, { from: token_owner }); // check state assert.equal(address_zero, await I_SecurityToken.controller.call(), "State not changed"); assert.equal(true, await I_SecurityToken.controllerDisabled.call(), "State not changed"); @@ -1242,8 +1241,7 @@ contract("SecurityToken", async (accounts) => { }); it("Should fail to freeze controller functionality because already frozen", async () => { - let signature = (web3.eth.accounts.sign(disableControllerAckHash, "0x" + token_owner_pk)).signature; - await catchRevert(I_SecurityToken.disableController(signature, { from: token_owner })); + await catchRevert(I_SecurityToken.disableController(disableControllerAckHash, { from: token_owner })); }); it("Should fail to set controller because controller functionality frozen", async () => { diff --git a/yarn.lock b/yarn.lock index 91971f5ee..98f9ccdec 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2631,6 +2631,18 @@ eth-sig-util@^1.4.2: ethereumjs-abi "git+https://github.com/ethereumjs/ethereumjs-abi.git" ethereumjs-util "^5.1.1" +eth-sig-util@^2.1.1: + version "2.1.1" + resolved "https://registry.yarnpkg.com/eth-sig-util/-/eth-sig-util-2.1.1.tgz#4459ee4dd59091754b50410c9ca6c70fb339f59f" + integrity sha512-B9VA2WCuf+dp0UbWlzsCXWcryZe1H9PixrNmG+tQDBpyTiIbDvf2w8jUb1BNPbxFXeWHUcr2I6pmg+MkdA4Ovg== + dependencies: + buffer "^5.2.1" + elliptic "^6.4.0" + ethereumjs-abi "0.6.5" + ethereumjs-util "^5.1.1" + tweetnacl "^1.0.0" + tweetnacl-util "^0.15.0" + eth-tx-summary@^3.1.2: version "3.2.3" resolved "https://registry.yarnpkg.com/eth-tx-summary/-/eth-tx-summary-3.2.3.tgz#a52d7215616888e012fbc083b3eacd28f3e64764" @@ -2699,6 +2711,14 @@ ethereumjs-abi@0.6.4: bn.js "^4.10.0" ethereumjs-util "^4.3.0" +ethereumjs-abi@0.6.5: + version "0.6.5" + resolved "https://registry.yarnpkg.com/ethereumjs-abi/-/ethereumjs-abi-0.6.5.tgz#5a637ef16ab43473fa72a29ad90871405b3f5241" + integrity sha1-WmN+8Wq0NHP6cqKa2QhxQFs/UkE= + dependencies: + bn.js "^4.10.0" + ethereumjs-util "^4.3.0" + ethereumjs-abi@^0.6.5, "ethereumjs-abi@git+https://github.com/ethereumjs/ethereumjs-abi.git": version "0.6.6" resolved "git+https://github.com/ethereumjs/ethereumjs-abi.git#d84a96796079c8595a0c78accd1e7709f2277215" @@ -7069,6 +7089,11 @@ tunnel-agent@^0.6.0: dependencies: safe-buffer "^5.0.1" +tweetnacl-util@^0.15.0: + version "0.15.0" + resolved "https://registry.yarnpkg.com/tweetnacl-util/-/tweetnacl-util-0.15.0.tgz#4576c1cee5e2d63d207fee52f1ba02819480bc75" + integrity sha1-RXbBzuXi1j0gf+5S8boCgZSAvHU= + tweetnacl@0.13.2: version "0.13.2" resolved "https://registry.yarnpkg.com/tweetnacl/-/tweetnacl-0.13.2.tgz#453161770469d45cd266c36404e2bc99a8fa9944" @@ -7079,6 +7104,11 @@ tweetnacl@^0.14.3, tweetnacl@~0.14.0: resolved "https://registry.yarnpkg.com/tweetnacl/-/tweetnacl-0.14.5.tgz#5ae68177f192d4456269d108afa93ff8743f4f64" integrity sha1-WuaBd/GS1EViadEIr6k/+HQ/T2Q= +tweetnacl@^1.0.0: + version "1.0.1" + resolved "https://registry.yarnpkg.com/tweetnacl/-/tweetnacl-1.0.1.tgz#2594d42da73cd036bd0d2a54683dd35a6b55ca17" + integrity sha512-kcoMoKTPYnoeS50tzoqjPY3Uv9axeuuFAZY9M/9zFnhoVvRfxz9K29IMPD7jGmt2c8SW7i3gT9WqDl2+nV7p4A== + type-check@~0.3.2: version "0.3.2" resolved "https://registry.yarnpkg.com/type-check/-/type-check-0.3.2.tgz#5884cab512cf1d355e3fb784f30804b2b520db72" From 69ac7b4284f9b74cb7b6e9fdd994764a6efbd349 Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Thu, 14 Mar 2019 16:37:39 +0530 Subject: [PATCH 05/10] Cleanup --- contracts/libraries/TokenLib.sol | 2 +- contracts/tokens/SecurityToken.sol | 9 +-------- test/helpers/signData.js | 4 ++-- test/o_security_token.js | 2 -- 4 files changed, 4 insertions(+), 13 deletions(-) diff --git a/contracts/libraries/TokenLib.sol b/contracts/libraries/TokenLib.sol index 6b90ffb59..a181b7e52 100644 --- a/contracts/libraries/TokenLib.sol +++ b/contracts/libraries/TokenLib.sol @@ -56,7 +56,7 @@ library TokenLib { } function hash(Acknowledgment memory _ack) internal pure returns (bytes32) { - return keccak256(abi.encode(keccak256(bytes(_ack.text)))); + return keccak256(abi.encode(ACK_TYPEHASH, keccak256(bytes(_ack.text)))); } function recoverFreezeIssuanceAckSigner(bytes memory _signature) public view returns (address) { diff --git a/contracts/tokens/SecurityToken.sol b/contracts/tokens/SecurityToken.sol index 976f86ce8..0e02bf363 100644 --- a/contracts/tokens/SecurityToken.sol +++ b/contracts/tokens/SecurityToken.sol @@ -51,8 +51,6 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi event GranularityChanged(uint256 _oldGranularity, uint256 _newGranularity); // Emit when is permanently frozen by the issuer event FreezeIssuance(); - - event AddressEvent(address sig); // Emit when transfers are frozen or unfrozen event FreezeTransfers(bool _status); // Emit when Module get archived from the securityToken @@ -479,17 +477,12 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi return issuance; } - - /** * @notice Permanently freeze issuance of this security token. * @dev It MUST NOT be possible to increase `totalSuppy` after this function is called. */ function freezeIssuance(bytes calldata _signature) external onlyOwner { - emit AddressEvent(address(this)); - address ad = TokenLib.recoverFreezeIssuanceAckSigner(_signature); - emit AddressEvent(ad); - require(owner() == ad, "Owner did not sign"); + require(owner() == TokenLib.recoverFreezeIssuanceAckSigner(_signature), "Owner did not sign"); issuance = false; /*solium-disable-next-line security/no-block-members*/ emit FreezeIssuance(); diff --git a/test/helpers/signData.js b/test/helpers/signData.js index 399e45bf4..e6f1ad414 100644 --- a/test/helpers/signData.js +++ b/test/helpers/signData.js @@ -1,5 +1,5 @@ const Web3 = require("web3"); -const sigUtil = require('eth-sig-util') +//const sigUtil = require('eth-sig-util') let BN = Web3.utils.BN; function getSignSTMData(tmAddress, nonce, validFrom, expiry, fromAddress, toAddress, amount, pk) { @@ -74,7 +74,7 @@ async function getFreezeIssuanceAck(stAddress, from) { // data: typedData, // sig: result // }) - // console.log('recovered', recovered); + // console.log('recovered address', recovered); return result; } diff --git a/test/o_security_token.js b/test/o_security_token.js index 75061b6a5..b0d289593 100644 --- a/test/o_security_token.js +++ b/test/o_security_token.js @@ -322,8 +322,6 @@ contract("SecurityToken", async (accounts) => { it("Should finish minting & restrict the further minting", async () => { let id = await takeSnapshot(); - console.log(freezeIssuanceAckHash, token_owner); - await I_SecurityToken.freezeIssuance(freezeIssuanceAckHash, { from: token_owner }); assert.isFalse(await I_SecurityToken.isIssuable.call()); await catchRevert(I_SecurityToken.issue(account_affiliate1, new BN(100).mul(new BN(10).pow(new BN(18))), "0x0", { from: token_owner, gas: 500000 })); From ce84e531f7843b0668e4a1682badf53dd8c2a8ba Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Thu, 14 Mar 2019 16:48:33 +0530 Subject: [PATCH 06/10] More cleanup --- contracts/tokens/SecurityToken.sol | 4 +--- package.json | 1 - yarn.lock | 30 ------------------------------ 3 files changed, 1 insertion(+), 34 deletions(-) diff --git a/contracts/tokens/SecurityToken.sol b/contracts/tokens/SecurityToken.sol index 0e02bf363..9e247f8d8 100644 --- a/contracts/tokens/SecurityToken.sol +++ b/contracts/tokens/SecurityToken.sol @@ -17,7 +17,6 @@ import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; import "openzeppelin-solidity/contracts/utils/ReentrancyGuard.sol"; import "openzeppelin-solidity/contracts/token/ERC20/ERC20.sol"; import "openzeppelin-solidity/contracts/token/ERC20/ERC20Detailed.sol"; -import "openzeppelin-solidity/contracts/cryptography/ECDSA.sol"; /** * @title Security Token contract @@ -32,7 +31,6 @@ import "openzeppelin-solidity/contracts/cryptography/ECDSA.sol"; contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, SecurityTokenStorage, IERC1594, IERC1643, IERC1644, Proxy { using SafeMath for uint256; - using ECDSA for bytes32; // Emit at the time when module get added event ModuleAdded( @@ -481,7 +479,7 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi * @notice Permanently freeze issuance of this security token. * @dev It MUST NOT be possible to increase `totalSuppy` after this function is called. */ - function freezeIssuance(bytes calldata _signature) external onlyOwner { + function freezeIssuance(bytes calldata _signature) external isIssuanceAllowed onlyOwner { require(owner() == TokenLib.recoverFreezeIssuanceAckSigner(_signature), "Owner did not sign"); issuance = false; /*solium-disable-next-line security/no-block-members*/ diff --git a/package.json b/package.json index a6315a6ff..3cc06302e 100644 --- a/package.json +++ b/package.json @@ -57,7 +57,6 @@ }, "homepage": "https://github.com/PolymathNetwork/polymath-core#readme", "dependencies": { - "eth-sig-util": "^2.1.1", "truffle": "^5.0.4", "truffle-hdwallet-provider": "^1.0.4", "web3-provider-engine": "^14.1.0" diff --git a/yarn.lock b/yarn.lock index 98f9ccdec..91971f5ee 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2631,18 +2631,6 @@ eth-sig-util@^1.4.2: ethereumjs-abi "git+https://github.com/ethereumjs/ethereumjs-abi.git" ethereumjs-util "^5.1.1" -eth-sig-util@^2.1.1: - version "2.1.1" - resolved "https://registry.yarnpkg.com/eth-sig-util/-/eth-sig-util-2.1.1.tgz#4459ee4dd59091754b50410c9ca6c70fb339f59f" - integrity sha512-B9VA2WCuf+dp0UbWlzsCXWcryZe1H9PixrNmG+tQDBpyTiIbDvf2w8jUb1BNPbxFXeWHUcr2I6pmg+MkdA4Ovg== - dependencies: - buffer "^5.2.1" - elliptic "^6.4.0" - ethereumjs-abi "0.6.5" - ethereumjs-util "^5.1.1" - tweetnacl "^1.0.0" - tweetnacl-util "^0.15.0" - eth-tx-summary@^3.1.2: version "3.2.3" resolved "https://registry.yarnpkg.com/eth-tx-summary/-/eth-tx-summary-3.2.3.tgz#a52d7215616888e012fbc083b3eacd28f3e64764" @@ -2711,14 +2699,6 @@ ethereumjs-abi@0.6.4: bn.js "^4.10.0" ethereumjs-util "^4.3.0" -ethereumjs-abi@0.6.5: - version "0.6.5" - resolved "https://registry.yarnpkg.com/ethereumjs-abi/-/ethereumjs-abi-0.6.5.tgz#5a637ef16ab43473fa72a29ad90871405b3f5241" - integrity sha1-WmN+8Wq0NHP6cqKa2QhxQFs/UkE= - dependencies: - bn.js "^4.10.0" - ethereumjs-util "^4.3.0" - ethereumjs-abi@^0.6.5, "ethereumjs-abi@git+https://github.com/ethereumjs/ethereumjs-abi.git": version "0.6.6" resolved "git+https://github.com/ethereumjs/ethereumjs-abi.git#d84a96796079c8595a0c78accd1e7709f2277215" @@ -7089,11 +7069,6 @@ tunnel-agent@^0.6.0: dependencies: safe-buffer "^5.0.1" -tweetnacl-util@^0.15.0: - version "0.15.0" - resolved "https://registry.yarnpkg.com/tweetnacl-util/-/tweetnacl-util-0.15.0.tgz#4576c1cee5e2d63d207fee52f1ba02819480bc75" - integrity sha1-RXbBzuXi1j0gf+5S8boCgZSAvHU= - tweetnacl@0.13.2: version "0.13.2" resolved "https://registry.yarnpkg.com/tweetnacl/-/tweetnacl-0.13.2.tgz#453161770469d45cd266c36404e2bc99a8fa9944" @@ -7104,11 +7079,6 @@ tweetnacl@^0.14.3, tweetnacl@~0.14.0: resolved "https://registry.yarnpkg.com/tweetnacl/-/tweetnacl-0.14.5.tgz#5ae68177f192d4456269d108afa93ff8743f4f64" integrity sha1-WuaBd/GS1EViadEIr6k/+HQ/T2Q= -tweetnacl@^1.0.0: - version "1.0.1" - resolved "https://registry.yarnpkg.com/tweetnacl/-/tweetnacl-1.0.1.tgz#2594d42da73cd036bd0d2a54683dd35a6b55ca17" - integrity sha512-kcoMoKTPYnoeS50tzoqjPY3Uv9axeuuFAZY9M/9zFnhoVvRfxz9K29IMPD7jGmt2c8SW7i3gT9WqDl2+nV7p4A== - type-check@~0.3.2: version "0.3.2" resolved "https://registry.yarnpkg.com/type-check/-/type-check-0.3.2.tgz#5884cab512cf1d355e3fb784f30804b2b520db72" From 29bd8dd77f000cd0d54799981e95e6a365734c83 Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Thu, 14 Mar 2019 17:02:45 +0530 Subject: [PATCH 07/10] Cleaned up tests --- test/o_security_token.js | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/test/o_security_token.js b/test/o_security_token.js index b0d289593..a08ba55c9 100644 --- a/test/o_security_token.js +++ b/test/o_security_token.js @@ -3,7 +3,6 @@ import { duration, ensureException, promisifyLogWatch, latestBlock } from "./hel import { getFreezeIssuanceAck, getDisableControllerAck } from "./helpers/signData"; import { takeSnapshot, increaseTime, revertToSnapshot } from "./helpers/time"; import { encodeProxyCall, encodeModuleCall } from "./helpers/encodeCall"; -import { pk } from "./helpers/testprivateKey"; import { catchRevert } from "./helpers/exceptions"; import { setUpPolymathNetwork, @@ -31,7 +30,6 @@ contract("SecurityToken", async (accounts) => { let account_investor1; let account_issuer; let token_owner; - let token_owner_pk; let disableControllerAckHash; let freezeIssuanceAckHash; let account_investor2; @@ -135,8 +133,6 @@ contract("SecurityToken", async (accounts) => { account_investor1 = accounts[9]; token_owner = account_issuer; - token_owner_pk = pk.account_1; - account_controller = account_temp; // Step:1 Create the polymath ecosystem contract instances @@ -200,10 +196,7 @@ contract("SecurityToken", async (accounts) => { I_SecurityToken = await SecurityToken.at(tx.logs[2].args._securityTokenAddress); stGetter = await STGetter.at(I_SecurityToken.address); assert.equal(await stGetter.getTreasuryWallet.call(), token_owner, "Incorrect wallet set") - const log = (await I_SecurityToken.getPastEvents('ModuleAdded', {filter: {transactionHash: tx.transactionHash}}))[0]; - - disableControllerAckHash = "0x6c33f5d82a4088dba7f7969350798c7a2900b5a3689f123d0630d513d05e5611"; - + const log = (await I_SecurityToken.getPastEvents('ModuleAdded', {filter: {transactionHash: tx.transactionHash}}))[0]; // Verify that GeneralTransferManager module get added successfully or not assert.equal(log.args._types[0].toNumber(), transferManagerKey); @@ -311,8 +304,10 @@ contract("SecurityToken", async (accounts) => { }) it("Should finish the minting -- fail because owner didn't sign correct acknowledegement", async () => { - let fakeHash = await getFreezeIssuanceAck(I_SecurityToken.address, account_investor2); - await catchRevert(I_SecurityToken.freezeIssuance(fakeHash, { from: token_owner })); + let trueButOutOfPlaceAcknowledegement = web3.utils.utf8ToHex( + "F O'Brien is the best!" + ); + await catchRevert(I_SecurityToken.freezeIssuance(trueButOutOfPlaceAcknowledegement, { from: token_owner })); }); it("Should finish the minting -- fail because msg.sender is not the owner", async () => { @@ -325,7 +320,6 @@ contract("SecurityToken", async (accounts) => { await I_SecurityToken.freezeIssuance(freezeIssuanceAckHash, { from: token_owner }); assert.isFalse(await I_SecurityToken.isIssuable.call()); await catchRevert(I_SecurityToken.issue(account_affiliate1, new BN(100).mul(new BN(10).pow(new BN(18))), "0x0", { from: token_owner, gas: 500000 })); - // assert.isTrue(false); await revertToSnapshot(id); }); From 5d2475a5443b909e274f028081ce70b6028e395e Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Fri, 15 Mar 2019 10:38:28 +0530 Subject: [PATCH 08/10] Skip acknowledgement tests in coverage --- test/o_security_token.js | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/test/o_security_token.js b/test/o_security_token.js index a08ba55c9..9ff0e6f2d 100644 --- a/test/o_security_token.js +++ b/test/o_security_token.js @@ -303,6 +303,7 @@ contract("SecurityToken", async (accounts) => { assert.isTrue(await I_SecurityToken.isIssuable.call()); }) + it("Should finish the minting -- fail because owner didn't sign correct acknowledegement", async () => { let trueButOutOfPlaceAcknowledegement = web3.utils.utf8ToHex( "F O'Brien is the best!" @@ -310,12 +311,14 @@ contract("SecurityToken", async (accounts) => { await catchRevert(I_SecurityToken.freezeIssuance(trueButOutOfPlaceAcknowledegement, { from: token_owner })); }); - it("Should finish the minting -- fail because msg.sender is not the owner", async () => { + // solidity-coverage uses an older version of testrpc that does not support eth_signTypedData. It is required to signt he acknowledgement + process.env.COVERAGE ? it.skip : it("Should finish the minting -- fail because msg.sender is not the owner", async () => { freezeIssuanceAckHash = await getFreezeIssuanceAck(I_SecurityToken.address, token_owner); await catchRevert(I_SecurityToken.freezeIssuance(freezeIssuanceAckHash, { from: account_temp })); }); - it("Should finish minting & restrict the further minting", async () => { + // solidity-coverage uses an older version of testrpc that does not support eth_signTypedData. It is required to signt he acknowledgement + process.env.COVERAGE ? it.skip : it("Should finish minting & restrict the further minting", async () => { let id = await takeSnapshot(); await I_SecurityToken.freezeIssuance(freezeIssuanceAckHash, { from: token_owner }); assert.isFalse(await I_SecurityToken.isIssuable.call()); @@ -384,7 +387,8 @@ contract("SecurityToken", async (accounts) => { assert.equal(balance.div(new BN(10).pow(new BN(18))).toNumber(), 300); }); - it("Should fail to issue tokens while STO attached after freezeMinting called", async () => { + // solidity-coverage uses an older version of testrpc that does not support eth_signTypedData. It is required to signt he acknowledgement + process.env.COVERAGE ? it.skip : it("Should fail to issue tokens while STO attached after freezeMinting called", async () => { let id = await takeSnapshot(); await I_SecurityToken.freezeIssuance(freezeIssuanceAckHash, { from: token_owner }); @@ -811,7 +815,8 @@ contract("SecurityToken", async (accounts) => { assert.equal((await I_SecurityToken.balanceOf(account_investor1)).div(new BN(10).pow(new BN(18))).toNumber(), 1000); }); - it("STO should fail to issue tokens after minting is frozen", async () => { + // solidity-coverage uses an older version of testrpc that does not support eth_signTypedData. It is required to signt he acknowledgement + process.env.COVERAGE ? it.skip : it("STO should fail to issue tokens after minting is frozen", async () => { let id = await takeSnapshot(); await I_SecurityToken.freezeIssuance(freezeIssuanceAckHash, { from: token_owner }); @@ -1212,35 +1217,39 @@ contract("SecurityToken", async (accounts) => { }); it("Should fail to freeze controller functionality because proper acknowledgement not signed by owner", async () => { - let fakeHash = await getDisableControllerAck(I_SecurityToken.address, account_investor2); - await catchRevert(I_SecurityToken.disableController(fakeHash, { from: token_owner })); + let trueButOutOfPlaceAcknowledegement = web3.utils.utf8ToHex( + "F O'Brien is the best!" + ); + await catchRevert(I_SecurityToken.disableController(trueButOutOfPlaceAcknowledegement, { from: token_owner })); }); - it("Should fail to freeze controller functionality because not owner", async () => { + // solidity-coverage uses an old version of testrpc that does not support eth_signTypedData. It is required to sign he acknowledgement + process.env.COVERAGE ? it.skip : it("Should fail to freeze controller functionality because not owner", async () => { disableControllerAckHash = await getDisableControllerAck(I_SecurityToken.address, token_owner); await catchRevert(I_SecurityToken.disableController(disableControllerAckHash, { from: account_investor1 })); }); - it("Should successfully freeze controller functionality", async () => { + // solidity-coverage uses an old version of testrpc that does not support eth_signTypedData. It is required to sign he acknowledgement + process.env.COVERAGE ? it.skip : it("Should successfully freeze controller functionality", async () => { await I_SecurityToken.disableController(disableControllerAckHash, { from: token_owner }); // check state assert.equal(address_zero, await I_SecurityToken.controller.call(), "State not changed"); assert.equal(true, await I_SecurityToken.controllerDisabled.call(), "State not changed"); - }); - - it("Should ST be not controllable", async() => { assert.isFalse(await I_SecurityToken.isControllable.call()); }); - it("Should fail to freeze controller functionality because already frozen", async () => { + // solidity-coverage uses an old version of testrpc that does not support eth_signTypedData. It is required to sign he acknowledgement + process.env.COVERAGE ? it.skip : it("Should fail to freeze controller functionality because already frozen", async () => { await catchRevert(I_SecurityToken.disableController(disableControllerAckHash, { from: token_owner })); }); - it("Should fail to set controller because controller functionality frozen", async () => { + // solidity-coverage uses an old version of testrpc that does not support eth_signTypedData. It is required to sign he acknowledgement + process.env.COVERAGE ? it.skip : it("Should fail to set controller because controller functionality frozen", async () => { await catchRevert(I_SecurityToken.setController(account_controller, { from: token_owner })); }); - it("Should fail to controllerTransfer because controller functionality frozen", async () => { + // solidity-coverage uses an old version of testrpc that does not support eth_signTypedData. It is required to sign he acknowledgement + process.env.COVERAGE ? it.skip : it("Should fail to controllerTransfer because controller functionality frozen", async () => { await catchRevert( I_SecurityToken.controllerTransfer(account_investor1, account_investor2, new BN(web3.utils.toWei("10", "ether")), "0x0", web3.utils.fromAscii("reason"), { from: account_controller From e97e20abf8c80ec2a2fb4a75fe05144855a23f3b Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Fri, 15 Mar 2019 11:20:14 +0530 Subject: [PATCH 09/10] Test fix --- test/o_security_token.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/o_security_token.js b/test/o_security_token.js index 9ff0e6f2d..f8bd26ab7 100644 --- a/test/o_security_token.js +++ b/test/o_security_token.js @@ -1363,6 +1363,7 @@ contract("SecurityToken", async (accounts) => { assert.oneOf( await readStorage(I_SecurityToken.address, 7), [ + (await stGetter.controller.call()).toLowerCase(), (await stGetter.controller.call()).substring(0, 4), (await stGetter.controller.call()).substring(0, 3), await stGetter.controller.call() From be8103d93d0e49691682aa4f9b46a79db3377fef Mon Sep 17 00:00:00 2001 From: Mudit Gupta Date: Fri, 22 Mar 2019 17:57:42 +0530 Subject: [PATCH 10/10] Merge fix --- contracts/tokens/SecurityToken.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/tokens/SecurityToken.sol b/contracts/tokens/SecurityToken.sol index 5ebda0313..1600237d1 100644 --- a/contracts/tokens/SecurityToken.sol +++ b/contracts/tokens/SecurityToken.sol @@ -611,7 +611,7 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi */ function disableController(bytes calldata _signature) external onlyOwner { require(owner() == TokenLib.recoverDisableControllerAckSigner(_signature), "Owner did not sign"); - require(_isControllable()); + require(isControllable()); controllerDisabled = true; delete controller; emit DisableController();