Skip to content

Instantly share code, notes, and snippets.

@loiluu

loiluu/digix-report.md

Last active April 20, 2018 06:29
Show Gist options
  • Star 2 You must be signed in to star a gist
  • Fork 1 You must be signed in to fork a gist
  • Save loiluu/0363070e1bada977f6192c8e78348438 to your computer and use it in GitHub Desktop.
Save loiluu/0363070e1bada977f6192c8e78348438 to your computer and use it in GitHub Desktop.
Download ZIP
Audit report for Digix's smart contract platform
Raw
digix-report.md

Table of Contents

=================

1. Introduction

In the dates of May 21st 2017 to June 18th 2017, Digix engaged Yaron Velner and Loi Luu to perform security audit for Digix smart contracts. The audited code was taken from three private github repositories:

  1. core2 repository: commit 3ef8bd454322c8133b4686efcaa9543bb16dfe05.
  2. cacp-contracts repository: commit b190ec94af728d4c973dd9b8f03008ea5ee035b9.
  3. solidity-core-libraries repository: commit 9a787b3502d2aa9c70b43dd3bb855a3018b74fac.

1.1 Overview of the Digix platform

Digix is an asset-tokenisation platform built on Ethereum. It allows users to purchase gold backed tokens, which they can transfer and convert to gold.

The Digix platform collects demurrage fees from its users in return of storing the physical gold bars. Hence, its token contract requires sophisticated implementation for the demurrage fees and cannot reuse existing standard ERC20 token implementation.

1.2 Scope of audit

In the audit we reviewed the smart contracts that implement the token mechanism, which includes an implementation for all ER20 token operations as well as conversion (recast) of a token into gold (to be precise, a registration on blockchain of the recast operation).

In particular the following files were reviewed:

  1. CoreStorage.sol
  2. TokenInterface.sol
  3. TokenLoggerCallback.sol
  4. TokenInfoController.sol
  5. TokenApprovalController.sol
  6. TokenRecastController.sol
  7. TokenTransferController.sol
  8. AssetDisplay.sol
  9. AssetExplorerInterface.sol
  10. Controller.sol
  11. AssetStatus.sol
  12. MathUtils.sol
  13. ACOOwned.sol
  14. ACGroups.sol
  15. ContractResolver.sol
  16. ResolverClient.sol
  17. AssetIndexStorage.sol
  18. AssetInfoStorage.sol
  19. AssetCustodianController.sol
  20. LibraryDLL.sol

1.3 Terminology

This audit uses the following terminology. Note that we only rank the likelihood, impact and $Severity$ for bug/security-related issues.

1.3.1 Likelihood

How likely a bug is to be encountered or exploited in the wild, as specified by the OWASP risk rating methodology.

1.3.2 Impact

The impact a bug would have if exploited, as specified by the OWASP risk rating methodology.

1.3.2 Severity

How serious the issue is, derived from Likelihood and Impact as specified by the OWASP risk rating methodology.

2. Summary of Findings

Overall, the code is clearly written, and demonstrates effective use of abstraction, separation of concerns, and modularity. Digix development team demonstrated high technical capabilities, both in the design of the architecture and in the implementation.

We found one critical issue and several additional issues that require the attention of the Digix team. Given the subjective nature of some assessments, it will be up to the Digix team to decide whether any changes should be made.

2.1 Major Issues

2.1.1 Sending tokens to self increases users' balance

  • Likelihood: high
  • Impact: high

This is a critical bug that allows user to create tokens out of thin air. We recommend blocking the option of sending tokens to self by checking if msg.sender == _to. Blocking can be done either by a throw or an error return, or simply by a return that skips the operation. The bug is applicable for both transfer and transferFrom functions. Making the validation at TokenTransferController.sol would take care of both cases. In addition, when msg.sender == _to, the demurrage fees are not properly collected.

2.1.1.1 Sending tokens to collectors make them pay demurrage fees

  • Likelihood: high
  • Impact: low

User can send 0 tokens to one of the collectors addresses (Collectors::demurrage, Collectors::recast, Collectors::transfer). It will make them pay demurrage fees. Severity is low as the demurrage collectors can later compensate themselves. The bug is easy to fix. Especially, the transfer and transferFrom functions should verify that destination addresses are not Collectors::demurrage, Collectors::recast and Collectors::transfer. Given the upgradability mechanism in the code, an additional security can be obtained by ensuring the source address is not either Collectors::demurrage, Collectors::recast or Collectors::transfer.

2.1.2 The implementation of demurrage fees is not ideal and can incentivise unwelcome behaviors

  • Likelihood: medium
  • Impact: medium

The potential importance of this bug is high, but it could just be a misunderstanding from our side. We identify two issues in the implementation:

  1. calculate_demurrage_time does not calculate next payment date properly. If in the middle of the day a user performs two transactions, he will be charged for a full day fee. The core reason for the bug is _next_payment_date = subtract(add(_last_payment_date, _demurrage_seconds), _remainder); which to our understanding should be
    _next_payment_date = add(_last_payment_date, _demurrage_seconds);

  2. calculate_demurrage_fee value is too high when paid for long durations. In the current implementation the fees are amount * daily_fees * num_days. To our understanding it should be sum_{i=1}^{num_days} amount * (1-daily_fees)^{i-1}*daily_fees.

The current implementation suggests that daily fees are the same over time (rather than exponentially reduced). Thus, the current demurrage fees for a given amount and days could be different depending on user actions. In particular, suppose the daily demurrage fees are 1% and a user has 100 tokens. With the current implementation, the honest user will have to pay 2 tokens for the fees after 2 days. However, a more rational user can exploit this demurrage fees scheme to get less fees. Specially, the rational user will choose to pay fees every day, so the total fees he pays after 2 days is 100 * 0.01 + 99*0.01 = 1.99 tokens. This could be significant over long periods. As a result, the current implementation suggests the user to strategically perform dummy operations, in order to pay lower fees.

2.1.3 A bug in transfer prevents recipient fees from being collected

  • Likelihood: high
  • Impact: medium

In file TokenTransferController.sol, the value of _d.recipient.demurrage.fee is never set. A bug in line 141 set _d.sender.demurrage.fee instead.

2.1.4 Lack of testing to token operations

At the time of the audit, there were not tests for the token operations. This is a major issue since it is important to test all the code before deployment.

2.2 Implementation Recommendations

2.2.1 Use of doubly linked list in code

  • Likelihood: low
  • Impact: medium

The Digix team self-implemented linked list in solidity code in order to store and display existing gold bars to users. Implementing linked list from scratch is notoriously error prone.

We recommend the Digix team to implement a set (rather then list) using a simple array. The main advantage is that set is a data structure that has much easier (and thus safer) implementation. Another advantage is that the iteration over a linked list could return inconsistent results when traversing backwards. On the other hand, in an array-based implementation for the set, elements can be accessed quickly and the set returns consistent results. The main disadvantage of the set data structure is that the order of fetched elements is arbitrary, while linked list maintains a FIFO order of gold tokens.

In addition, the current implementation is not scalable, and simple append function will run out of gas if list length is in the order of 10,000. We recommend a solution for the scaling problem in Section 5.

2.2.2 Implementation of token approve function does not mitigate known attack

  • Likelihood: medium
  • Impact: low

The current implementation does not mitigate the problem. We recommend changing line 35 in TokenApprovalController to

if (_a.pre > 0 && amount > 0 ) throw;

or alternatively, just returns false.

3. Design Clarifications and Recommendations

We have discussed the following design details with the Digix team.

3.1 Transaction fees can be avoided.

  • Likelihood: unknown
  • Impact: medium

Digix has confirmed that they are aware of the fact that transaction fees could be avoided by creating a token backed by Digix gold token.

Although there are always demurrage fees that Digix will charge for storage and maintenance of the physical gold bars, the tx fees could be collected by a competing platform, which offers lower or even zero fees. We understand that DGD holders (including Digix) share all the transaction fees, and only part of the demurrage fees. Hence, its bad for DGD holders if the tx fees are collected by competing platforms.

3.2 Transaction fees are reduced from sent amount

Users should be aware that fees are reduced from sent amount, and they should take it into account when using the token for payment. This behavior is non-standard in ERC20 token contracts, however is needed to support Digix business model.

3.3 Digix has full control over the tokens

The contract is fully upgradeable and as a result Digix administrators can in anytime change the behavior of its token, including increasing or decreasing user balances at will.

3.4 Asset explorer has a risk of presenting no longer available assets

  • Likelihood: medium
  • Impact: low

A race condition between users who recast their tokens is unavoidable. As a result, the recast operation could fail even in a normal scenarios. For example, two users choose which asset (i.e. gold bar) they want to recast, and they both will likely choose the first gold bar to recast. If someone recasts first, the other will fail. It is because the asset explorer needs to iterate through all assets. This problem could be mitigated by a front end implementation that will try to present different orders of assets to different users.

3.5 Suggestion: allow token contract to drain its received funds

History suggests that users mistakenly transfer tokens or Ether to token contracts. Consider having owner_transfer(address tokenAddress, amount) function that drains token contract from tokens and Ether it holds.

4. Detailed report

In this section we report issues, notes and some improvement suggestions of lower importance.

4.1 General notes

4.1.1 Use of strings as enum values

Strings like "i:token" should be replaced by

string constant I_TOKEN = "i:token";

Using precise string prevents compiler from catching typos. As these strings directly dictates the permission level of a contract, it would be beneficial to replace them with constants.

4.1.2 Child initialize parent variable

A better practice is to replace code like:

function TokenInterface(address _resolver)
{
   resolver = _resolver;
   init("i:token");
}

with a code where resolver is initialized in the parent class constructor. In general, it is a better practice if objects do not access the variables of their parent directly.

4.1.3 Use of create(0,0,0) to create unique addresses

A more elegant way is to have uint256 counter variable in storage and return address(counter++). create(0,0,0) creates a blockchain bloat and is more expensive in gas.

4.1.4 Explicitly returning the declared return variables is not necessary

For example in:

  function read_demurrage_data()
           constant
           returns (uint256 _collector_balance,
                    uint256 _base,
                    uint256 _rate,
                    address _collector)
  {
    _collector_balance = system.users[system.collectors.demurrage].raw_balance;
    _base = system.fees.demurrage.base;
    _rate = system.fees.demurrage.rate;
    _collector = system.collectors.demurrage;

    return (_collector_balance,
            _base,
            _rate,
            _collector);
  }

The last part, namely,

    return (_collector_balance,
            _base,
            _rate,
            _collector);

can be omitted. This can shorten some of the info functions by half and prevent potential bugs.

4.1.5 Unnecessary use of uint8

Using uint8 is more expensive then using uint256, and solidity compiler had at least one bug in the past for wrong handling of non 256-bit types. In your code you mainly use uint8 to store an enumeration value. We recommend to change it to uint256.

4.1.6 Style inconsistency when defining functions as public

Declaring a function as public is already done by default. There is no problem doing it, but in most files you don't explicitly define functions as public while in some places you do.

4.2 Per file notes

4.2.1 CoreStorage.sol

  1. The name total_supply could be renamed to total_token_supply in order to be consistent with total_grams.
_demurrage_collector := create(0,0,0);
_recast_collector := create(0,0,0);
_transfer_collector := create(0,0,0);
_minter := create(0,0,0);

could be replaced by

_demurrage_collector = address(0);
_recast_collector = address(1);
_transfer_collector = address(2);
_minter = address(3);

4.2.2 TokenInterface.sol

We recommend to implement a drain function.

4.2.3 TokenLoggerCallback.sol

In log_mint: logging the event as a Transfer event is not a standard. On the other hand, there are no standard approaches for this. At the very least, log a dummy from address.

4.2.4 TokenInfoController.sol

Not sure why you bother to define BalanceInfoData and Transformable structures, as they are only used once.

4.2.5 TokenApprovalController.sol

  1. Not clear why you define an Allowance structure. You can just use stack variables.
  2. log_approve should log _a.post, rather then amount.
  3. Your implementation does not mitigate the problem from https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit#

4.2.6 TokenRecastController.sol

Consider logging demurrage and recast fees as transfer events

4.2.7 TokenTransferController.sol

  1. DemurrageCollectorData and TransferCollectorData structures are the same (not a bug, but consider defining only a single structure).
  2. When sending tokens, fees are reduced from sent amount (rather than from sender balance). This is different behavior in comparison to sending ether or other ERC20 tokens. Though it is similar behavior to a bitcoin transaction.
  3. d.sender.demurrage.fee is set twice. This is a bug! second time should be d.recipient.demurrage.fee (line 141).
  4. Consider logging demurrage fees as transfer events
  5. Block sending to self. Your implementation increases the sender's balance if he transfers to himself, this is a bug. In addition demurrage is calculated wrongly when sending to self.
  6. Block sending to collectors. Consider blocking sending from collectors.

4.2.8 MathUtils.sol

  1. We suspect calculate_demurrage_fee and calculate_demurrage_time implementations are different from what was explained to us as aforementioned.
  2. rate_of function is never used in your code. It is hard to evaluate if it is implemented correctly.

4.2.9 AssetDisplay.sol

Casting to uint8 can and should be avoided.

4.2.10 AssetExplorerInterface.sol

All info functions should be defined as constant.

4.2.11 Controller.sol

No issues were found.

4.2.12 AssetStatus.sol

No issues were found.

4.2.13 ACOOwned.sol

No issues were found.

4.2.14 ACGroups.sol

No issues were found.

4.2.15 ContractResolver.sol

The role of locked variable is not clear. It is read in ResolverClient.sol file, but its role there is also not clear, as all the functions that are reading it are not in use anywhere in the code.

4.2.16 ResolverClient.sol

lines 13-14:

  /// Make our own address available to us as a constant
  address public CONTRACT_ADDRESS;

The variable is not a constant and its naming convention is not consistent with other variables. Moreover, this variable is always set to this. Hence it is not clear why it is needed

4.2.17 AssetIndexStorage

  1. Use of create(0,0,0) could be avoided.
  2. update_swap_between_collections and update_vendor_put_asset performs two operations on linked list. In both functions the second operation should be avoided if first operation failed. And first operation should be roll-backed when second operation fails.
  3. In read_next_in_collection and read_previous_in_collection you should first check that current element is valid before calling the linked list functions.

4.2.18 AssetInfoStorage.sol

  1. Elements are only added to documents, hence a simple array could replace the linked list.
  2. In read_asset_previous_document and read_asset_next_document you should verify that current document hash is valid.

4.2.19 AssetCustodianController.sol

  1. In put_asset_mint: If core_storage().update_asset_mint fails then marketplace_storage().delete_custodian_delivery_after_accept should not be called. Similarly, if info_storage().update_mint_asset fails then don't call index_storage().update_custodian_mint_asset.
  2. get_asset should be defined as constant.

4.2.20 LibraryDLL.sol

  1. We recommend to exclude this file from the project and implement set instead.
  2. uint, bytes32 and address, all takes the same amount of memory (32 bytes). Implementing the same functionality three times (one for each data type) could be avoided by defining all of them as uint (or bytes32) and require the callee to cast his data to uint (bytes32). Or have some encapsulation inside LibraryDLL.sol code.
  3. If called on-chain, find could quickly run out of gas. Consider implement find with a simple mapping between value and index.
  4. append would run out of gas when calling find if list is too long.
  5. In remove function: consider verify that _index is valid. Currently if _index is invalid then the entire list can be corrupted. Alternatively, make it an internal function that is only called by remove_item. In addition, consider adding a sanity check that self.count is not already 0.
  6. remove_item: again, find could run out of gas if list is too long.
  7. _item_index != 0. Replace 0 with NONE.
  8. In valid: you explicitly rely on overflow to happen if _index == NONE. It is not very pretty.
  9. In previous_item and next_item: why do you use 0 instead of NONE?
  10. Same comments are applicable for address and bytes32 implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment