[3.5, 3.7, 3.8] Fix custom modules #698

adamdossa · 2019-05-28T21:28:31Z

Changes made:

made useModule and registerModule non-reentrant as they make external calls to potentially untrusted contracts
store factory owner on initial registration locally and reference this rather than calling back out to owner on the module factory. This means that a malicious factory cannot change the value of owner dynamically to e.g. allow security tokens to add it (by returning an owner equal the ST owner). The initial owner cannot be malicious as registerModule requires msg.sender to be the factory owner.
fix issue where a malicious factory could return a type of 0, causing it to be un-removable
modified getModulesByType to only return verified modules, so that a malicious unverified module cannot grief this function.
added getAllModulesByType can be used to return a raw list of all registered modules instead
modify getFactoryDetails to also return the factory owner

maxsam4

In registerModule() L217, the complete array of modules by type is read from storage and copied into memory. This means once the list grows too big (~15k modules), this function will stop working.

Also, 2 tests are failing.

maxsam4 · 2019-05-29T06:44:58Z

contracts/ModuleRegistry.sol

@@ -129,10 +144,11 @@ contract ModuleRegistry is IModuleRegistry, EternalStorage {
     * @param _moduleFactory is the address of the relevant module factory
     * @param _isUpgrade whether or not the function is being called as a result of an upgrade
     */
-    function useModule(address _moduleFactory, bool _isUpgrade) external {
-        if (IFeatureRegistry(getAddressValue(FEATURE_REGISTRY)).getFeatureStatus("customModulesAllowed")) {
+    function useModule(address _moduleFactory, bool _isUpgrade) external nonReentrant {


useModule() does not need to be guarded for reentrancy. There is no harm in guarding it either so it's fine anyway.

isCompatibleModule called from useModule makes external calls

yes but there is no point in re-entering useModule(). I don't see any reason why any (malicious) user might want to renter useModule().

maxsam4 · 2019-05-29T06:45:45Z

contracts/ModuleRegistry.sol

        bool isFactory = msg.sender == _moduleFactory;
-        require(isOwner || isFactoryOwner || isFactory, "Not authorised");
+        bool isFactoryOwner = msg.sender == getAddressValue(Encoder.getKey("factoryOwner", _moduleFactory));
+        // Ordering (lazy evaluation) in below require statement is important. A moduleFactory should not be able


This code comment isn't valid anymore, is it?

True - was from an earlier iteration - removed.

maxsam4 · 2019-05-29T06:47:48Z

contracts/ModuleRegistry.sol

     * @param _moduleType Type of Module
     * @return address array that contains the list of addresses of module factory contracts.
     */
    function getModulesByType(uint8 _moduleType) public view returns(address[] memory) {
+        address[] memory _addressList = getArrayAddress(Encoder.getKey("moduleList", uint256(_moduleType)));


This will only return modules with primary type (type[0]) == _moduleType. Maybe we should add the factory to all the relevant module type lists in registerModule()?

Yeah - that would probably be better, although we'd also have to modify the removeModule logic and the "moduleListIndex" etc. to reflect this, so I'd say this is better in 3.1 than 3.0

Agreed. As this function is really only for offchain stuff, I don't mind it returning incomplete data. Perhaps we should add a code comment about this though. I don't want dApp and CLI teams scratching their heads a few months down the line :).

maxsam4 · 2019-05-29T06:48:57Z

contracts/ModuleRegistry.sol

@@ -208,8 +228,10 @@ contract ModuleRegistry is IModuleRegistry, EternalStorage {
        uint256 moduleType = getUintValue(Encoder.getKey("registry", _moduleFactory));

        require(moduleType != 0, "Module factory should be registered");
+        // Ordering (lazy evaluation) in below require statement is important. A moduleFactory should not be able


This code comment isn't valid anymore, is it?

maxsam4 · 2019-05-29T06:51:33Z

contracts/ModuleRegistry.sol

     * @param _moduleType Type of Module
     * @return address array that contains the list of addresses of module factory contracts.
     */
    function getModulesByType(uint8 _moduleType) public view returns(address[] memory) {
+        address[] memory _addressList = getArrayAddress(Encoder.getKey("moduleList", uint256(_moduleType)));


Since we are fetching all modules (verified and unverified) here and looping over them, this function can still be griefed by unverified modules. It will require like 10k unverified modules to make this function cost 8m gas but it can be done.

Yep - although the expectation is that this function would only be called off-chain, so could use an even higher gas limit. The bigger concern is in registerModule where we read in the whole list which obviously is done on chain so could be griefed. Assuming registering a module costs 100,000 gas and a price of 3GWei it only costs 3.3 ETH to register 10,000 modules, so this is a pretty cheap attack. Having said that, our defence would be to upgrade the module registry and fix (e.g. by removing the bad modules and modifying the code) so probably no-one would be very motivated to do this.

Agreed that this is more of an inconvenience than an Issue. I'd want to fix the registerModule issue before allowing custom modules (which may be a while away).

btw the attack will cost ~150 ETH because the cost of registering modules will keep increasing and it will actually require ~15k modules to hit the target. Given the cost, I'd be fine with just adding a code comment about it for now.

coveralls · 2019-05-29T13:59:50Z

Coverage increased (+0.03%) to 95.215% when pulling ea913b9 on Custom-Module-Fixes into db6ee34 on dev-3.0.0.

tintinweb · 2019-06-11T21:15:28Z

👍 tin/diligence

* Add StatusCode library * make inline library * remove the helpers/PolyToken.sol * remove the IBoot from the contracts (#687) * cleanup (#686) * remove unnecessary modifiers (#685) * Remove KindMath from TokenLib (#684) * cleanup * remove the KindMath * Marked some functions as external (#678) * Removed redundant pause/unpause (#676) * Made onlyModuleOrOwner definition consisitent (#674) * Made onlyModuleOrOwner definition consisitent * Added braces * Added braces * minor undeflow fix in vesting ewallet (#673) * Updated license to Apache 2.0 * Update LICENSE * [3.20] Don't ask for name when registering ticker (#706) * Removed storing token name when registering ticker * Added backwards compatible functions * Updated tests to use latest functions * Renamed functions for easy testing on truffle * Fixed test * [3.38]: Add ST storage layout check script in CI pipeline (#704) * add st storage layout check script in CI pipeline * add info comment * 3.30 Remove OwnedProxy (#701) * Audit fix * Fix * Moved variable to storage contract * [3.31] Removed take usage fee (#700) * Removed takeUsageFee * Test fix * tests fixed * [3.5, 3.7, 3.8] Fix custom modules (#698) * Fix custom modules * Remove unnecessary modifier * Fix some test cases * disallow creation of 0 supply dividend (#697) * Added break in deleteDelegate (#696) * Added 0 length name check (#695) * Audit 3.4 & 3.14 Fix ST upgrades (#694) * Fixes + test cases * Small change in test * Add fixes for 3.14 * Fix setProtocolFactory() (#689) * Remove inconsistency of the index value (#688) * remove the unneccessary code from partitionsOf() (#681) * Added constructors (#672) * 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. * Migration fixed * test fix * test fix * Add the ability to configure the new STR atomically * Fix the returnPartition function (#680) * fix the returnPartition function * minor fix * Remove comment from codebase (#714) * Update interfaces to named parameters (#708) * WIP * Change some values * Make some mappings public for automatic getters * Put back truffle version * updated transfer manager results (#693) * Allow Custom oracle in USDTieredSTO (#691) * Added custom oracles to USDTSTO * Updated custom oracles logic * Pinned solidity version (#675) * Pinned solidity version * Fix merge conflicts * minor transfer optimization (#668) * Fix the BalanceOfPartition audit item (#679) * fix balance of partition function * fix balance of Partition function * return balance in the parent implementation of the getTokensByPartition() * Extra Items (#702) * minor improvements * permission fixes * Synchronise the ISTR with STR (#682) * synchronise the ISTR with STR * fix the interface problem * minor fixes * add some function in interface * add comment in STR * consistency in interfaces of contracts * improve the st interface * remove shadow declaration * add #706 new function declartion in the ISTR * add missing functions in interface * Specific contract type used (#683) * specific contract type used * add more type contract * remove the redundant KindMath * remove unnecessary casting * Merge fixes * Fix PLCR Proposal inconsistency (#713) * fix the proposal * minor styling and comments addition * Fix contract size issue. * Update licenses - Apache 2.0 As per https://spdx.org/licenses/ * Fix canTransfer spec (#709) * Fix spec * Fix conflict * Fix merge conflict * Small fixes * Revert package.json change * Fix test case * Set solcjs as default compiler (#716) * Set solcjs as default compiler If you want to use native solc, set the environemnt variable POLYMATH_NATIVE_SOLC as true. * Removed native solc from travis * Removed native solc version query from travis

* PLCR voting module * WIP * Clean up types & tags * Fix test cases * Check that there is a non-zero supply when creating a dividend * improvements and fixes * factory fix * minor fix * improve the PLCR voting * add more functionality and add test cases * Ported STO fixes from dev-2.1.0 (#591) * Ported STO fixes from dev-2.1.0 * Revert when cap reached instead of failing silently * Restored missing require * USDTSTO granularity edge case fixed * add more tests * add overflow check * add overflow check * GTM minor refactoring * gtm test fix * MATM optimizations * Fixed matm tests * minor fixes * add comments * minor comment * Added a test case * Minor optimization * add getTokensByPartition in GTM and BTM * Minor optimization in verify transfer * add test for the VRTM getTokensByPartition * add the pause functionality in the getTokensByPartition() * Get subset of investors at a checkpoint (#611) * Updated checkpoint event and optimized st * Optimized dividends push payment * Added test * Minor datastore optimization * Test fixed * Add acknowledgement for irrevocable actions using EIP712 (#599) * Require signed user ack * Added test * Small fix * WIP * Cleanup * More cleanup * Cleaned up tests * Skip acknowledgement tests in coverage * Test fix * Merge fix * add pause effects in LTM * Allow to define fee in Poly * fix test * Updated factory deployments * Updated STR tests * Tests fixed * Added code comment to oracle * Add STR tests * Simplified change currency functions * Updated STR tests * Added module factory tests * Fix timestamp for checkpoints tests * Cosmetic changes * Fix timestamp for checkpoints tests * Skip cp module in coverage * Port dividend fix (#610) * port the 100% tax withholding changes * minor size optimization * add reclaim functions in every module * minor fixes * restrict the changes if dividend is expired * Allow token name change (#600) * Allow owner to change name * Reduce size * Update ST interface * Added update name event * Updated OZ * Updated storage verification test * Moved BTM, LTM, Vesting out of experimental * Typo fix * Test fix * minor fixes * start proposal Id from 1 instead of 0 * BTM optimizations and bug fix * Bug fix * Typo fix * Added clash checker script * Beutified function selector clash check script * Throw on finding a clash * Added circle CI job * Upgradable tokens (#602) * wip * Fixes for migration * Broken :-( * Fixes * Fix tests * Some more fixes * Lots more logic * Add ability to add modules as archived * Add a test * some more tests * Remove test file * Cleanup some test cases * Fix module / token versioning * Fix more tests * More version checking & fixes * Remove badtest * Reduce size of mock contract * Some updates * Remove unnecessary constructors * Updates * Merge fixes * Minor update * Merge fixes * implement all functions of ERC1410 * LTM optimizations and bug fix * minor fix * Vesting escrow wallet optimizations and bug fix * remove the approvals mapping * Added test cases for bug fixes * reduce the ST code size upto 23.84 KB * remove unnecessary mocks contract and cutdown the ST size till 23.72 KB * Increased max module types (#636) Increased the number of module types that the STFactory must check for potential incompatibilities before allowing the token to upgrade. * add test cases and fix bugs * resolve conflicts and add test cases * minor test fixes * minor test fix * Increase solidity coverage memory limit * Raised memory limits of truffle and ganache * refactoring voting modules & adding functionality * Refresh / Upgrade tokens (#632) * Added refresh token function * Reduced STR size * Added test cases for refreshToken * reuse function * Added refresh token event * fix compiler error * Port the VRTM audit fixes (#635) * port the VRTM audit fixes * cleanup code * move voting modules from experimental * fix test & add test * fix test for weighted vote module * add more tests * increase test coverage * increase more coverage * Minor optimizations (#628) * Optimized datastore batch functions * fixed ST mock compiler warnings * Optimized 10^18 * Name null check updated * GPM optimizations * CTM optimization * GTM optimization * Transfer type made enum * Added compiler version to supress warning * Minor STR optimization * Minor verify transfer optimizations * Added set bytes function * Marked initialize function of STR non-payable * Deleted registry updater * Added comments for pre computed hashes * Added more pre computed hashes * batch functions optimized * minor ST optimizations * Can transfer minor optimization * Reduced mock contract size * Removed dead code * Minor changes * Increased STR compatibility (#640) * Made STR backwards compatible * Fixed tests * Added test case * Merge fix * Hardcode version checks * Fixed pclr voting test * Revert prettification This reverts commit 7efad95. * build fix * Typo fix * Fixed tests * Updated and patched soldiity docgen * Update changelog for some extent * Copy dev-2.1.0 CLI into dev-3.0.0 * Fix Errors + add missing functions & Events * Add ISecurityTokenRegistry to contract abis + fix OZ ERC20 abi * Fix/Update ST20Generator for V3.0.0. * Add ISecurityToken to contract abis * Blue Bull - because I like it * Add events and some public constant getters to ISecurityToken * Token Manager CLI Fixes * revert yarn.lock changes * more token manager fixes * More CLI STM and TM fixes * WIP: More TM CLI fixes and updates * Update as per PR #669 to master * TM CLI updates for setting and checking investor flags * CLI Fixes for remaining transfer manager modules * Port Combine modify whitelist commands (ref PR #667) * STO CLI 3.0.0 fixes * Transfer CLI fixes * Investor CLI fixes * dividend manager CLI fixes * Contract Manager CLI Fixes and updates * Minor ST20 Generator change * Permission Manager Cli Fixes * Pin solc to 0.5.8 * made solc executable * Transfer managers with version * usd and poly fees for STR * Labeled modules Holder count * CLI Treasury wallet * CLI Change token name * CLI ST Documents * CLI controller transfer renamed * CLI partitions and operators * CLI inputs with limits abstracted to input.js * Moved OZ from devDependencies to dependencies (#707) * Approved Audit fixes (#705) * Add StatusCode library * make inline library * remove the helpers/PolyToken.sol * remove the IBoot from the contracts (#687) * cleanup (#686) * remove unnecessary modifiers (#685) * Remove KindMath from TokenLib (#684) * cleanup * remove the KindMath * Marked some functions as external (#678) * Removed redundant pause/unpause (#676) * Made onlyModuleOrOwner definition consisitent (#674) * Made onlyModuleOrOwner definition consisitent * Added braces * Added braces * minor undeflow fix in vesting ewallet (#673) * Updated license to Apache 2.0 * Update LICENSE * [3.20] Don't ask for name when registering ticker (#706) * Removed storing token name when registering ticker * Added backwards compatible functions * Updated tests to use latest functions * Renamed functions for easy testing on truffle * Fixed test * [3.38]: Add ST storage layout check script in CI pipeline (#704) * add st storage layout check script in CI pipeline * add info comment * 3.30 Remove OwnedProxy (#701) * Audit fix * Fix * Moved variable to storage contract * [3.31] Removed take usage fee (#700) * Removed takeUsageFee * Test fix * tests fixed * [3.5, 3.7, 3.8] Fix custom modules (#698) * Fix custom modules * Remove unnecessary modifier * Fix some test cases * disallow creation of 0 supply dividend (#697) * Added break in deleteDelegate (#696) * Added 0 length name check (#695) * Audit 3.4 & 3.14 Fix ST upgrades (#694) * Fixes + test cases * Small change in test * Add fixes for 3.14 * Fix setProtocolFactory() (#689) * Remove inconsistency of the index value (#688) * remove the unneccessary code from partitionsOf() (#681) * Added constructors (#672) * 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. * Migration fixed * test fix * test fix * Add the ability to configure the new STR atomically * Fix the returnPartition function (#680) * fix the returnPartition function * minor fix * Remove comment from codebase (#714) * Update interfaces to named parameters (#708) * WIP * Change some values * Make some mappings public for automatic getters * Put back truffle version * updated transfer manager results (#693) * Allow Custom oracle in USDTieredSTO (#691) * Added custom oracles to USDTSTO * Updated custom oracles logic * Pinned solidity version (#675) * Pinned solidity version * Fix merge conflicts * minor transfer optimization (#668) * Fix the BalanceOfPartition audit item (#679) * fix balance of partition function * fix balance of Partition function * return balance in the parent implementation of the getTokensByPartition() * Extra Items (#702) * minor improvements * permission fixes * Synchronise the ISTR with STR (#682) * synchronise the ISTR with STR * fix the interface problem * minor fixes * add some function in interface * add comment in STR * consistency in interfaces of contracts * improve the st interface * remove shadow declaration * add #706 new function declartion in the ISTR * add missing functions in interface * Specific contract type used (#683) * specific contract type used * add more type contract * remove the redundant KindMath * remove unnecessary casting * Merge fixes * Fix PLCR Proposal inconsistency (#713) * fix the proposal * minor styling and comments addition * Fix contract size issue. * Update licenses - Apache 2.0 As per https://spdx.org/licenses/ * Fix canTransfer spec (#709) * Fix spec * Fix conflict * Fix merge conflict * Small fixes * Revert package.json change * Fix test case * Set solcjs as default compiler (#716) * Set solcjs as default compiler If you want to use native solc, set the environemnt variable POLYMATH_NATIVE_SOLC as true. * Removed native solc from travis * Removed native solc version query from travis * CLI common selectToken inlcuding tokensByDelegate * Merge Dev-3.0.0 interfaces * Fix underflow in BlacklistTM (#721) * CLI Refresh security token * CLI Permissions updated * CLI Provider validator regex * Update changelog after the audit fixes * Fixed errors * Update CHANGELOG.md * Update CHANGELOG.md * Update CHANGELOG.md * Issue with forceburn wording and no controllerredeem listed * CLI _owner issue fixed * CLI NewSecurityToken event renamed * Revert "CLI NewSecurityToken event renamed" This reverts commit 4a7fbbe. * CLI ST20 granularity support * Verify transfer on each TM Show which module failed in a TransferFailure Messages improved * Protocol upgrade fixes (#733) * Update tags, types, lower & upper bounds (#725) * Make useModule backwards compatible * Make deployToken backwards compatible (#726) * Make deployToken backwards compatible * Small fix for scheduledCheckpoint * Updated STR interface * Removed extra event * Fix interface mismatch * cleaning up as per the audit recommendation (#722) * TakeFee was removed. * ISTR script (#731) * Added Interface sync script * Added interface check script to CI * return 1 on error * Use local truffle in scripts * Removed npx dependency * minor fixes * CLI Permission manager refactoring * Fixed CTM verifyTransfer bug (#736) * Fixed CTM canTransferBug * Added test case * Added comment for devs * Move some variables / functions to internal (#737) Move some variables / functions to internal * CLI Fix * Minor fix * CLI Wallet manager * Added VEW to migrations * CLI VEW schedules in batches * Styling * Added note for valid templates * Added wallet modules to ST manager * Fixed bug at checking balance * fix: getTreasuryWallet function added to DividendCheckpoint.sol * Minor fixes * Update with v3 contract addresses * Add files via upload * Update README.md

Fix custom modules

b3ecdec

adamdossa changed the title ~~Fix custom modules~~ [3.5, 3.7, 3.8] Fix custom modules May 28, 2019

Remove unnecessary modifier

d5c8c35

maxsam4 reviewed May 29, 2019

View reviewed changes

adamdossa added 2 commits May 29, 2019 07:20

Merge branch 'dev-3.0.0' into Custom-Module-Fixes

61e2460

Fix some test cases

ea913b9

maxsam4 mentioned this pull request May 31, 2019

Remove the custom module logic. #677

Closed

maxsam4 approved these changes Jun 12, 2019

View reviewed changes

maxsam4 changed the base branch from dev-3.0.0 to audit-fixes June 12, 2019 07:50

maxsam4 merged commit 10c1d62 into audit-fixes Jun 12, 2019

maxsam4 deleted the Custom-Module-Fixes branch June 12, 2019 07:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.5, 3.7, 3.8] Fix custom modules #698

[3.5, 3.7, 3.8] Fix custom modules #698

adamdossa commented May 28, 2019 •

edited

Loading

maxsam4 left a comment •

edited

Loading

maxsam4 May 29, 2019

adamdossa May 29, 2019

maxsam4 May 29, 2019

maxsam4 May 29, 2019

adamdossa May 29, 2019

maxsam4 May 29, 2019

adamdossa May 29, 2019

maxsam4 May 29, 2019

maxsam4 May 29, 2019

adamdossa May 29, 2019

maxsam4 May 29, 2019

adamdossa May 29, 2019

maxsam4 May 29, 2019

coveralls commented May 29, 2019

tintinweb commented Jun 11, 2019

[3.5, 3.7, 3.8] Fix custom modules #698

[3.5, 3.7, 3.8] Fix custom modules #698

Conversation

adamdossa commented May 28, 2019 • edited Loading

maxsam4 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 29, 2019

tintinweb commented Jun 11, 2019

adamdossa commented May 28, 2019 •

edited

Loading

maxsam4 left a comment •

edited

Loading