Skip to content
This repository has been archived by the owner on Jul 20, 2023. It is now read-only.

Consensys/polymath-audit-report-2019-04

Repository files navigation

Polymath-Core Audit

1 Summary

ConsenSys Diligence conducted a security audit on the Polymath Core smart contracts that comprise a system for launching regulatory-compliant securities tokens on a decentralized blockchain.

1.1 Audit Dashboard


Audit Details

Number of issues by severity

Minor Medium Major Critical
Open 0 0 0 0
Closed 20 3 7 4

1.2 Audit Goals

The focus of the audit was to verify that the smart contract system is secure, resilient and working according to its specifications. The audit activities can be grouped in the following three categories:

Security: Identifying security related issues within each contract and within the system of contracts.

Sound Architecture: Evaluation of the architecture of this system through the lens of established smart contract best practices and general software best practices.

Code Correctness and Quality: A full review of the contract source code. The primary areas of focus include:

  • Correctness
  • Readability
  • Sections of code with high complexity
  • Improving scalability
  • Quantity and quality of test coverage

1.3 System Overview

Documentation

The following documentation was available to the audit team:

Detail

This section outlines the top-level contracts and libraries of the system. A summary of the inheritance structure for the main component groups is given in the Design section. A complete view of the inheritance structure can be found here (dot). The complete call-graph is too large to be rendered as an image and is therefore only provided as a graphviz dot file.

Registries
  • PolymathRegistry
  • FeatureRegistry
  • ModuleRegistry and ModuleRegistryProxy
  • SecurityTokenRegistry, STRGetter and SecurityTokenRegistryProxy
Oracle Interfaces
  • MakerDAOOracle
  • StableOracle
  • PolyOracle
SecurityToken
  • SecurityTokenFactory (STFactory)
    • SecurityToken, STGetter and SecurityTokenProxy
Modules
CheckPoint
  • ERC20DividendCheckpoint, -Factory and -Proxy
  • EtherDividendCheckpoint, -Factory and -Proxy
  • WeightedVoteCheckpoint, -Factory and -Proxy
  • PLCRVotingCheckpoint, -Factory and -Proxy
SecurityTokenOffering (STO)
  • CappedSTO, -Factory, -Storage and -Proxy
  • USDTieredSTO, -Factory, -Storage and -Proxy
  • PresaleSTO, -Factory, -Storage and -Proxy
TransferManager
  • BlacklistTransferManager, -Factory, -Storage and -Proxy
  • CountTransferManager, -Factory, -Storage and -Proxy
  • GeneralTransferManager, -Factory, -Storage and -Proxy
  • LockupTransferManager, -Factory, -Storage and -Proxy
  • ManualApprovalTransferManager, -Factory, -Storage and -Proxy
  • PercentageTransferManager, -Factory, -Storage and -Proxy
  • VolumeRestrictionTransferManager, -Factory, -Storage and -Proxy
Wallet
  • VestingEscrowWallet, -Factory, -Storage and -Proxy
PermissionManager
  • GeneralPermissionManager, -Factory, -Storage and -Proxy

Scope

The scope of the audit was defined as the Solidity code in the ./contracts/ folder except

  • Experimental modules (contracts/modules/Experimental)
  • Migrations
  • Mocks (contracts/mocks)
  • External (contracts/external)
  • Helpers (contracts/helpers)

The list of files and their hashes can be found in Appendix 1 - File Hashes.

In addition to the v3.0.0 audit the team also reviewed the three v2.2.0 modules to match their v3.0.0 counterparts. Details can be found in the section Business Logic Review v2.2.0 to v3.0.0 Modules.

Design

The system is a framework to launch and manage security tokens. An end user typically creates a new security token by registering a token symbol and deploying a security token contract. A security token can make use of one or more modules that specify the behavior of the token.

The system consists of three major component groups:

  • Registries

    inheritance(dot)

    Registries are administered and deployed by Polymath and form the backbone of the system. There are four Registries:

    • SecurityTokenRegistry

      The SecurityTokenRegistry is the main entry point to the protocol and allows token issuers to register symbols and generate new security tokens. It keeps track of all security tokens and links issuers to security tokens and the symbols they reserved.

      The SecurityTokenRegistry is proxied and can be upgraded. Ownership remains with Polymath.

      Main Functionality:

      • manage symbols: create, modify, and remove tickers and transfer their ownership
      • manage security tokens: generate and modify security tokens
      • manage fees: handle fees for ticker registration and security token launch
    • ModuleRegistry

      The ModuleRegistry keeps track of all registered modules that are available to security tokens and provides access to the module factories.

      The ModuleRegistry is proxied and can be upgraded. Ownership remains with Polymath.

      Main Functionality:

      • manage modules: register, remove, verify, unverify, and access modules
    • FeatureRegistry

      The FeatureRegistry supports enabling and disabling features in the system. It maps a feature name to a boolean value indicating whether that feature is enabled. In the audited code, the only feature tracked is called customModulesAllowed. This feature is checked by the ModuleRegistry to see whether custom modules (those not managed by Polymath) should be allowed.

      The FeatureRegistry is not proxied and cannot be upgraded. Ownership remains with Polymath.

      Main Functionality:

      • manage features: globally enable and disable features
    • PolymathRegistry

      The PolymathRegistry is the main registry that keeps track of core components of the system including the addresses of the PolyToken (e.g. used in the fee system), the ModuleRegistry, the FeatureRegistry, and the SecurityTokenRegistry.

      The PolymathRegistry is not proxied and cannot be upgraded. Ownership remains with Polymath.

      Main Functionality:

      • manage core components: set and change the address of core components.

    (dot)

  • SecurityToken

    inheritance(dot)

    This is the central component of the system which implements ERC 1400, also known as the Security Token Standard, which is comprised of a group of standards that aim to create a representation of securities on the Ethereum blockchain. It is compatible with ERC 20 and ERC 777 and includes:

    Security Tokens can have modules attached to them. Modules control the Security Tokens behavior and might have special capabilities or permissions in the system.

    To issue a new SecurityToken an issuer interfaces with the SecurityTokenRegistry to register a symbol and generate a new security token. The SecurityTokenRegistry defines which factory (STFactory) is used to deploy a new SecurityTokenProxy linked to a specific implementation of the SecurityToken. The SecurityTokenFactory by default adds a data store and default transfer manager module (unless no defaults are set) and sets the treasury wallet address for the security token. The issuer may then attach other modules to the security token.

    The SecurityToken is proxied and can be upgraded.
    Ownership of the SecurityTokenFactory (STFactory) and logic contracts (SecurityToken implementation) remains with Polymath. Ownership of the security token (SecurityTokenProxy) is finally transferred to the issuer.

    Main Functionality:

    • manage modules: add, remove, archive, unarchive, and upgrade modules
    • manage the security token: update token details and name, upgrade the token
    • manage security token transfers: transfer, freeze, and unfreeze transfers

    (dot)

  • Modules

    Modules control the security token's behavior and can have special capabilities or permissions in the system.

    The system comes with five types of modules:

    • TransferManager

      inheritance(dot)

      A transfer manager controls the transfer of tokens. The following variants of transfer managers are provided by Polymath:

      • BlacklistTransferManager - prevent certain investors from transferring tokens during issuer-defined blackout periods
      • CountTransferManager - only allow up to a specified number of token holders
      • GeneralTransferManager - enforces whitelists, KYC, and other restrictions
      • LockupTransferManager - specify named lockup periods for certain investors
      • ManualApprovalTransferManager - transfers can only be made if pre-approved by token admin
      • PercentageTransferManager - limit the percentage of total tokens that can be owned by a single account
      • VolumeRestrictedTransferManager - limit the number of tokens that can be owned by a single account

      (dot)

    • PermissionManager

      inheritance(dot)

      The follow variants of permission managers are provided by Polymath:

      • GeneralPermissionManager - allows the issuer to add wallets as delegates and gives permissions to use restricted functions on other modules
    • CheckPoint

      inheritance(dot)

      CheckPoint modules keep track of and make use of historical token balances.

      • DividendCheckpoint Modules

        Creates checkpoint when dividends are paid so token holders are paid proportionally according to the token balance they held at the time the dividend was created.

        The system provides two variants of DividendCheckpoint modules: An ERC20DividendCheckpoint and an EtherDividendCheckpoint.

      • VotingCheckpoint Modules

        Allow the token owner to create ballots for voting, where voting weights are determined by the token balances of each voter at the time the vote was created.

        • WeightedVoteCheckpoint

          Basic weighted vote checkpoint module.

        • PLCRVotingCheckpoint

          The partial-lock commit/reveal (PLCR) vote uses a commit/reveal scheme to keep votes private during the voting period. Votes are revealed when voting has completed.

      (dot)

    • SecurityTokenOffering

      inheritance(dot)

      SecurityTokenOffering modules allow security tokens to be issued in return for investment in various currencies (ETH, POLY and USD stable coin)

      • PresalesSTO - private presales

      • USDTieredSTO - price of token is specified in US dollars, with multiple price tiers

      • CappedSTO - sets a limit on the funding that is accepted and tokens that will be distributed

      (dot)

    • Wallet

      inheritance(dot)

      The system provides a VestingEscrowWallet by default that allows approved staff to create a token vesting schedule for employees/affiliates.

      (dot)

  • Oracles

    inheritance(dot)

    Oracles are a trusted entity in the system. Polymath wraps the oracles in order to allow a manual price override or to freeze the price. For example, oracles are used with STO modules to convert USD prices into POLY or ether.

    (dot)

1.4 Key Observations/Recommendations

Positive observations

  • Best practices are followed where they make sense.
  • Common design patterns are used where required instead of inventing new interfaces and designs.
  • The token is following the ERC1400 Security Token Standard.
  • An extensive test-suite with 1544 individual tests is provided with the framework.
  • Arithmetic operations are performed using SafeMath.
  • Smart contracts are split into multiple files, most of them being comprised of an interface and an implementation. Common functionality is provided by library contracts.
  • Inline documentation for methods and classes is available and describes functionality and input parameters.
  • The codebase did not yield compiler warnings that were in scope for this audit.
  • The project provided easy to use scripts (npm/package.json) for flattening relevant contracts, executing the test-suite and generating coverage statistics.

Opportunities for improvement

  • Test coverage is incomplete. Any contract system that is used on the main net should have as a minimum requirement a 100% test coverage especially with a risk profile of a Security Token Network. In particular it's useful to include negative test cases ensuring that undesirable changes are detected early and kept in-line with the specification. Security focused test cases verifying that permissions are set-up according to the security-specification and trust-model should be consistently implemented. Methods that require special permissions must be tightly covered. None of the contracts that comprise the contract system should be exempt from testing/coverage. Please not that having a 5% rate of uncovered statements can easily mean that a couple of hundred statements are uncovered for large projects, potentially including critical code paths as well.
  • High Complexity. A multiple library/contract system is complex in nature. The Polymath system consists of a high number of individual contracts that need to be deployed and maintained. The inter-contract communication rate is high. The inheritance model of the system is complex.
  • The projects npm run flatten-all script does not flatten all relevant modules.
  • Improve the Documentation. Inconsistencies exist between the specification documents provided during the audit as well as the projects wiki.

It is recommended to fix all the issues listed in the following chapters, at the very least the ones with severity Critical, Major and Medium.

Individual issues are listed below, but it's important to note that the contract system is quite large, with complex proxying, upgradeability mechanisms, exponential combinations of modules, and insufficient testing. This means that there are likely vulnerabilities in the system that our team did not find. In addition to existing hidden vulnerabilities, new modules or upgrades to existing components could easily create new vulnerabilities. Fortunately, the administrative control retained by Polymath and token issuers will make it possible to recover from many types of vulnerabilities once discovered.

2 Issue Overview

The following table contains all the issues discovered during the audit. The issues are ordered based on their severity. More detailed description on the levels of severity can be found in the Severity Definitions Appendix. The table also contains the status of any discovered issue.

Chapter Issue Title Issue Status Severity
3.1 SecurityToken contract should always be initialized Closed Critical
3.2 Unpredictable behavior due to front running or general bad timing Closed Critical
3.3 Polymath can arbitrarily change prices during a USDTieredSTO token sale Closed Critical
3.4 No new ST can be created after implementation upgrade Closed Critical
3.5 ModuleRegistry - Custom modules can block their own removal Closed Major
3.6 Transfer decisions by default should be consistent Closed Major
3.7 ModuleRegistry - Custom module verification should be bounded to module version Closed Major
3.8 ModuleRegistry - Risks of allowing custom modules in the system Closed Major
3.9 _balanceOfByPartition function returns wrong value Closed Major
3.10 _returnPartition function of SecurityToken always returns UNLOCKED partition Closed Major
3.11 partitionsOf function always returns empty array Closed Major
3.12 Security token upgrade info should be editable Closed Medium
3.13 VestingEscrowWallet - Integer Underflow and unchecked array access in pushAvailableTokensMulti() Closed Medium
3.14 SecurityTokenRegistry does not inherit from ISecurityTokenRegistry Closed Medium
3.15 Different implementations for the same modifier whenNotPausedOrOwner Closed Minor
3.16 SecurityToken - security token name change may cause inconsistency Closed Minor
3.17 Where possible, a specific contract type should be used rather than address Closed Minor
3.18 SecurityToken - Missing Input Validation changeName Closed Minor
3.19 deleteDelegate should be implemented without array iteration in permission manager Closed Minor
3.20 Improve Code Reusability - Use const variables instead of literals for EIP-1066 status codes. Closed Minor
3.21 TokenLib - Unused Import KindMath Closed Minor
3.22 Module - authentication modifier onlyFactoryOwner and onlyFactoryOrOwner are never used Closed Minor
3.23 SecurityToken - authentication modifier onlyTokenFactory is never used Closed Minor
3.24 SecurityToken/STGetter (ERC1643) getAllDocuments - can be cause gas or memory issues if used Closed Minor
3.25 PolyToken - events are redefined in the implementation Closed Minor
3.26 Use latest stable version of Solidity Closed Minor
3.27 OwnedProxy should be removed Closed Minor
3.28 Module.takeUsageFee() allows admins to drain a module's approved POLY tokens Closed Minor
3.29 Redundant pause and unpause functions in TransferManager Closed Minor
3.30 Modules shouldn't be casted to IBoot Closed Minor
3.31 public functions in TokenLib could be external instead Closed Minor
3.32 EtherDividendCheckpoint and ERC20DividendCheckpoint division by zero Closed Minor
3.33 SecurityToken / STGetter storage layout is hard to maintain Closed Minor
3.34 LockUpTransferManager._removeLockUpFromUser checks for an impossible condition Closed Minor

3 Issue Details

3.1 SecurityToken contract should always be initialized


Severity: Critical

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#672.

NB - we also fixed this in the SecurityTokenRegistry which had a similar issue.


Description

All security tokens are proxies that delegate all calls to SecurityToken implementation contract deployed by the Polymath. Since SecurityToken is a proxy itself, it's possible to delete the SecurityToken contract if it delegates calls to a malicious contract with selfdestruct. Because of that, it might be possible to use Create2 to replace the implementation with a malicious contract and steal tokens. Create2 attack can only be done by polymath, but anyone else can just delete the SecurityToken and break all the tokens irrevocably.

Remediation

Ensure initialization of the SecurityToken implementation contract with the safe getterDelegate. That prevents from creating an opportunity to destroy the contract or to use it maliciously. For example, it should be enough to add initialized = true; to SecurityToken constructor.

3.2 Unpredictable behavior due to front running or general bad timing


Severity: Critical

Status: Closed

Resolution: This issue has been addressed with the following comment:

PolymathRegistry / SecurityToken.updateFromRegistry: We have left this alone. In practice we don't expect issuers to need to call this (as all registries can be updated in place) and if they do, they would trigger it, not us.

New modules: We have left the logic as it is. If an issuer is concerned about this risk they can add the new module with _archived set to true which would add the module but not activate it. They can then check the details of the module added before activating it. They can also specify an upper bound on the module fee to avoid any front-running to change the fee.

New tokens: We have modified setProtocolFactory() to not allow changing the factory associated with an existing version. An issuer can now specify an exact version to deploy and we can't modify that version by front-running their transaction: PolymathNetwork/polymath-core#689

In general we will use the CD audit report to generate a transparency document specifying the areas that Polymath has control in, and the areas that we don't. The guiding principle is that, until the point that a token is created, Polymath has a good amount of control (e.g. through front-running an upgrade to an STFactory token) but once the token has been deployed we have no unilateral control (with certain documented exceptions, specifically that we can render the module registry inactive and have some initial access over the oracle pricing for STOs (see 3.3)).


Description

In a large number of cases, Polymath can update or upgrade things in the system without warning. This has the potential to violate a security goal of the system: once a security token has been created, Polymath shouldn't be able to interfere with that token's functioning.

Specifically, Polymath could use front running to make malicious changes just ahead of incoming transactions, or purely accidental negative effects could occur due to unfortunate timing of changes.

Some instances of this are more important than others, but in general token issuers should have assurances about the behavior of the action they're about to take.

Examples

The PolymathRegistry is used to store addresses for a wide variety of things, including oracles, the POLY token, security token registry, module registry, etc. By front running a call to SecurityToken.updateFromRegistry, Polymath could get a security token to use arbitrary addresses for the module registry, security token registry, and POLY token. In general, any call to PolymathRegistry.getAddress is vulnerable to front running.

When adding a new module to a token, the latest version is used from the module token registry. By front running a call to add a module to an existing security token, Polymath could add a new version of a module and cause the new code to be added to the existing security token. If the module is of the right type, e.g. one that allows minting and/or burning tokens, this could allow for direct theft of valuable assets.

Similarly, when deploying a new security token, the latest version is used from the security token registry. Someone creating a token thus has no way to be sure what version of the code they'll receive when they deploy a new token.

Remediation

The underlying issue is that users of the system can't be sure what the behavior of a function call will be, and this is because the behavior can change at any time.

There are two broad strategies for addressing this:

  1. Let the user lock things down from changing. For example, allow them to specify what version of a module or token contract they expect, and if that version is no longer current by the time the transaction is processed, revert.
  2. Give the user advance notice of changes with a time lock. For example, make all upgrades require two steps with a mandatory time window between them. The first step merely broadcasts to users that a particular change is coming, and the second step commits that change after a suitable waiting period.

Different aspects of the system may call for different specific remediations.

References

See also issue 3.3, which discusses the danger of oracle manipulation in particular.

3.3 Polymath can arbitrarily change prices during a USDTieredSTO token sale


Severity: Critical

Status: Closed

Resolution: This issue has been addressed with PolymathNetwork/polymath-core#691.

We have added a function modifyOracle that allows an issuer to take control of their own oracles if they wish to. The default is to use the Polymath "controlled" oracles. This follows the pattern of defaulting to have a certain amount of trust in Polymath but allowing an issuer to remove this if required.


Description

The USDTieredSTO token sale uses oracles to convert from USD to ETH or POLY. The oracle addresses come from the PolymathRegistry, which means that Polymath has the ability to swap out those oracles at any time. This enables Polymath to set an arbitrary price for themselves to participate in an STO, including a price of 0.

Remediation

We recommend putting time locks in place. It shouldn't be possible to replace the oracle implementations or to put them into manual override mode without announcing the change first and waiting a reasonable amount of time. This gives token issuers an opportunity to shut down any ongoing token sales before the oracle changes take effect.

3.4 No new ST can be created after implementation upgrade


Severity: Critical

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#694 (also see issue 3.12)


Description

Polymath should be able to upgrade SecurityToken implementation by creating a new security token contract. After that, a link to the new version should be set in STFactory contract by calling setLogicContract function.

code/contracts/tokens/STFactory.sol:L139-L148

function setLogicContract(string calldata _version, address _logicContract, bytes calldata _upgradeData) external onlyOwner {
    require(keccak256(abi.encodePacked(_version)) != keccak256(abi.encodePacked(logicContracts[latestUpgrade].version)), "Same version");
    require(_logicContract != logicContracts[latestUpgrade].logicContract, "Same version");
    require(_logicContract != address(0), "Invalid address");
    latestUpgrade++;
    logicContracts[latestUpgrade].version = _version;
    logicContracts[latestUpgrade].logicContract = _logicContract;
    logicContracts[latestUpgrade].upgradeData = _upgradeData;
    emit LogicContractSet(_version, _logicContract, _upgradeData);
}

The problem is that in this function initializationData is not set for the latest version of the logic contract. That means that all new security tokens will not have init data and they will not be initialized and most probably will revert on creation.

Remediation

Add logicContracts[latestUpgrade].initializationData initialisation in setLogicContract and check that initializationData length is at least 4 bytes.

3.5 ModuleRegistry - Custom modules can block their own removal


Severity: Major

Status: Closed

Resolution: closed with PolymathNetwork/polymath-core#698


Description

The module registry is controlled by the module registry owner. Both the registry owner and the module owner may remove modules from the registry. Modules are usually deployed by Polymath but the system also allows the use of custom modules. Custom modules can be enabled in the feature registry. This allows potentially untrusted entities to register modules while remaining in control of the module factory.

code/contracts/ModuleRegistry.sol:L207-L214

function removeModule(address _moduleFactory) external whenNotPausedOrOwner {
    uint256 moduleType = getUintValue(Encoder.getKey("registry", _moduleFactory));

    require(moduleType != 0, "Module factory should be registered");
    require(
        msg.sender == IOwnable(_moduleFactory).owner() || msg.sender == owner(),
        "msg.sender must be the Module Factory owner or registry curator"
    );

The order of the ownership checks in the current implementation give precedence to a potentially untrusted external call to the module factory to check if the sender is the module owner. Modules can therefore intentionally block their own removal by reverting on the external ownership call (IOwnable(_moduleFactory).owner()) . The check if the module registry owner is the sender will not be performed.

Remediation

Make sure the module registry owner can always remove modules from the registry. Avoid external calls to untrusted modules that might intentionally revert the execution. Give precedence to the trusted call and only after that call out to untrusted contracts.

Please note that unverifyModule() can also be blocked in the same way by a malicious module.

References

3.6 Transfer decisions by default should be consistent


Severity: Major

Status: Closed

Resolution: Logic has been modified so that transfers fail if there are no unarchived TMs. Documentation around the behaviour has been added: PolymathNetwork/polymath-core#693 https://github.com/PolymathNetwork/polymath-core/wiki/Transfer-manager-results


Description

code/contracts/tokens/SecurityToken.sol:L661-L696

function _executeTransfer(
    address _from,
    address _to,
    uint256 _value,
    bytes memory _data
)
    internal
    checkGranularity(_value)
    returns(bool)
{
    if (!transfersFrozen) {
        bool isInvalid;
        bool isValid;
        bool isForceValid;
        bool unarchived;
        address module;
        uint256 tmLength = modules[TRANSFER_KEY].length;
        for (uint256 i = 0; i < tmLength; i++) {
            module = modules[TRANSFER_KEY][i];
            if (!modulesToData[module].isArchived) {
                unarchived = true;
                ITransferManager.Result valid = ITransferManager(module).executeTransfer(_from, _to, _value, _data);
                if (valid == ITransferManager.Result.INVALID) {
                    isInvalid = true;
                } else if (valid == ITransferManager.Result.VALID) {
                    isValid = true;
                } else if (valid == ITransferManager.Result.FORCE_VALID) {
                    isForceValid = true;
                }
            }
        }
        // If no unarchived modules, return true by default
        return unarchived ? (isForceValid ? true : (isInvalid ? false : isValid)) : true;
    }
    return false;
}

If there are no active modules then _executeTransfer returns true by default. If there are active modules and all modules say NA transfer should have the same result, but it returns false.

Remediation

Make default decisions consistent. It looks like it makes more sense to make all transfers VALID by default.

3.7 ModuleRegistry - Custom module verification should be bounded to module version


Severity: Major

Status: Closed

Resolution: This issue has been addressed as part of issue 3.8. Module verification is not bound to version.


Description

If custom modules feature is enabled there is a mechanism of registering new custom modules by anyone. These modules can only be used by any other security token if polymath admins verify them. The problem is that verification is only bounded to module factory regardless of module version.

code/contracts/ModuleRegistry.sol:L245

function verifyModule(address _moduleFactory) external onlyOwner {

Once the factory is verified, the factory owner can upgrade a module to a malicious one.

Remediation

Add a module version to verification details and only allow a verified version to be used in security tokens. That would require polymath admins to review every new upgrade in a custom module. As an alternative, polymath admins can just make sure that they only verify module factories that are not upgradable.

3.8 ModuleRegistry - Risks of allowing custom modules in the system


Severity: Major

Status: Closed

Resolution: The functionality to register custom modules to the system remains available, increases complexity and extends the attack surface. A number of risks described with this issue have been addressed with PolymathNetwork/polymath-core#698.

  • 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 single 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

This leaves the issue that someone could register ~8k new modules and cause loading moduleList to exceed the block gas limit. Whilst this is theoretically possible, in practice it would cost ~120 ETH and if it were to happen we could simply upgrade the ModuleRegistry to fix the problem. This should be a sufficient disincentive for anyone to attempt this griefing attack.


Description

The global setting customModulesAllowed is maintained in the FeatureRegistry and controls whether it is allowed to register custom modules in the ModuleRegistry for use with security tokens. By default only the ModuleRegistry owner can register modules. Setting customModulesAllowed opens up registration to anyone for custom modules. This has several security implications.

There are two scenarios in connection with custom modules:

  1. A security token issuer would like to use their custom modules with their own security token (common use-case).

  2. A module developer would like to make their module generally available to other users in the system (marketplace use-case). This requires the module to be verified by Polymath.

Registration

code/contracts/ModuleRegistry.sol:L171-L180

function registerModule(address _moduleFactory) external whenNotPausedOrOwner {
    if (IFeatureRegistry(getAddressValue(FEATURE_REGISTRY)).getFeatureStatus("customModulesAllowed")) {
        require(
            msg.sender == IOwnable(_moduleFactory).owner() || msg.sender == owner(),
            "msg.sender must be the Module Factory owner or registry curator"
        );
    } else {
        require(msg.sender == owner(), "Only owner allowed to register modules");
    }
    require(getUintValue(Encoder.getKey("registry", _moduleFactory)) == 0, "Module factory should not be pre-registered");
  • Anyone (usually a security token owner) can register a moduleFactory by calling registerModule() provided the caller is the owner of the factory.
  • After getting the factories moduleType, the factory is stored in the moduleList. The module is still unverified and can only be used by a security token owner that owns the factory. This gives anyone control over the size of the moduleList allowing someone to spam the list and potentially cause DoS conditions in other parts of the system that are iterating over all items in the moduleList (see subsection Listing modules)
  • Note: the method is inefficient as it performs multiple external calls instead of re-using previously returned values (moduleFactory.types(), IOwnable(_moduleFactory).owner()).
Checking if a module can be used

code/contracts/ModuleRegistry.sol:L132-L140

function useModule(address _moduleFactory, bool _isUpgrade) external {
    if (IFeatureRegistry(getAddressValue(FEATURE_REGISTRY)).getFeatureStatus("customModulesAllowed")) {
        require(
            getBoolValue(Encoder.getKey("verified", _moduleFactory)) || IOwnable(_moduleFactory).owner() == IOwnable(msg.sender).owner(),
            "ModuleFactory must be verified or SecurityToken owner must be ModuleFactory owner"
        );
    } else {
        require(getBoolValue(Encoder.getKey("verified", _moduleFactory)), "ModuleFactory must be verified");
    }
Listing modules

code/contracts/ModuleRegistry.sol:L345-L371

function getModulesByTypeAndToken(uint8 _moduleType, address _securityToken) public view returns(address[] memory) {
    address[] memory _addressList = getArrayAddress(Encoder.getKey("moduleList", uint256(_moduleType)));
    uint256 _len = _addressList.length;
    bool _isCustomModuleAllowed = IFeatureRegistry(getAddressValue(FEATURE_REGISTRY)).getFeatureStatus(
        "customModulesAllowed"
    );
    uint256 counter = 0;
    for (uint256 i = 0; i < _len; i++) {
        if (_isCustomModuleAllowed) {
            if (IOwnable(_addressList[i]).owner() == IOwnable(_securityToken).owner() || getBoolValue(
                Encoder.getKey("verified", _addressList[i])
            )) if (isCompatibleModule(_addressList[i], _securityToken)) counter++;
        } else if (getBoolValue(Encoder.getKey("verified", _addressList[i]))) {
            if (isCompatibleModule(_addressList[i], _securityToken)) counter++;
        }
    }
    address[] memory _tempArray = new address[](counter);
    counter = 0;
    for (uint256 j = 0; j < _len; j++) {
        if (_isCustomModuleAllowed) {
            if (IOwnable(_addressList[j]).owner() == IOwnable(_securityToken).owner() || getBoolValue(
                Encoder.getKey("verified", _addressList[j])
            )) {
                if (isCompatibleModule(_addressList[j], _securityToken)) {
                    _tempArray[counter] = _addressList[j];
                    counter++;
                }
  • Modules are organized in typed moduleLists and custom modules are not separated from built-in modules. This opens the possibility for a malicious actor to cause a DoS condition by filling the moduleList with dummy factories resulting in an out of gas condition while iterating the list.
  • Even more straight forward a malicious module could just revert in the call to IOwnable(_addressList[i]).owner() causing the method to always fail.
  • The check whether a module is verified should be performed first to allow a shortcut that is avoiding the external call checking whether the module factory is owned by the security token owner. Note that the external call may intentionally fail (issue 3.5).

code/contracts/ModuleRegistry.sol:L354-L355

if (IOwnable(_addressList[i]).owner() == IOwnable(_securityToken).owner() || getBoolValue(
    Encoder.getKey("verified", _addressList[i])

Remediation

There is considerable risk in allowing untrusted parties to provide and maintain custom modules in the system. Untrusted modules can be registered by anyone, are not separated from built-in modules and may therefore interfere with the system by causing Out-of-Gas DoS conditions, unexpected reverts or open up a potential for reentrancy attacks.

It is therefore recommended to assess whether this functionality is actually being used or required in the system. The general advice is to remove any functionality that is not actively used or still in development. To safely allow the use of custom modules it is recommended to rework the current code based on the changing trust assumptions.

  • A custom module can only be used if the moduleFactory owner is the security token owner, or the moduleFactory is verified by Polymath. In order to get a module verified a manual step is required. In any case this step must include a thorough security inspection of the module to make sure that the moduleFactory and module implementations adhere to the interfaces provided by Polymath and to verify that the moduleFactory has no malicious intent, i.e. none of the module components can be updated after the verification has been performed.
  • Trust assumptions are changing due to the external party maintaining full control of the module and its components (factory, implementation).
  • A verification should always be bound to a specific version of a module (issue 3.7)
  • Custom modules should be separated from built-in modules and the code should be refactored to make sure the existence of custom modules does not interfere with users relying on built-in modules (e.g. DoS/Out-of-Gas conditions).
  • Always make sure that the registry owner is able to maintain and administer the registry and cannot be blocked by an external call (issue 3.5)
  • Guard for potential reentrancy attacks.

3.9 _balanceOfByPartition function returns wrong value


Severity: Major

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#679.

// In UNLOCKED partition we are returning the minimum of all the unlocked balances // In locked partition we are returning the maximum of all the Locked balances


Description

code/contracts/tokens/SecurityToken.sol:L490

function _balanceOfByPartition(bytes32 _partition, address _tokenHolder, uint256 _additionalBalance) internal view returns(uint256 max) {

This function returns the maximum of the balances from all transfer managers. It should not be the case for UNLOCKED partition for example, because GTM may return all balance, while LTM can have half of the balance locked. In the result _balanceOfByPartition(UNLOCKED) will return full balance, but only half of it can actually be transferred.

Remediation

UNLOCKED partition balance should be the minimum of all balances from all transfer managers. There is an issue with that solution because some transfer managers do not implement this function properly and just return 0. That needs to be fixed by implementing getTokensByPartition function in every TM or adding a flag that shows whether this function is implemented.

LOCKED partition balances should remain the same (maximum).

3.10 _returnPartition function of SecurityToken always returns UNLOCKED partition


Severity: Major

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#680


Description

code/contracts/tokens/SecurityToken.sol:L538-L545

function _returnPartition(uint256 _beforeBalance, uint256 _afterBalance, uint256 _value) internal pure returns(bytes32 toPartition) {
    // return LOCKED only when the transaction `_value` should be equal to the change in the LOCKED partition
    // balance otherwise return UNLOCKED
    if (_afterBalance.sub(_beforeBalance) == _value)
        toPartition = LOCKED;
    // Returning the same partition UNLOCKED
    toPartition = UNLOCKED;
}

It's commented in the function that it should return LOCKED under some circumstances. Function _returnPartition always returns UNLOCKED partition. Additionally, the comment says that LOCKED should be returned when a change in the locked partition equals value but it actually checks the change in _partition which is usually UNLOCKED

Remediation

Review what actually should be returned here. If the initial intention is correct, actually check LOCKED partition balance change and return correct partition.

3.11 partitionsOf function always returns empty array


Severity: Major

Status: Closed

Resolution: addressed with PolymathNetwork/polymath-core#681 by hard-coding the partitions that are supported in 3.0.


Description

partitionsOf is a function inside STGetter contract that should return a list of the partitions for a particular token holder. _appendPartition function is used inside partitionsOf to append a new partition to the partitions array in case it's not already there. Instead, it does the opposite and only appends partitions that are already in the partitions array. Eventually, that results in the fact that no partitions are added and partitionsOf always returns an empty array.

Remediation

code/contracts/tokens/STGetter.sol:L279

if (duplicate) {

Change duplicate to !duplicate in if statement.

3.12 Security token upgrade info should be editable


Severity: Medium

Status: Closed

Resolution: This issue has been addressed with PolymathNetwork/polymath-core#694 (see issue 3.4) by adding functionality to edit upgrade info. Front-running opportunity remains.


Description

Polymath admins can upgrade security token implementation. In order to do so, they submit an address of a new token implementation and upgradeData for initialization. If they do it wrong, there is no way to edit that data and all tokens of lower versions will become non-upgradable.

Remediation

Allow editing security token upgrade information. It's important to avoid adding frontrunning opportunities while doing that.

3.13 VestingEscrowWallet - Integer Underflow and unchecked array access in pushAvailableTokensMulti()


Severity: Medium

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#673.


Description

pushAvailableTokensMulti() attempts to check if the argument _toIndex is within bounds of beneficiaries[]. However, the check already underflows at line 436 if beneficiaries is empty allowing the caller (withPerm(OPERATOR)) to provide any value for _toIndex that is smaller or equal to UINT256_MAX. The failed check may also lead to an invalid array access at line 438 causing an exception.

code/contracts/modules/Wallet/VestingEscrowWallet.sol:L435-L441

function pushAvailableTokensMulti(uint256 _fromIndex, uint256 _toIndex) public withPerm(OPERATOR) {
    require(_toIndex <= beneficiaries.length - 1, "Array out of bound");
    for (uint256 i = _fromIndex; i <= _toIndex; i++) {
        if (schedules[beneficiaries[i]].length !=0)
            pushAvailableTokens(beneficiaries[i]);
    }
}

Remediation

A check for _toIndex < beneficiaries.length is sufficient. Optionally also add a check to require _fromIndex <= toIndex to avoid that the method silently returns without performing an action on invalid input.

References

3.14 SecurityTokenRegistry does not inherit from ISecurityTokenRegistry


Severity: Medium

Status: Closed

Resolution: This issue has been addressed with PolymathNetwork/polymath-core#682 updating the mismatching interface but not inheriting from it (due to some functions living in the proxied STRGetter). Recommendation: Add dev-note to SecurityTokenRegistry reminding dev to update interface as it is not automatically enforced.

We did not inherit ISTR from STR as this doesn't quite work as some of the functions in ISTR live in the STRGetter (due to contract size limitations). We did review and align the functions between these contracts though in: PolymathNetwork/polymath-core#682.


Description

SecurityTokenRegistry.sol imports ./interfaces/ISecurityTokenRegistry.sol but it is not used in the code.

code/contracts/SecurityTokenRegistry.sol:L7

import "./interfaces/ISecurityTokenRegistry.sol";

Remediation

SecurityTokenRegistry should inherit from ISecurityTokenRegistry to make sure it is an implementation compliant to the interface.

3.15 Different implementations for the same modifier whenNotPausedOrOwner


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#674.


Description

ModuleRegistry and SecurityTokenRegistry provide different implementations for the same modifier.

code/contracts/ModuleRegistry.sol:L82-L88

modifier whenNotPausedOrOwner() {
    if (msg.sender == owner()) _;
    else {
        require(!isPaused(), "Already paused");
        _;
    }
}

code/contracts/SecurityTokenRegistry.sol:L168-L176

modifier whenNotPausedOrOwner() {
    _whenNotPausedOrOwner();
    _;
}

function _whenNotPausedOrOwner() internal view {
    if (msg.sender != owner())
        require(!isPaused(), "Paused");
}

Remediation

Stick to one variant of the implementation to improve code maintainability.

3.16 SecurityToken - security token name change may cause inconsistency


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#706.

We modified the code so that token name is only stored on the ST itself, and the STR retrieves the details from the ST directly.

To retain backwards compatibility, if there is a name stored in the STR it is returned.


Description

When generating registering a ticker and generating a security token a name for the security token is specified. The token name is stored in the security token registry while registering the ticker name (_storeTickerDetails(), registeredTickers_tokenName<TokenName>). However, an issuer can unilaterally change the name of the token stored in the security token (see related issue issue 3.18). This update is not reflected in the security token registry causing an inconsistency with the security token.

code/contracts/SecurityTokenRegistry.sol:L462-L482

function _storeTickerDetails(
    string memory _ticker,
    address _owner,
    uint256 _registrationDate,
    uint256 _expiryDate,
    string memory _tokenName,
    bool _status
)
    internal
{
    bytes32 key = Encoder.getKey("registeredTickers_owner", _ticker);
    set(key, _owner);
    key = Encoder.getKey("registeredTickers_registrationDate", _ticker);
    set(key, _registrationDate);
    key = Encoder.getKey("registeredTickers_expiryDate", _ticker);
    set(key, _expiryDate);
    key = Encoder.getKey("registeredTickers_tokenName", _ticker);
    set(key, _tokenName);
    key = Encoder.getKey("registeredTickers_status", _ticker);
    set(key, _status);
}

Remediation

Indicate that the security token name stored in the registry is the initial token name or consider to disallow security token owners to change the name of their token.

3.17 Where possible, a specific contract type should be used rather than address


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#683.


Description

Rather than storing addresses and then casting to the known contract type, it's better to use the best type available so the compiler can check for type safety.

Examples

ModuleStorage.securityToken is of type address, but it could be type ISecurityToken instead. Not only would this give a little more type safety when deploying new modules, but it would avoid repeated casts throughout the codebase of the form ISecurityToken(securityToken).

Remediation

Where possible, use more specific types instead of address. This goes for parameter types as well as state variable types.

3.18 SecurityToken - Missing Input Validation changeName


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#695.


Description

The security token name is initially set and validated when calling SecurityTokenRegistry.generateSecurityToken(). After the new security token proxy has been deployed ownership is transferred to the issuer. The issuer can then call changeName on the security token to set a new name without enforcing any restrictions.

SecurityTokenRegistry initial deployment and input validation

code/contracts/SecurityTokenRegistry.sol:L564-L575

function generateNewSecurityToken(
    string memory _name,
    string memory _ticker,
    string memory _tokenDetails,
    bool _divisible,
    address _treasuryWallet,
    uint256 _protocolVersion
)
    public
    whenNotPausedOrOwner
{
    require(bytes(_name).length > 0 && bytes(_ticker).length > 0, "Bad ticker");

SecurityToken - no input validation

code/contracts/tokens/SecurityToken.sol:L361-L365

function changeName(string calldata _name) external {
    _onlyOwner();
    emit UpdateTokenName(name, _name);
    name = _name;
}

Remediation

Make sure to enforce consistent input validation while deploying a new security token, but also when allowing an issuer to change certain variables.

3.19 deleteDelegate should be implemented without array iteration in permission manager


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#696.

The approach taken here was to leave the array iteration, but modify deleteDelegate so that it can be used to prune the list of delegates without iterating through the entire list.

Since delegates are set by the issuer, and a reasonable number of delegates is 1 - 5, this seems a reasonable fix to us.


Description

deleteDelegate function of GeneralPermissionManager contract iterates over all delegates to find the right one. It might become impossible to delete a delegate because of the array size (though it's unlikely, it still becomes more expensive). There should be a more efficient way to remove a delegate.

Remediation

Store indexes of delegates in the array or have a view function to determine the array index and pass that index to the deleteDelegate.

3.20 Improve Code Reusability - Use const variables instead of literals for EIP-1066 status codes.


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#690. Severity rating has been downgraded to Minor.


Description

EIP-1066 Status code literals are used throughout the codebase which makes it hard to maintain and understand the codebase.

code/contracts/libraries/TokenLib.sol:L458-L483

function canTransfer(
    bool success,
    bytes32 appCode,
    address to,
    uint256 value,
    uint256 balanceOfFrom
)
    public
    pure
    returns (bool, byte, bytes32)
{
    if (!success)
        return (false, 0x50, appCode);

    if (balanceOfFrom < value)
        return (false, 0x52, bytes32(0));

    if (to == address(0))
        return (false, 0x57, bytes32(0));

    // Balance overflow can never happen due to totalsupply being a uint256 as well
    // else if (!KindMath.checkAdd(balanceOf(_to), _value))
    //     return (false, 0x50, bytes32(0));

    return (true, 0x51, bytes32(0));
}

Remediation

Consider creating a Library/SubContract that exports const EIP-1066 Status Codes with descriptive variable names for use in the codebase.

References

3.21 TokenLib - Unused Import KindMath


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#684.


Description

TokenLib imports ./KindMath.sol but it is never used.

code/contracts/libraries/TokenLib.sol:L10

import "./KindMath.sol";

Remediation

Remove unused imports.

3.22 Module - authentication modifier onlyFactoryOwner and onlyFactoryOrOwner are never used


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#685.


Description

The modifiers onlyFactoryOwner and onlyFactoryOrOwner are defined in Module but never used. This can indicate a lack of authentication control for methods exposed by any of the modules inheriting from Module but can also be a preparation for future modules. The modifiers check if msg.sender is the modules factory owner or msg.sender is the factory calling the method. The modules factory is set in the constructor of ModuleStorage when initializing the module.

code/contracts/modules/Module.sol:L48-L56

modifier onlyFactoryOwner() {
    require(msg.sender == Ownable(factory).owner(), "Sender is not factory owner");
    _;
}

modifier onlyFactoryOrOwner() {
    require((msg.sender == Ownable(securityToken).owner()) || (msg.sender == factory), "Sender is not factory or owner");
    _;
}

code/contracts/storage/modules/ModuleStorage.sol:L28

factory = msg.sender;

Remediation

A general recommendation is to remove unused functionality and re-introduce it when there is actually an implementation that is using it. This makes it easier to understand what the code is actually doing and does not pose a risk of someone assuming certain security checks are in place. Verify all exposed interfaces of contracts inheriting from Module for missing authentication. Remove the modifier if the TokenFactory does not call to any of the modules and no special authentication is required.

3.23 SecurityToken - authentication modifier onlyTokenFactory is never used


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#686.


Description

The modifier onlyTokenFactory is defined in SecurityToken but never used. This can indicate a lack of authentication control for methods exposed by SecurityToken. The modifier checks if msg.sender is the tokenFactory that has been set while initializing the contract.

code/contracts/tokens/SecurityToken.sol:L169-L172

modifier onlyTokenFactory() {
    require(msg.sender == tokenFactory);
    _;
}

Remediation

A general recommendation is to remove unused functionality and re-introduce it when needed. This makes it easier to understand what the code is actually doing and does not pose a risk of someone assuming certain security checks are in place. Verify exposed interfaces of SecurityToken for missing authentication. Remove the modifier if the TokenFactory does not call SecurityToken and no special authentication is required.

3.24 SecurityToken/STGetter (ERC1643) getAllDocuments - can be cause gas or memory issues if used


Severity: Minor

Status: Closed

Resolution: This issue has been addressed with the following statement:

In general, for getter functions which are only expected to be called off-chain, and can therefore have a very high gas limit (beyond the block gas limit) we are comfortable having functions which potentially have high gas usage. We have this pattern in getters (intended only for a dApp) throughout our code.


Description

The function getAllDocuments() can be blocked if too many documents are added. Contracts interacting with it might run out of gas. This can also create problems with other code that relies or tries to read the result value of getAllDocuments().

Because the output is unlimited, this can cause out of gas, memory or execution problems.

This problem can be avoided if there is no code that interacts with this function or the number of documents will always be a low number.

Examples

Calling the getAllDocuments() function has this gas impact with the specified number of documents:

  • 200 documents will use ~100k gas;
  • 1000 documents will use ~370k gas;
  • 1700 documents will use ~650k gas;
  • 5000 documents will use ~2 million gas.

This contract was used to generate the gas costs above:

contract Reader {
    ERC1643 public d;
    constructor(ERC1643 _addr) public {
        d = _addr;
    }
    
    uint256 public count;
    
    function readDocs() public returns (uint256) {
        count = d.getAllDocuments().length;
        return count;
    }
}

Remediation

Because this is part of the standard implementation and the specs, there is no code remediation possible. The advice is to avoid calling the function and only use getDocument().

References

  • Geth provides MaxUint64 / 2 gas when calling view, or constant functions but this was not always the case:

internal/ethapi/api.go:L740

gas := uint64(math.MaxUint64 / 2)

Ganache and Parity have different defaults. Other clients will implement their own defaults.

3.25 PolyToken - events are redefined in the implementation


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#692 by removing PolyToken from the repository.


Description

The PolyToken implementation extends the IPoly interface which defines the ERC20 events (Transfer, Approval) that can be emitted in the contract. Because the events are already defined in the interface, they do not need to be defined again in the implementation.

Defining the events again can lead to mistakes, in case the developer defines a similar but not quite exact event, or confusion, because somebody that reads the contract now needs to check if the specified event in the implementation matches the one defined in the interface.

Having the events defined only in one place reduces confusion and possible mistakes, while increasing code maintainability.

code/contracts/interfaces/IPoly.sol:L17-L18

event Transfer(address indexed from, address indexed to, uint256 value);
event Approval(address indexed owner, address indexed spender, uint256 value);

code/contracts/helpers/PolyToken.sol:L80-L81

event Transfer(address indexed from, address indexed to, uint256 value);
event Approval(address indexed owner, address indexed spender, uint256 value);

Examples

The code when emitting the Boom() event in each case is very similar but the emitted event might not be the expected one.

pragma solidity >0.5.0;

interface MyInterface {
    function explode(bytes calldata _data) external;
    function implode(uint256 _data) external;
    
    event Boom(bytes _data);
}

contract MyImplementation is MyInterface {
    // We define an event with the same name as the one in the interface
    // but a different argument type
    event Boom(uint256 _data); 
    
    /*
    This function emits the MyInterface.Boom(bytes _data) event:
    [
    	{
    		"from": "0xec5bee2dbb67da8757091ad3d9526ba3ed2e2137",
    		"topic": "0xf0525cb4f38ada4b00a047310c294a112a153e48e11a55b7f4b2fe1024405383",
    		"event": "Boom",
    		"args": {
    			"0": "0x1234",
    			"_data": "0x1234",
    			"length": 1
    		}
    	}
    ]
    */
    function explode(bytes memory _data) public {
        emit Boom(_data);
    }
    
    
    /*
    This function emits the MyImplementation.Boom(uint256 _data) event:
    [
    	{
    		"from": "0xec5bee2dbb67da8757091ad3d9526ba3ed2e2137",
    		"topic": "0x1167d8fb28af59d807f1ba2dd442f6a78306131cc03e11970dd6e66c8adc9f57",
    		"event": "Boom",
    		"args": {
    			"0": "4660",
    			"_data": "4660",
    			"length": 1
    		}
    	}
    ]
    */
    function implode(uint256 _data) public {
        emit Boom(_data);        
    }
}

Remediation

Remove the events which are already defined in the interface.

3.26 Use latest stable version of Solidity


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#675.


Description

Solidity is very active in pushing out new versions. Make sure to include the latest stable version that is compatible with your code. The codebase is currently using solidity v0.5.0.

code/flat/FeatureRegistry.sol:L1

pragma solidity ^0.5.0;

Remediation

At the moment of writing, the latest stable version is 0.5.8.

It is a good practice to pin to a specific version. Having a floating version can open your application to newer, possibly not well tested, versions (SWC-103).

// Fixed version
pragma solidity 0.5.8; 

// As opposed to a floating pragma
pragma solidity ^0.5.8;

References

https://github.com/ethereum/solidity/releases

3.27 OwnedProxy should be removed


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#701.


Description

The OwnedProxy contract is only used in the DataStoreProxy, and Proxy would be a better choice there. In general, there's no compelling use case for this contract, as ownership isn't being used for anything.

Remediation

Have DataStoreProxy inherit from Proxy instead and then delete the dead OwnedProxy code.

3.28 Module.takeUsageFee() allows admins to drain a module's approved POLY tokens


Severity: Minor

Status: Closed

Resolution: This issue has been closed by removing the functionality with PolymathNetwork/polymath-core#700.


Description

There are no limits to when and how often Module.takeUsageFee() can be called, and failure to collect has no on-chain consequences. It currently allows any admin to drain a module of approved POLY tokens. It, in fact, prevents modules from properly implementing POLY-based usage fees unless they take care to override this function.

Remediation

Remove Module.takeUsageFee(). If future modules want to collect a fee, they can implement a (safe) version of this function when it is needed.

Optionally, also remove ModuleFactory.usageCost and ModuleFactory.usageCostInPoly(), which will be completely unused if takeUsageFee() is removed.

3.29 Redundant pause and unpause functions in TransferManager


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#676


Description

code/contracts/modules/TransferManager/TransferManager.sol:L19-L27

function unpause() public {
    _onlySecurityTokenOwner();
    super._unpause();
}

function pause() public {
    _onlySecurityTokenOwner();
    super._pause();
}

pause and unpause functions implementations duplicate checks from Module contract that TransferManager inherits.

Remediation

Remove pause and unpause functions from TransferManager contract.

3.30 Modules shouldn't be casted to IBoot


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#687.


Description

code/contracts/modules/ModuleFactory.sol:L222

bytes4 initFunction = IBoot(_module).getInitFunction();

Modules do not explicitly inherit IBoot even though IModule and IBoot have the same function getInitFunction.

Remediation

Either cast _module to IModule or make IModule inherit IBoot.

3.31 public functions in TokenLib could be external instead


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#678.


Description

Most, if not all, public functions in TokenLib could be made external instead. This would save on gas at both deployment time and runtime. It would also make the code easier to read/audit by eliminating the possibility of internal callers.

Note that these functions could instead be made internal, but this would cause them to be inlined into the calling contract, and we believe it was an intentional decision to use TokenLib as a way to reduce the size of the calling contract.

Remediation

Mark the functions as external instead of public wherever possible.

3.32 EtherDividendCheckpoint and ERC20DividendCheckpoint division by zero


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#697.


Description

EtherDividendCheckpoint._createDividendWithCheckpointAndExclusions and ERC20DividendCheckpoint._createDividendWithCheckpointAndExclusions reject dividends when the currentSupply is zero, presumably to prevent future division by zero errors when calculating dividend payouts. Code for EtherDividendCheckpoint:

code/contracts/modules/Checkpoint/Dividend/Ether/EtherDividendCheckpoint.sol:L134

require(currentSupply > 0, "Invalid supply");

However, the actual totalSupply for the dividend is calculated by subtracting balances of excluded addresses from currentSupply. This means that a zero is possible:

code/contracts/modules/Checkpoint/Dividend/Ether/EtherDividendCheckpoint.sol:L152-L158

for (uint256 j = 0; j < _excluded.length; j++) {
    require(_excluded[j] != address(0), "Invalid address");
    require(!dividends[dividendIndex].dividendExcluded[_excluded[j]], "duped exclude address");
    excludedSupply = excludedSupply.add(ISecurityToken(securityToken).balanceOfAt(_excluded[j], _checkpointId));
    dividends[dividendIndex].dividendExcluded[_excluded[j]] = true;
}
dividends[dividendIndex].totalSupply = currentSupply.sub(excludedSupply);

(The same issue is present in ERC20DividendCheckpoint.)

The impact of this bug is minor because no payouts will be possible on a dividend with no eligible balances.

Remediation

The best fix is probably to move the require statement down below the totalSupply calculation and base it on that value:

require(dividends[dividendIndex].totalSupply > 0, "Invalid total supply");

3.33 SecurityToken / STGetter storage layout is hard to maintain


Severity: Minor

Status: Closed

Resolution: This issue has been addressed with PolymathNetwork/polymath-core#704.

We added a script to our deployment pipeline to check that memory is laid out correctly.


Description

SecurityToken contract delegates to STGetter as a workaround for contract deployment size limitations. As is the case for any delegation between contracts, the two contracts need to maintain identical storage layouts.

Unlike in other proxies throughout the codebase, SecurityToken and STGetter do not inherit from the same base storage contracts to enforce the storage layout. Rather, STGetter uses the OZStorage contract. The "OZ" presumably stands for "OpenZeppelin" because the contract mimics the storage layout of the ERC20 and ReentrancyGuard contracts from OpenZeppelin.

Although the current code looks correct, maintaining the relationship between SecurityToken and STGetter's contract storage is both difficult and critical from a security perspective.

Remediation

  1. Replace OZStorage with ERC20 and ReentrancyGuard directly in STGetter inheritance. This has the advantage of making the code more transparent and eliminating OZStorage altogether. It will make STGetter more expensive to deploy, but this is a one-time cost.
  2. At a minimum, add comments to OZStorage, SecurityToken, STGetter that explains the issue, so future code maintainers remember to ensure consistent storage layout.

3.34 LockUpTransferManager._removeLockUpFromUser checks for an impossible condition


Severity: Minor

Status: Closed

Resolution: This issue has been closed with PolymathNetwork/polymath-core#688.


Description

The following lines check whether an index is equal to an array length, but such a thing cannot occur except in error cases that would cause the transaction to revert anyway. Presumably there's a missing - 1 in both places:

code/contracts/modules/TransferManager/LTM/LockUpTransferManager.sol:L553

if ( _lockupIndex != _len) {

code/contracts/modules/TransferManager/LTM/LockUpTransferManager.sol:L563

if ( _userIndex != _len) {

In both cases, the erroneous check is harmless because it was just attempting to save gas anyway by avoiding a no-op swap.

Remediation

Check against _len - 1 instead.

4 Threat Model

The creation of a threat model is beneficial when building smart contract systems as it helps to understand the potential securi