A CONSENSYS DILIGENCE Audit Report

Codefi ERC1400 Assessment

1 Executive Summary

This report presents the results of our engagement with Codefi to review ERC-1400.

The review was conducted over the course of two weeks, from June 1 to June 12 2020, by Daniel Luca. A total of 10 person-days were spent.

During the first week, we reviewed the changes since the last code review our team did in June 2019 but quickly realized that it would not be effective since a lot of code was changed. We set up a meeting with the development team on Tuesday to explain our process, understand the changes, agree on the scope of the review, pick a commit hash and where we should focus mostly.

We agreed to audit the current version of ERC1400.sol and not focus on the changes since some do not reflect the existing code. The initial meeting with the development team revealed that we should focus on the contract extension, contract size optimization and gas optimizations; of course, any security issues should also be reported.

The EIP-1400 has suffered many changes, forcing the development team to keep the implementation in sync with the evolving standard.

We ran a few tools to check for glaring security problems and to create a better overview of how the implementation works. The output of these tools was added in the Artifacts section.

We proceeded to check the current implementation against the EIP-1400 standard. The code is well documented and well structured and it was easy to read and understand.

During the second week, we continued our code review and proceeded to check all of the remaining functionality. Again, no significant problems were found in the code.

We set up a couple of meetings with the development team to discuss decisions and initial findings. We presented ideas on how to reduce code complexity and how to improve gas consumption, return error codes and different caveats.

Towards the end of the week, the client added to the scope of the audit two additional contracts: ERC1400CertificateNonce.sol and ERC1400CertificateSalt.sol. Their code was audited previously in the previous review in June 2019, but the structure of the contracts was changed.

At the end of the week, on Friday, we delivered our report to the client.

2 Scope

Our review focused on the commit hash f6de24d50c54471f85985e2303a04bb92c27ac71. The list of files in scope is in the Appendix.

2.1 Objectives

Together with the Codefi team, we identified the following priorities for our review:

  1. Ensure that the system is implemented consistently with the intended functionality and without unintended edge cases.
  2. Identify known vulnerabilities particular to smart contract systems, as outlined in our Smart Contract Best Practices, and the Smart Contract Weakness Classification Registry.
  3. Investigate ways to reduce the size of the contract because it is close to 24576 bytes (0x6000) contract size limit as described in EIP-170.
  4. Make sure the contract extensions do not create problems.
  5. Gas optimizations.

3 Security Specification

This section describes, from a security perspective, the expected behavior of the system under audit. It is not a substitute for documentation. The purpose of this section is to identify specific security properties that were validated by the audit team.

3.1 Actors

The relevant actors are listed below with their respective abilities:

4 Issues

Each issue has an assigned severity:

  • Minor issues are subjective in nature. They are typically suggestions around best practices or readability. Code maintainers should use their own judgment as to whether to address such issues.
  • Medium issues are objective in nature but are not security vulnerabilities. These should be addressed unless there is a clear reason not to.
  • Major issues are security vulnerabilities that may not be directly exploitable or may require certain conditions in order to be exploited. All major issues should be addressed.
  • Critical issues are directly exploitable security vulnerabilities that need to be fixed.

4.1 ERC1400ERC20 whitelist circumvents partition restrictions Critical ✓ Fixed

Resolution

This is fixed in ConsenSys/ERC1400#13.

Description

ERC1400/1410 enable “partially fungible tokens” in that not all tokens are equivalent. A specific use case is placing restrictions on some tokens, such as lock-up periods.

The whitelist in ERC1400ERC20 circumvents these restrictions. When a token holder uses the ERC20 transfer function, tokens are transferred from that user’s “default partitions”, which a user can choose themselves by calling ERC1410.setDefaultPartitions. This means they can transfer tokens from any partition, and the only restriction that’s placed on the transfer is that the recipient must be whitelisted.

It should be noted that the comment and error message around the whitelisting feature suggests that it is meant to be applied to both the sender and recipient:

code/contracts/token/ERC20/ERC1400ERC20.sol:L24-L30

/**
 * @dev Modifier to verify if sender and recipient are whitelisted.
 */
modifier isWhitelisted(address recipient) {
  require(_whitelisted[recipient], "A3: Transfer Blocked - Sender lockup period not ended");
  _;
}

Remediation

There are many possibilities, but here are concrete suggestions for addressing this:

  1. Require whitelisting both the sender and recipient, and make sure that whitelisted accounts only own (and will only own) unrestricted tokens.
  2. Make sure that the only whitelisted recipients are those that apply partition restrictions when receiving tokens. (I.e. they implement the modified ERC777 receiving hook, examine the source partition, and reject transfers that should not occur.)
  3. Instead of implementing the ERC20 interface on top of the ERC1400 token, support transferring out of the ERC1400 token and into a standard ERC20 token. Partition restrictions can then be applied on the ERC1400 transfer, and once ERC20 tokens are obtained, they can be transferred without restriction.
  4. Don’t allow token holders to set their own default partitions. Rather, have the token specify a single, unrestricted partition that is used for all ERC20 transfers.

4.2 Certificate controllers do not always constrain the last argument Critical ✓ Fixed

Resolution

The existing back end already does its own ABI encoding, which means it’s not vulnerable to this issue. Documentation has been added in https://gitlab.com/ConsenSys/client/fr/dauriel/smart-contracts/certificate-controller/merge_requests/9 to ensure future maintainers understand this potential issue.

Description

The certificate controllers (CertificateControllerNonce and CertificateControllerSalt) are used by passing a signature as a final argument in a function call. This signature is over the other arguments to the function. Specifically, the signature must match the call data that precedes the signature.

The way this is implemented assumes standard ABI encoding of parameters, but there’s actually some room for manipulation by a malicious user. This manipulation can allow the user to change some of the call data without invalidating the signature.

The following code is from CertificateControllerNonce, but similar logic applies to CertificateControllerSalt:

code2/contracts/CertificateControllerNonce.sol:L127-L134

bytes memory payload;

assembly {
  let payloadsize := sub(calldatasize, 160)
  payload := mload(0x40) // allocate new memory
  mstore(0x40, add(payload, and(add(add(payloadsize, 0x20), 0x1f), not(0x1f)))) // boolean trick for padding to 0x40
  mstore(payload, payloadsize) // set length
  calldatacopy(add(add(payload, 0x20), 4), 4, sub(payloadsize, 4))

Here the signature is over all call data except the final 160 bytes. 160 bytes makes sense because the byte array is length 97, and it’s preceded by a 32-byte size. This is a total of 129 bytes, and typical ABI encoded pads this to the next multiple of 32, which is 160.

If an attacker does not pad their arguments, they can use just 129 bytes for the signature or even 128 bytes if the v value happens to be 0. This means that when checking the signature, not only will the signature be excluded, but also the 31 or 32 bytes that come before the signature. This means the attacker can call a function with a different final argument than the one that was signed.

That final argument is, in many cases, the number of tokens to transfer, redeem, or issue.

Mitigating factors

For this to be exploitable, the attacker has to be able to obtain a signature over shortened call data.

If the signer accepts raw arguments and does its own ABI encoding with standard padding, then there’s likely no opportunity for an attacker to exploit this vulnerability. (They can shorten the call data length when they make the function call later, but the signature won’t match.)

Remediation

We have two suggestions for how to address this:

  1. Instead of signatures being checked directly against call data, compute a new hash based on the decoded values, e.g. keccak256(abi.encode(argument1, argument2, ...)).
  2. Address this at the signing layer (off chain) by doing the ABI encoding there and denying an attacker the opportunity to construct their own call data.

4.3 Salt-based certificate controller is subject to signature replay Critical ✓ Fixed

Resolution

Description

The salt-based certificate controller prevents signature replay by storing each full signature. Only a signature that is exactly identical to a previously-used signature will be rejected.

For ECDSA signatures, each signature has a second S value (and flipped V to match) that will recover the same address. An attacker can produce such a second signature trivially without knowing the signer’s private key. This gives an attacker a way to produce a new unique signature based on a previously used one. This effectively means every signature can be used twice.

code2/contracts/CertificateControllerSalt.sol:L25-L32

modifier isValidCertificate(bytes memory data) {

  require(
    _certificateSigners[msg.sender] || _checkCertificate(data, 0, 0x00000000),
    "A3: Transfer Blocked - Sender lockup period not ended"
  );

  _usedCertificate[data] = true; // Use certificate

References

See https://smartcontractsecurity.github.io/SWC-registry/docs/SWC-117.

Remediation

Instead of rejecting used signatures based on the full signature value, keep track of used salts (which are then better referred to as “nonces”).

4.4 EIP-1400 is missing canTransfer* functions Major  Acknowledged

Description

The EIP-1400 states defines the interface to be implemented containing the 3 functions:

  // Transfer Validity
  function canTransfer(address _to, uint256 _value, bytes _data) external view returns (byte, bytes32);
  function canTransferFrom(address _from, address _to, uint256 _value, bytes _data) external view returns (byte, bytes32);
  function canTransferByPartition(address _from, address _to, bytes32 _partition, uint256 _value, bytes _data) external view returns (byte, bytes32, bytes32);

These functions were not implemented in ERC1400, thus making the implementation not completely compatible with EIP-1400.

In case the deployed contract needs to be added as a “lego block” part of a another application, there is a high chance that it will not correctly function. That external application could potentially call the EIP-1400 functions canTransfer, canTransferFrom or canTransferByPartition, in which case the transaction will likely fail.

This means that the current implementation will not be able to become part of external markets, exchanges or applications that need to interact with a generic EIP-1400 implementation.

Remediation

Even if the functions do not correctly reflect the transfer possibility, their omission can break other contracts interacting with the implementation.

A suggestion would be to add these functions and make them always return true. This way the contracts interacting with the current implementation do not break when they call these functions, while the actual transfer of the tokens is still limited by the current logic.

4.5 ERC777 incompatibilities Major ✓ Fixed

Resolution

This is fixed in ConsenSys/ERC1400#26.

Description

As noted in the README, the ERC777 contract is not actually compatible with ERC 777.

Functions and events have been renamed, and the hooks ERC777TokensRecipient and ERC777TokensSender have been modified to add a partition parameter.

This means no tools that deal with standard ERC 777 contracts will work with this code’s tokens.

Remediation

We suggest renaming these contracts to not use the term “ERC777”, as they lack compatibility. Most importantly, we recommend not using the interface names “ERC777TokensRecipient” and “ERC777TokensSender” when looking up the appropriate hook contracts via ERC 1820. Contracts that handle that interface will not be capable of handling the modified interface used here.

4.6 Buffer over-read in ERC1410._getDestinationPartition Major ✓ Fixed

Resolution

This is fixed in ConsenSys/ERC1400#16.

Description

There’s no check that data is at least 64 bytes long, so the following code can read past the end of data:

code/contracts/token/ERC1410/ERC1410.sol:L348-L361

function _getDestinationPartition(bytes32 fromPartition, bytes memory data) internal pure returns(bytes32 toPartition) {
  bytes32 changePartitionFlag = 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
  bytes32 flag;
  assembly {
    flag := mload(add(data, 32))
  }
  if(flag == changePartitionFlag) {
    assembly {
      toPartition := mload(add(data, 64))
    }
  } else {
    toPartition = fromPartition;
  }
}

The only caller is _transferByPartition, which only checks that data.length > 0:

code/contracts/token/ERC1410/ERC1410.sol:L263-L264

if(operatorData.length != 0 && data.length != 0) {
  toPartition = _getDestinationPartition(fromPartition, data);

Depending on how the compiler chooses to lay out memory, the next data in memory is probably the operatorData buffer, so data may inadvertently be read from there.

Remediation

Check for sufficient length (at least 64 bytes) before attempting to read it.

4.7 ERC20/ERC777 compatibility: ERC20 transfer functions should not revert if the recipient is a contract without a registered ERC777TokensRecipient implementation Medium ✓ Fixed

Resolution

This is fixed in ConsenSys/ERC1400#17.

Description

The ERC20 functions ERC1400ERC20.transfer and ERC1400ERC20.transferFrom call ERC1410._transferByDefaultPartitions, which calls ERC1410._transferByPartition, which calls ERC777._transferWithData with the preventLocking argument of true.

This will block transfers to a contract that doesn’t have an ERC777TokensRecipient implementation. This is in violation of ERC 777, which says:

If the recipient is a contract, which has not registered an ERC777TokensRecipient implementation; then the token contract:

  • MUST revert if the tokensReceived hook is called from a mint or send call.
  • SHOULD continue processing the transaction if the tokensReceived hook is called from an ERC20 transfer or transferFrom call.

Remediation

Make sure that ERC20-compatible transfer calls do not set preventLocking to true.

4.8 ERC777 compatibility: authorizeOperator and revokeOperator should revert when the caller and operator are the same account Medium ✓ Fixed

Resolution

This is fixed in ConsenSys/ERC1400#19.

Description

From ERC 777:

NOTE: The holder (msg.sender) is always an operator for itself. This right SHALL NOT be revoked. Hence this function MUST revert if it is called to authorize the holder (msg.sender) as an operator for itself (i.e. if operator is equal to msg.sender).

The autohrizeOperator implementation does not do that:

code/contracts/token/ERC777/ERC777.sol:L144-L147

function authorizeOperator(address operator) external {
  _authorizedOperator[operator][msg.sender] = true;
  emit AuthorizedOperator(operator, msg.sender);
}

The same holds for revokeOperator:

code/contracts/token/ERC777/ERC777.sol:L155-L158

function revokeOperator(address operator) external {
  _authorizedOperator[operator][msg.sender] = false;
  emit RevokedOperator(operator, msg.sender);
}

Remediation

Add require(operator != msg.sender) to those two functions.

4.9 Token receiver can mint gas tokens with sender’s gas Minor  Acknowledged

Description

When a transfer is executed, there are hooks activated on the sender’s and on the receiver’s side.

This is possible because the contract implements ERC1820Client which allows any address to define an implementation:

contracts/ERC1820Client.sol:L16-L19

function setInterfaceImplementation(string memory _interfaceLabel, address _implementation) internal {
    bytes32 interfaceHash = keccak256(abi.encodePacked(_interfaceLabel));
    ERC1820REGISTRY.setInterfaceImplementer(address(this), interfaceHash, _implementation);
}

Considering the receiver’s side:

contracts/ERC1400.sol:L1016-L1020

recipientImplementation = interfaceAddr(to, ERC1400_TOKENS_RECIPIENT);

if (recipientImplementation != address(0)) {
  IERC1400TokensRecipient(recipientImplementation).tokensReceived(msg.sig, partition, operator, from, to, value, data, operatorData);
}

The sender has to pay for the gas for the transaction to go through.

Because the receiver can define a contract to be called when receiving the tokens, and the sender has to pay for the gas, the receiver can mint gas tokens (or waste the gas).

Remediation

Because this is the way Ethereum works and the implementation allows calling external methods, there’s no recommended remediation for this issue. It’s just something the senders need to be aware of.

4.10 Missing ERC Functions Minor ✓ Fixed

Resolution

This is fixed in https://github.com/ConsenSys/ERC1400/pull/18.

Description

There exist some functions, such as isOperator() ,that are part of the ERC1410 spec. Removing functions expected by ERC may break things like block explorers that expect to be able to query standard contracts for relevant metadata.

Remediation

It would be good to explicitly state any expected incompatibilities.

4.11 Inaccurate error message in ERC777ERC20.approve Minor ✓ Fixed

Resolution

This is fixed in ConsenSys/ERC1400#20.

Description

If the spender is address 0, the revert message says that the receiver is not eligible.

code/contracts/token/ERC20/ERC777ERC20.sol:L153

require(spender != address(0), "A6: Transfer Blocked - Receiver not eligible");

Remediation

Fix the revert message to match the actual issue.

4.12 Non-standard treatment of a from address of 0 Minor ✓ Fixed

Resolution

This is fixed in ConsenSys/ERC1400#21.

Description

A number of functions throughout the system treat a from address of 0 as equivalent to msg.sender. In some cases, this seems to violate existing standards (e.g. in ERC20 transfers). In other cases, it is merely surprising.

ERC1400ERC20.transferFrom and ERC777ERC20.transferFrom both treat a from address as 0 as equivalent to msg.sender. This is unexpected behavior for an ERC20 token.

Examples

code/contracts/ERC1400.sol:L206-L214

function canOperatorTransferByPartition(bytes32 partition, address from, address to, uint256 value, bytes calldata data, bytes calldata operatorData)
  external
  view
  returns (byte, bytes32, bytes32)
{
  if(!_checkCertificate(operatorData, 0, 0x8c0dee9c)) { // 4 first bytes of keccak256(operatorTransferByPartition(bytes32,address,address,uint256,bytes,bytes))
    return(hex"A3", "", partition); // Transfer Blocked - Sender lockup period not ended
  } else {
    address _from = (from == address(0)) ? msg.sender : from;

code/contracts/ERC1400.sol:L417-L421

function redeemFrom(address from, uint256 value, bytes calldata data, bytes calldata operatorData)
  external
  isValidCertificate(operatorData)
{
  address _from = (from == address(0)) ? msg.sender : from;

code/contracts/token/ERC20/ERC1400ERC20.sol:L180-L181

function transferFrom(address from, address to, uint256 value) external isWhitelisted(to) returns (bool) {
  address _from = (from == address(0)) ? msg.sender : from;

code/contracts/token/ERC20/ERC777ERC20.sol:L179-L180

function transferFrom(address from, address to, uint256 value) external isWhitelisted(to) returns (bool) {
  address _from = (from == address(0)) ? msg.sender : from;

code/contracts/token/ERC777/ERC777.sol:L194-L198

function transferFromWithData(address from, address to, uint256 value, bytes calldata data, bytes calldata operatorData)
  external
  isValidCertificate(operatorData)
{
  address _from = (from == address(0)) ? msg.sender : from;

code/contracts/token/ERC777/ERC777.sol:L226-L230

function redeemFrom(address from, uint256 value, bytes calldata data, bytes calldata operatorData)
  external
  isValidCertificate(operatorData)
{
  address _from = (from == address(0)) ? msg.sender : from;

code/contracts/token/ERC1410/ERC1410.sol:L130-L142

function operatorTransferByPartition(
  bytes32 partition,
  address from,
  address to,
  uint256 value,
  bytes calldata data,
  bytes calldata operatorData
)
  external
  isValidCertificate(operatorData)
  returns (bytes32)
{
  address _from = (from == address(0)) ? msg.sender : from;

code/contracts/token/ERC1410/ERC1410.sol:L430-L434

function transferFromWithData(address from, address to, uint256 value, bytes calldata data, bytes calldata operatorData)
  external
  isValidCertificate(operatorData)
{
  address _from = (from == address(0)) ? msg.sender : from;

Remediation

Remove this fallback logic and always use the from address that was passed in. This avoids surprises where, for example, an uninitialized value leads to loss of funds.

4.13 ERC1410’s redeem and redeemFrom should revert Minor ✓ Fixed

Resolution

This is fixed in ConsenSys/ERC1400#22.

Description

ERC1410 contains two functions: redeem and redeemFrom that “erase” the underlying ERC777 versions of these functions because those functions don’t handle partitions.

These functions silently succeed, while they should probably fail by reverting.

Examples

code/contracts/token/ERC1410/ERC1410.sol:L441-L453

/**
 * [NOT MANDATORY FOR ERC1410 STANDARD][OVERRIDES ERC777 METHOD]
 * @dev Empty function to erase ERC777 redeem() function since it doesn't handle partitions.
 */
function redeem(uint256 /*value*/, bytes calldata /*data*/) external { // Comments to avoid compilation warnings for unused variables.
}

/**
 * [NOT MANDATORY FOR ERC1410 STANDARD][OVERRIDES ERC777 METHOD]
 * @dev Empty function to erase ERC777 redeemFrom() function since it doesn't handle partitions.
 */
function redeemFrom(address /*from*/, uint256 /*value*/, bytes calldata /*data*/, bytes calldata /*operatorData*/) external { // Comments to avoid compilation warnings for unused variables.
}

Remediation

Add a revert() (possibly with a reason) so callers know that the call failed.

4.14 Unclear why operatorData.length is checked in _transferByPartition Minor ✓ Fixed

Resolution

This code is actually correct. When data is present but operatorData is not, the data is the certificate (signature), which should not be used to determine the destination partition. When operatorData is present, the certificate is found there, and data can be used to determine the destination partition. This code checks correctly for that condition.

Description

It’s unclear why operatorData.length is being checked here:

code/contracts/token/ERC1410/ERC1410.sol:L263-L264

if(operatorData.length != 0 && data.length != 0) {
  toPartition = _getDestinationPartition(fromPartition, data);

Remediation

Consider removing that check.

4.15 Global partition enumeration can run into gas limits Minor ✓ Fixed

Resolution

This is fixed in ConsenSys/ERC1400#25.

Description

In ERC1410, partitions are created on demand by issuing or transferring tokens, and these new partitions are added to the array _totalPartitions. When one of these partitions is later emptied, it’s removed from that array with the following code in _removeTokenFromPartition:

code/contracts/token/ERC1410/ERC1410.sol:L303-L313

// If the total supply is zero, finds and deletes the partition.
if(_totalSupplyByPartition[partition] == 0) {
  for (uint i = 0; i < _totalPartitions.length; i++) {
    if(_totalPartitions[i] == partition) {
      _totalPartitions[i] = _totalPartitions[_totalPartitions.length - 1];
      delete _totalPartitions[_totalPartitions.length - 1];
      _totalPartitions.length--;
      break;
    }
  }
}

Finding the partition requires iterating over the entire array. This means that _removeTokenFromPartition can become very expensive and eventually bump up against the block gas limit if lots of partitions are created. This could be an attack vector for a malicious operator.

The same issue applies to a token holder’s list of partitions, where transferring tokens in a large number of partitions to that token holder may block them from being able to transfer tokens out:

code/contracts/token/ERC1410/ERC1410.sol:L291-L301

// If the balance of the TokenHolder's partition is zero, finds and deletes the partition.
if(_balanceOfByPartition[from][partition] == 0) {
  for (uint i = 0; i < _partitionsOf[from].length; i++) {
    if(_partitionsOf[from][i] == partition) {
      _partitionsOf[from][i] = _partitionsOf[from][_partitionsOf[from].length - 1];
      delete _partitionsOf[from][_partitionsOf[from].length - 1];
      _partitionsOf[from].length--;
      break;
    }
  }
}

Remediation

Removing an item from a set can be accomplished in constant time if the set uses both an array (for storing the values) and a mapping of values to their index in that array. See https://programtheblockchain.com/posts/2018/06/03/storage-patterns-set/ for one example of doing this.

It also may be reasonable to cap the number of possible partitions or lock them down to a constant set of values on deployment, depending on the use case for the token.

4.16 Optimization: redundant delete in ERC1400. _removeTokenFromPartition Minor ✓ Fixed

Resolution

This is fixed in ConsenSys/ERC1400#23.

Description

Reducing the size of an array automatically deletes the removed elements, so the first of these two lines is redundant:

code/contracts/token/ERC1410/ERC1410.sol:L296-L297

delete _partitionsOf[from][_partitionsOf[from].length - 1];
_partitionsOf[from].length--;

The same applies here:

code/contracts/token/ERC1410/ERC1410.sol:L308-L309

delete _totalPartitions[_totalPartitions.length - 1];
_totalPartitions.length--;

Remediation

Remove the redundant deletions to save a little gas.

4.17 Avoid hardcoding function selectors Minor ✓ Fixed

Resolution

This is fixed in ConsenSys/ERC1400#24.

Description

In ERC1400, hardcoded function selectors can be replaced with this.transferByPartition.selector and this.operatorTransferByPartition.selector.

Examples

code/contracts/ERC1400.sol:L184

if(!_checkCertificate(data, 0, 0xf3d490db)) { // 4 first bytes of keccak256(transferByPartition(bytes32,address,uint256,bytes))

code/contracts/ERC1400.sol:L211

if(!_checkCertificate(operatorData, 0, 0x8c0dee9c)) { // 4 first bytes of keccak256(operatorTransferByPartition(bytes32,address,address,uint256,bytes,bytes))

Remediation

Replace the hardcoded function selectors with this.<method>.selector.

Appendix 1 - Recommendations and Suggestions

A.1.1 Contract Size Limit

The client said they are reaching the contract limits implemented by EIP-170 and might hit problems in the future. The current hard limit is 24576 bytes.

The deployed bytecode of the ERC1400 contract is 17677 bytes. However, the contract is made to be extended by other contracts that could hit the contract size limit imposed on Ethereum. Such implementations are ERC1400CertificateNonce and ERC1400CertificateSalt, which currently hit 22464 bytes and 22473 bytes and are dangerously close to the hard limit.

Other implementations that extend ERC1400 might easily go over this limit.

One such optimization, reducing the require reason strings was already implemented by the development team. Even though it reduces the readability of the error code, it reduces the contract size.

Diamond Standard

Another, more flexible, way to reduce contract size, while also allowing for parts of the contract to be extended, is the Diamond Standard which is an extension of Transparent Contract Standard. This allows the developer to move parts of the functionality into different deployed contracts, called “facets” of the diamond, and adding these “facets” to the main contract as extensions. This extended functionality uses delegatecall to use external runtime bytecode while using the same main storage. A detailed explanation and implementation example is in the official EIP-2535 proposal.

This way of extending the size of code poses some risks in the actual implementation of the delegatecall as well as careful planning on using the storage of the contract. A feature of this standard is that parts of the contract can be separately upgraded, but this also poses the risk of upgrading some code that doesn’t use the same contract storage layout as the previous code. The most significant risk is overwriting or misusing the contract storage.

Remove Wrapper Functions

The current implementation follows a more “classical” way of Object Oriented Programming pattern, where each public function calls an internal function, which in theory could be replaced by another implementation, while the public function remains the same. This does not match well with Ethereum, where code is considered immutable, code size and gas cost matters.

The internal functions can be dropped, and the body can be added directly into the public function. This way, the code size is reduced, while also reducing the gas cost. This is because there is no jump from the public function code to the internal function code.

Considering isOperator and the internal function _isOperator:

function isOperator(address operator, address tokenHolder) external view returns (bool) {
    return _isOperator(operator, tokenHolder);
}

function _isOperator(address operator, address tokenHolder) internal view returns (bool) {
    return (operator == tokenHolder
        || _authorizedOperator[operator][tokenHolder]
        || (_isControllable && _isController[operator])
    );
}

Can be rewritten to:

function isOperator(address operator, address tokenHolder) public view returns (bool) {
    return (operator == tokenHolder
        || _authorizedOperator[operator][tokenHolder]
        || (_isControllable && _isController[operator])
    );
}

The body of the internal function _isOperator was added directly as the body of the function isOperator and isOperator was changed from external to public to be called within the contract.

A list of functions which can be rewritten:

  • transferByPartition
  • isOperator
  • isOperatorForPartition
  • issueByPartition
  • redeemByPartition
  • setControllers
  • setPartitionControllers
  • setHookContract
  • migrate

Reducing the number of internal calls will also slightly improve the gas consumption.

A.1.2 Gas Optimizations

There are a few gas optimizations that can be done. These will not reduce the gas significantly, so only implement them (or some of them) if you feel the gas optimization is essential to you.

  • _transferWithData

The current code looks like this:

require(_balances[from] >= value, "52"); // 0x52	insufficient balance

_balances[from] = _balances[from].sub(value);

The safemath implementation looks like this:

function sub(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256) {
    require(b <= a, errorMessage);
    uint256 c = a - b;

    return c;
}

Which means that the check is done twice, not to underflow. To save the most gas, the safemath call can be removed in favor of simple subtraction, reducing the number of checks and jumps.

require(_balances[from] >= value, "52"); // 0x52	insufficient balance

_balances[from] = _balances[from] - value;
  • _isMultiple

The current code looks like this:

function _isMultiple(uint256 value) internal view returns(bool) {
    return(value.div(_granularity).mul(_granularity) == value);
}

Which is equivalent to:

function _isMultiple(uint256 value) internal view returns(bool) {
    return ((value % _granularity) == 0);
}

Which reduces the number of checks and jumps significantly.

  • operatorTransferByPartition

The current code looks like this:

if(_allowedByPartition[partition][from][msg.sender] >= value) {
    _allowedByPartition[partition][from][msg.sender] = _allowedByPartition[partition][from][msg.sender].sub(value);

It can be rewritten without safemath, because the if condition already checks for underflows.

if(_allowedByPartition[partition][from][msg.sender] >= value) {
    _allowedByPartition[partition][from][msg.sender] = _allowedByPartition[partition][from][msg.sender] - value;
  • transferFrom

Similar for:

if(_allowed[from][msg.sender] >= value) {
    _allowed[from][msg.sender] = _allowed[from][msg.sender].sub(value);

Can be rewritten to:

if(_allowed[from][msg.sender] >= value) {
    _allowed[from][msg.sender] = _allowed[from][msg.sender] - value;
  • _removeTokenFromPartition

The current code looks like this:

_balanceOfByPartition[from][partition] = _balanceOfByPartition[from][partition].sub(value);
_totalSupplyByPartition[partition] = _totalSupplyByPartition[partition].sub(value);

There are 2 instances where _removeTokenFromPartition is called:

In _redeemByPartition:

require(_balanceOfByPartition[from][fromPartition] >= value, "52"); // 0x52	insufficient balance

[...]

_removeTokenFromPartition(from, fromPartition, value);

And in _transferByPartition:

require(_balanceOfByPartition[from][fromPartition] >= value, "52"); // 0x52	insufficient balance

[...]

_removeTokenFromPartition(from, fromPartition, value);

And in both cases the value has to be smaller or equal to the balance, in which case no underflow can happen.

However, because there is a disconnect between where the check is done and where the subtraction happens, removing the safemath, even though it reduces the gas cost, makes the developer remember the _removeTokenFromPartition does not do a safe subtraction. This can be a problem if the developer needs to add more functionality and should remember the internal function does not do the necessary checks.

Observation

Other gas optimization techniques can be implemented. However, they might introduce significant rewrites or increase the code size. Hence they were not recommended in this case.

Also, with these suggestions, you should implement them only if you are sure you understand all the implications of reducing the gas cost, removing additional checks and gas reduced is significant for your application.

A.1.3 Error Codes

The EIP-1400 states that the function canTransfer MUST return a reason byte code on success or failure based on EIP-1066 error codes. This creates an inconsistency between these error codes and the other errors returned in the contract.

In order to have more consistency and a smaller contract size, the development team changed all the error strings to error codes in the rest of the implementation.

Examples

Error codes as strings in requires:

modifier isIssuableToken() {
    require(_isIssuable, "55"); // 0x55	funds locked (lockup period)
    _;
}

function transferFrom(address from, address to, uint256 value) external returns (bool) {
    require( _isOperator(msg.sender, from)
      || (value <= _allowed[from][msg.sender]), "53"); // 0x53	insufficient allowance

Error codes as hex argument in _canTransfer:

function _canTransfer(bytes4 functionSig, bytes32 partition, address operator, address from, address to, uint256 value, bytes memory data, bytes memory operatorData)
    internal
    view
    returns (byte, bytes32, bytes32)
{
    address checksImplementation = interfaceAddr(address(this), ERC1400_TOKENS_CHECKER);

    if((checksImplementation != address(0))) {
        return IERC1400TokensChecker(checksImplementation).canTransferByPartition(functionSig, partition, operator, from, to, value, data, operatorData);
    }
    else {
        return(hex"00", "", partition);
    }
}

Description

Excerpt from the EIP-1066 standard:

This provides a shared set of signals to allow smart contracts to react to situations autonomously, expose localized error messages to users, and so on.

The initial intention of returning these error codes is to allow other contracts to interpret the status instead of failing without knowing the reason why.

Because the EIP-1400 standard does not fully implement the error codes, it creates a difficult choice for the developer to have inconsistency between different parts of the code that return error strings in contrast with the _canTransfer method that returns hex errors.

To illustrate the difference of failing with error strings versus returning a status code we have created these test contracts.

contract ReturnTest {
    function returnRequireErrorString() public {
        require(false, "50");
    }
   
    function returnRequireErrorHex() public {
        require(false, hex"50");
    }
   
    function returnFirstByteArgument() public returns (byte status) {
        return hex"50";
    }
}

contract ReadReturn {
    ReturnTest public returnTest;
   
    event Success(bool);
    event Message(bytes);
   
    constructor () public {
        returnTest = new ReturnTest();
    }
   
    function callMethod(string calldata _method) external returns (bool, bytes memory) {
        (bool success, bytes memory message) = address(returnTest).call(abi.encodeWithSignature(_method));
       
        emit Success(success);
        emit Message(message);
    }
}

Calling ReadReturn.callMethod() emits these events for each _method argument:

  • callMethod('returnRequireErrorString()')
Success(false)
Message('0x08c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000023530000000000000000000000000000000000000000000000000000000000000')

generated by

require(false, "50");

From the returned message, 0x3530 represents the string “50” and the preceding 0x02 is the length of the returned string.

  • callMethod('returnRequireErrorHex()')
Success(false)
Message('0x08c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000015000000000000000000000000000000000000000000000000000000000000000')

generated by

require(false, hex"50");

This time the returned message contains 0x50 which represents the hex"50" and the preceding 0x01 is the length of the returned string.

  • callMethod('returnFirstByteArgument()')
Success(true)
Message('0x5000000000000000000000000000000000000000000000000000000000000000')

generated by

return hex"50";

This time the returned value contains 0x50 which is the first byte of the returned bytes, and can be easily used in another smart contract as specified by ERC-1066.

The recurring hex in the first 2 calls 0x08c379a0 is the first 4 bytes of keccak256("Error(string)") which is always returned when a revert or require is triggered.

Remediation

There is no recommendation in this case because of the already described limitations of contract size and adhering to the EIP-1400 standard, an elegant solution was not identified.

We wanted to make sure we explain the difference between all approaches, specifically the difference between:

  • require(false, "50");
  • require(false, hex"50");
  • return hex"50";

Appendix 2 - Files in Scope

The commit hash that was reviewed is f6de24d50c54471f85985e2303a04bb92c27ac71.

This code review covered the following files:

File Name SHA-1 Hash
./contracts/ERC1400.sol adc01ee17e9379e623639cd3bef68de5e5aee264
./contracts/certificate/ERC1400CertificateNonce.sol 55baaf3be216704b6170251829708f626ea1a347
./contracts/certificate/ERC1400CertificateSalt.sol 35e093fcbc90a086e6ed2f0b2611e4909c6f4562

Appendix 3 - Disclosure

ConsenSys Diligence (“CD”) typically receives compensation from one or more clients (the “Clients”) for performing the analysis contained in these reports (the “Reports”). The Reports may be distributed through other means, including via ConsenSys publications and other distributions.

The Reports are not an endorsement or indictment of any particular project or team, and the Reports do not guarantee the security of any particular project. This Report does not consider, and should not be interpreted as considering or having any bearing on, the potential economics of a token, token sale or any other product, service or other asset. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. No Report provides any warranty or representation to any Third-Party in any respect, including regarding the bugfree nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the Reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this Report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. CD owes no duty to any Third-Party by virtue of publishing these Reports.

PURPOSE OF REPORTS The Reports and the analysis described therein are created solely for Clients and published with their consent. The scope of our review is limited to a review of Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty.

CD makes the Reports available to parties other than the Clients (i.e., “third parties”) – on its website. CD hopes that by making these analyses publicly available, it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.

LINKS TO OTHER WEB SITES FROM THIS WEB SITE You may, through hypertext or other computer links, gain access to web sites operated by persons other than ConsenSys and CD. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such web sites’ owners. You agree that ConsenSys and CD are not responsible for the content or operation of such Web sites, and that ConsenSys and CD shall have no liability to you or any other person or entity for the use of third party Web sites. Except as described below, a hyperlink from this web Site to another web site does not imply or mean that ConsenSys and CD endorses the content on that Web site or the operator or operations of that site. You are solely responsible for determining the extent to which you may use any content at any other web sites to which you link from the Reports. ConsenSys and CD assumes no responsibility for the use of third party software on the Web Site and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.

TIMELINESS OF CONTENT The content contained in the Reports is current as of the date appearing on the Report and is subject to change without notice. Unless indicated otherwise, by ConsenSys and CD.

Appendix 4 - Artifacts

This section contains some of the artifacts generated during our review by automated tools, the test suite, etc. If any issues or recommendations were identified by the output presented here, they have been addressed in the appropriate section above.

A.4.1 MythX

MythX is a security analysis API for Ethereum smart contracts. It performs multiple types of analysis, including fuzzing and symbolic execution, to detect many common vulnerability types. The tool was used for automated vulnerability discovery for all audited contracts and libraries. More details on MythX can be found at mythx.io.

Below is the raw output of the MythX vulnerability scan:

The scan did not reveal any security issues.

A.4.2 Surya

Surya is a utility tool for smart contract systems. It provides a number of visual outputs and information about the structure of smart contracts. It also supports querying the function call graph in multiple ways to aid in the manual inspection and control flow analysis of contracts.

Below is a complete list of functions with their visibility and modifiers:

A.4.3 Tests Suite

The tests cover 100% of the code.

Below is the output generated by running the test suite: