DS Token Security Audit Review

Introduction

Coinfabrik was asked to audit the contracts setting the DS Protocol, which has the goal to enable security token offerings as well as managing the token lifecycle post-sale. Firstly, we will provide a summary of our discoveries and secondly, we will show the details of our findings.

Summary

The contracts audited are from the Securitize private repository. The audit is based on the
commit 6817cb339defe4d67e63a3835dffcf95ba458119 , and updated to reflect changes at
c3729ea216eb33f093e6e5b209378e4d6728bbd7 .

Audited Contracts

The audited contracts are the following:

• These contracts refer to the eternal storage:
  ● storage/EternalStorageClient.sol: Function wrappers for Eternal Storage access.
  Automatically generated.
  ● storage/DSEternalStorage.sol: The eternal storage itself, which adds delete functions
  to Zeppelin’s eternal storage.
  The rest of the contracts all use the Eternal Storage for storing data:
  ● token/DSToken.sol: The token contract code, it’s accessed by a proxy entry point
  with delegation. It’s also an ESPausableToken.
  ● token/ESPausableToken.sol: ESStandardToken with pausing.
  ● token/ESStandardToken.sol: ESBasicToken with all ERC20 functions.
  ● token/ESBasicToken.sol: The transfer only portion of an ERC20 token.
  ● compliance/ESComplianceService.sol: Validation functions for token actions.
  Declares overridable boolean functions for compliance customization.
  ● compliance/ESComplianceServiceNotRegulated.sol: ESComplianceService without
  regulation, all overrides do not restrict anything.
  ● compliance/ESComplianceServiceWhitelisted.sol: ESComplianceService with simple
  whitelisting regulation.
  ● compliance/ESComplianceServiceRegulated.sol: ESComplianceService with
  stronger regulations, like region based restrictions, investor counting or token locking.
  ● compliance/ESLockManager.sol: Token locking contract logic for compliance.
  ● compliance/ESWalletManager.sol: Allows categorization of wallet addresses for
  compliance.
  ● compliance/ESIssuanceInformationManager.sol: Allows storage and retrieval of
  simple compliance information in form of strings.
  ● trust/ESTrustService.sol: Common address role implementation for function
  privileges.
  ● ESServiceConsumer.sol: Allows access to different deployed contracts of the project,
  returning the properly typed interfaces. This is also where common function privilege
  modifiers reside.
  ● registry/ESRegistryService.sol: Allows adding and retrieving investor information.
  ● util/ESPausable.sol: Simple pause modifiers.
  These are proxy related contracts only being used by the token at the moment:
  ● util/Proxy.sol: Allows delegation of all calls to a different contract address, serving as
  a permanent entry point while maintaining storage.
  ● util/ProxyTarget.sol: Needed for proxy callable contracts, declares the two addresses
  stored in Proxy so they don’t overlap at the delegated contract.
  These are all interfaces, mostly used as return types for ESServiceConsumer:
  ● token/DSTokenInterface.sol
  ● compliance/DSComplianceServiceInterface.sol
  ● compliance/DSLockManagerInterface.sol
  ● compliance/DSWalletManagerInterface.sol
  ● compliance/DSIssuanceInformationManagerInterface.sol
  ● trust/DSTrustServiceInterface.sol
  ● DSServiceConsumerInterface.sol
  ● registry/DSRegistryServiceInterface.sol
  Unused contracts:
  ● trust/DSTrustService.sol: Same as ESTrustService without Eternal Storage.
  ● DSServiceConsumer.sol: Same as ESServiceConsumer without Eternal Storage.
  Truffle contracts:
  ● util/Migrations.sol: Truffle migration contract.

Analyses performed

The following analyses have been performed:
● Misuse of the different call methods: call.value(), send() and transfer().
● Integer rounding errors, overflow, underflow and related usage of SafeMath
functions.
● Old compiler version pragmas.
● Race conditions such as reentrancy attacks or front running.
● Misuse of block timestamps, assuming anything other than them being strictly
increasing.
● Contract softlocking attacks (DoS).
● Potential gas cost of functions being over the gas limit.
● Missing function qualifiers and their misuse.
● Fallback functions with a higher gas cost than the one that a transfer or send call
allows.
● Fraudulent or erroneous code.
● Code and contract interaction complexity.
● Wrong or missing error handling.
● Overuse of transfers in a single transaction instead of using withdrawal patterns.
● Insufficient analysis of the function input requirements.

Findings

Critical severity

None has been found.

Medium severity

Insufficient deletion at ESRegistryService
The function removeInvestor at ESRegistryService.sol includes this for loop to delete
attributes:

for ( uint index = 0 ; index < 8 ; ++ index )
{ deleteUint ( "investors" , _id , index , "value" );
deleteUint ( "investors" , _id , index , "expiry" );
deleteString ( "investors" , _id , index , "proof_hash" ); }

But the index counter only goes up to 8, while the parameter _attributeId in the function
setAttribute may be any value between 0 and 255:

function setAttribute ( string _id , uint8 _attributeId , uint256 _value ,
uint256 _expiry , string _proofHash ) public onlyExchangeOrAbove returns ( bool )
{ setUint8 ( "investors" , _id , _attributeId , "value" , _value );
setUint8 ( "investors" , _id , _attributeId , "expiry" , _expiry );
setString8 ( "investors" , _id , _attributeId , "proof_hash" , _proofHash );
setAddress ( "investors" , _id , "last_updated_by" , msg . sender );
emit DSRegistryServiceInvestorChanged ( _id , msg . sender ); return true; }

This will result in erroneous saved data after an investor deletion. We recommend either restricting this parameter to only contain values up to 8, or increasing the counter while checking the gas costs for removeInvestor do not get dangerously near the gas limit per block. This was fixed in multiple commits with the last change being in commit 6bdcb8fc454f2f68441eda7bf2b6842269e665c0.

The unassigned boolean return value of function defaults to false The function recordTransfer at ESComplianceServiceRegulated.sol does not have a return statement:

function recordTransfer ( address _from , address _to , uint _value ) internal returns ( bool )
{ if ( _value > 0 &&
getToken (). balanceOfInvestor ( getRegistryService (). getInvestor ( _from )) == _value ) {
adjustInvestorCount ( _from , false );
}
if ( _value > 0 &&
getToken (). balanceOfInvestor ( getRegistryService (). getInvestor ( _to )) == 0 ) {
adjustInvestorCount ( _to , true );
}
}

This means the function will return the default value of false , which means this require at the
end of the function will inevitably fail when called:

function validate ( address _from , address _to , uint _value ) onlyToken public {
uint code;
string memory reason;
( code , reason ) = preTransferCheck ( _from , _to , _value );
require ( code == 0 , reason );
require ( recordTransfer ( _from , _to , _value ));
}

We recommend returning a proper value, true specifically if the check never fails.
This was fixed in commit 9dfd0e8ee78ae61445aee9d57d844fc59d8454b2.

Minor severity

Possible hash collision in EternalStorageClient
The contract EternalStorageClient is automatically generated, and uses abi.encodePacked
to pack multiple parameters in a single bytes typed value. This value is then hashed in the
EternalStorage contract to get a valid storage address. One example of many:

function setBoolean ( string p1 , bool v ) internal {
setBooleanGeneric ( abi . encodePacked ( namespace , p1 ), v );
}
function setBoolean ( string p1 , string p2 , bool v ) internal {
setBooleanGeneric ( abi . encodePacked ( namespace , p1 , p2 ), v );
}

These functions are supposed to always point to different boolean values, but they don’t if
the string at the first function is the same as the concatenation of the strings at the second
function. That means, for example, these calls:

setBoolean (" Audit ", true );
setBoolean (" Au ", " dit ", true );

Are essentially the same, since abi.encodePacked concatenates the strings and does not
save length information. Note that this may also happen between different types, for
example of this function:

function setBoolean ( string p1 , address p2 , bool v ) internal {
setBooleanGeneric ( abi . encodePacked ( namespace , p1 , p2 ), v );
}

May also collide with the other two functions if the address p2 concatenated with p1 has the
same representation as the string p1 of the first function or the strings p1 and p2 of the
second one.
Since the file is automatically generated, we recommend adding some unique identifier at
each call of abi.encodePacked , ensuring these collisions will not happen between different
functions.
This issue was acknowledged by the team, it was not changed due to gas costs but they
ensured it will be fixed in the future.

Enhancements

Wrap common Eternal Storage accesses in functions
While the Eternal Storage has many upsides, it has the big downside of removing a lot of
compiler information and adding verbosity to the contracts. Since variables are accessed by
strings, these strings can contain typos and get or set erroneous values without the compiler
noticing. The types themselves may also differ, with a variable that was set an int may be
mistakenly accessed as a uint.
While we checked and haven’t found any of these problems, it may become an issue if the
contracts develop further in the future. This in conjunction with the verbosity will definitely
make more difficult the development of these contracts. Proper testing can definitely help
here, but it may not be perfect and some issues may go unnoticed until deployment.
We recommend at the very least declaring all the strings as constants to remove the
possibility of typos. We also recommend wrapping common function calls to reduce the
verbosity and the possibility of other errors. For example, this chain of function calls:

setUint ( "totalSupply" , getUint ( "totalSupply" ). add ( _value ));

Maybe replaced with something like this:

increaseTotalSupply ( _value);

With the definition of being just the calls you had before:

function increaseTotalSupply ( uint256 _value ) internal {
setUint ( "totalSupply" , getUint ( "totalSupply" ). add ( _value ));
}

Which can further be improved by declaring the strings as constant:

string constant TOTAL_SUPPLY = "totalSupply";
function increaseTotalSupply ( uint256 _value ) internal {
setUint ( TOTAL_SUPPLY , getUint ( TOTAL_SUPPLY ). add ( _value ));
}

This was enhanced in multiple commits, with the last change being at commit
c3729ea216eb33f093e6e5b209378e4d6728bbd7.

Conclusion

The contracts were well written and had a good amount of documentation. We found some
issues but nothing critical. It’s always good to see code reuse, in this case, reusing
OpenZeppelin’s contracts, which are already well tested and commonly used