Timvi Smart Contract Audit

Reading Time: 5 minutes

  •  
  •  
  •  
  • 1
  •  
  •  
    1
    Share

Introduction

CoinFabrik was asked to audit the contracts for the Timvi project. 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 Timvi repository at https://github.com/TimviOfficial/Timvi. The audit is based on the commit 9324706d1160996f7847e1989c8567168261382e.

The audited contracts are:

As they are important to the system architecture and security of the solution, we have also audited a python script and dockerfile which provides the ethereum price. Solidity code referenced these files in an immutable manner. They are stored using IPFS and have the hash QmVXihTAKo3mwEHBMeM4EDG8MbWHFTvxoigdFAgmTneWra.

Url-requests.py

Dockerfile

The following analyses were 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 input function requirements.

Detailed findings

Critical severity

Oracle python script crashes

The file provided indirectly through IPFS provides the Ether price by taking the average from multiple sources, which is later used by Oraclize to send the result to the contracts. The following line gives a runtime error when executed, crashing the script and consequently preventing the execution of the function ethUsdPrice from the contract PriceGetter.sol, making all contracts fail.

We recommend fixing this bug as it’s imperative for the correct contract functionality.

A possible solution is:

But we think that the proper code to accomplish what we understand is your intention would be:

Update: Since the Timvi project now uses Chainlink as its main oracle solution this particular issue isn’t present anymore.

Using API Keys in public script:

Using api keys in price providers is a security risk because anyone can see and use these keys in an abusive manner causing the price provider temporarily to suspend the key making the request sent by the script fail.

{‘path’: ‘https://api.etherscan.io/api?module=stats&action=ethprice&apikey=91DFNHV3CJDJE12PG4DD66FUZEK71TC6NW’, ‘headers’: {}},

An attacker may wait for several price providers not using API keys to be offline and then begin the attack. If enough of them fail, it will cause the error described above.

You may consider using the url data source in a nested query since all the script does is perform https requests. It would avoid this kind of problems, be less expensive (computation 0.50$ vs URL 0.01$) and it could be possible to use android authenticity proof with this data source.

Update: Since the Timvi project now uses Chainlink as its main oracle solution this particular issue isn’t present anymore.

Usage of tx.origin for financial instrument ownership

Using tx.origin for the creation of bid, bond and ask instruments results in those being owned by the external account that initiated the transaction:

Afterwards, every privilege check that restricts who is able to manipulate these instruments compares the aforementioned address with msg.sender, the caller address. 

The problem is that these two are not necessarily the same. Not every account being used by users needs to be an external account. Some accounts are instantiated as contracts such as multi-signature wallets. If a multi-signature wallet was used, the instrument will be owned by the address of the user who initiated the multi-signature transaction, but will be checked against the address of the wallet itself, which does not coincide.

We asked the developers and they responded that the use of tx.origin was made to allow contract middleware to be used when creating these instruments. While the intention is now clearer, the problem described above still applies.

One possible solution could be to warn your users not to use multi signature wallets or other type of contract that would wish to interact with this project. Another solution is for the middleware to propagate the msg.sender using an additional function parameter for these functions. This obviously relies on the user to trust the middleware to correctly pass the msg.sender address, but the middleware needs to be trusted by the user regardless of this issue.

Update: This was fixed in commit: d589cd10963e627da340a49335ed60360f4e26d0

Medium severity

Wrong oraclize implementation

According to the oraclize documentation you should specify an authenticity proof type. Since the default authenticity proof type is none using proofStorage won’t have any effect because there is nothing to store. So the code:

Should in this case also use TLSNotary which is the only available option for ‘computation’ data source.

On the other hand, we also recommend to handle the ‘proof’ parameter sent to the callback function accordingly, otherwise data may be compromised.

Update: Since the Timvi project now uses Chainlink as its main oracle solution this particular issue isn’t present anymore.

Minor severity

Failing Ether transfer can deny multiFill service function

The function multiFill allows the administrator of the contracts to fulfill multiple orders by id in a single transaction:

The problem with this function is that Solidity’s transfer function can fail, as the amount of transfer’s gas is limited and may not be enough to fulfill a transaction to the user if it’s using a contract to interact with. For example, the user may have an expensive fallback function on the receiver end. If this happens, the entire transaction will be rolled back, and no order will get fulfilled. Moreover, the spent gas will be lost, and the admin will need to call the function again removing the conflicting address. Since the addresses that come after the failing address were not checked, they may contain another address that also fails, making this iterative process very costly for the administrator.

We recommend using the send function instead, which allows to check if the transaction was successful without rolling back. This allows you to fulfil and delete only the order on success, and continue processing the remaining addresses. The conflicting addresses that failed can be filtered by using events the administrator may catch off chain.

Update: This was fixed in commit 0a6e3d1277ecb1a460563de495b570bf0a2ced61

Logical mistake in function changeYearFee

The function changeYearFee changes the yearly fee to one that is passed as a parameter. The intention is to make the change only if the new fee is different. But instead of retrieving the old fee for comparison, it retrieves the expiration date, which is completely unrelated:

Both integers fall into different ranges, they will never be equal and this function will always execute the body of the condition. Because of that, the impact of this particular bug is low. However, it should be fixed as it’s a logical error. We recommend replacing the first line of the function with something similar to the following:

Update: This was fixed in commit 2bd5a7fd1fba883560fc8625c67b7b7c9cea51b1

Enhancements

Avoid duplicate code

There are many instances of code duplication in the project. Code duplication is not usually recommended, it can lead to bugs if is not handled correctly. It also makes the overall project harder to understand as there are more lines of code available to parse.

For example, you have multiple modifiers that do checks, but in many instances you have the modifier content hardcoded at the beginning of the function instead:

There are also contract files that are very similar and only differ in a couple of lines. As is the case with ExchangeService.sol and LeverageService.sol.

Update: This was enhanced in commit a89a9a6e3b5b88b4b57a816e590fb95d34590281



  •  
  •  
  •  
  • 1
  •  
  •  
    1
    Share