Skip links
Request an Audit
CoinFabrik

Timvi Smart Contract Audit

Author
Published on:
Published in: Blog

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:

  • Logic ERC-721 Non-Fungible TBox Token contract. The main logic contract.
  • TimviSettings Settings store.
  • TimviToken ERC-20 Timvi stablecoin.
  • PriceGetter ETH/USD price oracle contract (using Oraclize).
  • ExchangeService exchange ETH to TMV according to the system’s rate.
  • LeverageService receive ETH for a collateral in ETH.
  • BondService withdraw and sell TMV to get ETH.
  • Gate This service allows to exchange TMVs for ETH.

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.

    return int(prices[0] * 100000)

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

A possible solution is:

            return int(prices[0])*100000

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

            return float(prices[0])

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:

        Bid memory _bid = Bid(
            tx.origin,
            msg.value,
            _percent
        );

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:

oraclize_setProof(proofStorage_IPFS);

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

provable_setProof(proofType_TLSNotary | proofStorage_IPFS);

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:

    function multiFill(uint256[] calldata _ids) external onlyAdmin() payable {
        emit Funded(msg.value);
        for (uint256 i = 0; i < _ids.length; i++) {
            uint256 id = _ids[i];
         require(orders[id].owner != address(0), "Order doesn't exist");
            uint256 tmv = orders[id].amount;
            uint256 eth = tmv2eth(tmv);
            require(address(this).balance >= eth, "Not enough funds");
            address payable owner = orders[id].owner;
            delete orders[id];
            IToken(settings.tmvAddress()).transfer(timviWallet, tmv);
            owner.transfer(eth);
            emit OrderFilledPool(id, owner, tmv, eth);
        }
    }

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:

    function changeYearFee(uint256 _id, uint256 _yearFee) internal {
        uint256 _oldYearFee = bonds[_id].expiration;
        if (_oldYearFee != _yearFee) {
            require(_yearFee <= 10000, "Fee out of range");
            bonds[_id].yearFee = _yearFee;
        }}

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:

uint256 _oldYearFee = bonds[_id].yearFee;

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:

    function changeOwner(uint256 _id, uint256 _deposit, uint256 _expiration, uint256 _yearFee)
        external
        payable
    {
        require(bonds[_id].owner == msg.sender && bonds[_id].emitter == address(0), "You are not the owner ot bond is matched");
        changeDeposit(_id, _deposit);
        changeExpiration(_id, _expiration);
        changeYearFee(_id, _yearFee);
        emit BondChanged(_id, _deposit, 0, _expiration, _yearFee, msg.sender);
    }

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

Conclusion

The contracts are straightforward and have an adequate amount of documentation. We found multiple issues that would prevent the contract correct functionality. Even though the project has unit tests, some of the problems we found won’t show with current implemented testing. For example an external functionality like the python oracle script or the multi-signature wallet problem won’t show and will require a more comprehensive testing strategy. We believe a project of this magnitude would benefit from having a testing phase with a wider coverage and more user interactivity. Once changes are implemented we would suggest conducting a second audit.

Disclaimer: This audit report is not a security warranty, investment advice, or an approval of the Timvi project since CoinFabrik has not reviewed its platform. Moreover, it does not provide a smart contract code faultlessness guarantee.

 

You may also like