Skip links
Web3 Audit Solutions
CoinFabrik
Lif Token

Líf Token Smart Contract Audit

Author
Published on:
Published in: Blog

By Pablo Yabo and Ismael Bejarano

Introduction

CoinFabrik’s smart contract audit team was requested to review contracts of the Líf Token for the Winding Tree platform. In the following sections, we will write the discoveries of this process.

The contracts audited are from the repository https://github.com/windingtree/LifToken, the audit was started at commit fa503ebe495b7ec11d19333731b1f506e897929b, and completed after commit b41662e398e4bafb4e6c08a08b58ad02c52dec94. Some issues were fixed in between them, we have left those issues in the report and marked them as fixed.

Overview

The contract under review consists of

  • LifCrowdsale: The contract that will manage the Crowdsale of the Líf Platform Token
  • LifToken: The ERC20 token for the Líf Platform
  • VestedPayment: Used by the Crowdsale to assign tokens to the Líf Founders, Líf Foundation and Líf Development Team.
  • LifMarketValidationMechanism: Deployed by Crowdsale to manage the extra funds to guarantee a buy price for the token for a period 24 or 48 months.

The LifCrowdsale is an uncapped token sale, which lasts two weeks at a fixed price. It has a minimum target of 5M USD. If the minimum is not reached every contributor will be able to claim their ether back. Total contributions below 10M USD will be transferred to the Foundation wallet.

If total contributions exceed 10M USD, the funds above this amount go to a Market Validation Mechanism. This mechanism will be used to guarantee a minimum price of Lif Tokens. If the contributions do not reach 40M USD, it will be configured for 24 months, and in case the crowdsale contributions exceed 40M USD it will last 48 months. The Market Validation allows the Foundation to claim a percentage of the Market funds following an exponential distribution.

lif crowdsale chart

We reviewed the contract for common attacks and errors, checked reentrancy problems, verify mathematical operation overflows, method access declarations, and checked the correct application of mathematical formulas in the Market Validation Mechanism.

We also verified it follows current smart contracts best practices. The source code documentation is good and is extensively commented. It generates events for token transfers to help block explorers and wallet show correct balances and operations.

Discoveries

Severe

Error formula in Market Validation Mechanism

In LifMarketValidationMechanism.sol contract we have the following formulas:

  • InitialWei: Is the initial funding of the LifMarketValidationMechanism.
  • CurrentCirculation: Is the current supply of the token contract.
  • OriginalSupply: Is the original supply of the token contract.
  • AccumulatedPercentage: Is the maximum percentage that can be retrieved by the Foundation at a point in time.
  • PriceFactor: It is a constant used to avoid rounding in Solidity because its lacks of floating point operations.

The first formula is the maximum amount of Ether claimable by the Foundation, and the second is the maximum amount that can be returned in exchange for tokens to the users.

The problem appears when someone refund tokens, this will decrease the supply of tokens because the tokens are effectively burned.

Let’s suppose that we have InitialWei = 2000, OriginalSupply = 1000, and we are in a period where AccumulatedPercentage = 10% and have PriceFactor = 1 for simplicity.

The total amount that can be refunded to the token holders is:

And the maximum claimable by Foundation will be 200.

If all tokens are refunded then the balance of the contract will be 200 (InitialWei 2000 – 1800), enough to pay the Foundation claimable. But after the tokens are refunded, the token supply will be zero, meaning the Foundation will not be able to claim any of those funds until the periods’ end (24 or 48 months).

If a user exchanges 50 tokens (5% of the total) they will receive 90 wei. The balance remaining in the Market contract will be 1910 (InitialWei 2000 minus 90), and the token circulation will be 950 (OriginalSupply 1000 minus 50). After the transaction the maximum claimable by the Foundation will be:

instead of the 200 wei.

Let’s suppose we are in the next period with 20% of AccumulatedPercentage. Then we have the following results:

Total claimable by users is 1520 and maximum claimable by the Foundation is 380. The total claimable is 1900, but the current balance is 1910. The difference cannot be claimed until the end of the Market Validation period.

If a user again exchanges 50 tokens they will receive 80 wei. The remaining Market balance will be 1830 wei.

If the next period has 30% of AccumulatedPercentage the total claimable will be 1800 wei, 1260 = 2000*0.7*900/(1*1000)  claimable by users 540 = 2000*0.3*900/(1*1000) plus claimable by the Foundation. Again, this difference cannot be claimed until the Market Validation has finished. Every time the token circulation diminishes a percentage of the balance will not be claimable until the Market Validation has ended.

A possible solution is to reset the values used in the formulas at the begin of each period so the whole balance will be available for refund, and it will not use the original token supply but the existing supply. But this is an important change to the dynamic of the formula behind the Market Validation Mechanism that requires more exhaustive verification.

Medium severity

Using tx.origin

It is not recommended to use tx.origin by Ethereum developers, Vitalik Buterin has expressed at StackExchange “Do NOT assume that tx.origin will continue to be usable or meaningful”. It is recommended to replace it by msg.sender.

It is used at several places in LifToken.sol. For example in method transferData line 76.

 function transferData(address to, uint value, bytes data) whenNotPaused returns (bool) {

   require(to != address(this));

   // If transfer have value process it

   if (value > 0) {
    balances[tx.origin] = balances[tx.origin].sub(value);
    balances[to] = balances[to].add(value);
   }

   if (to.call(data)) {
      TransferData(tx.origin, to, value, data);
      return true;
   } else {
      return false;
   }
}

Fixed on commit 4e0daabf3826b9cbb9bba28a6989d328e4ac85f0

Inconsistent use of contract balance

Some methods of LifCrowdsale like funded use weiRaised as the raised amount. In function funded in LifCrowdsale.sol line 359.

 function funded() public constant returns (bool) {
   assert(weiPerUSDinTGE > 0);

   return weiRaised >= minCapUSD.mul(weiPerUSDinTGE);
 }

But in forwardFunds (LifCrowdsale.sol line 264) it uses this.balance

 function forwardFunds() internal {

   // calculate the max amount of wei for the foundation
   uint256 foundationBalanceCapWei = maxFoundationCapUSD.mul(weiPerUSDinTGE);

   // if the minimiun cap for the MVM is not reached transfer all funds to foundation
   // else if the min cap for the MVM is reached, create it and send the remaining funds
   if (this.balance <= foundationBalanceCapWei) {
     foundationWallet.transfer(this.balance);
     mintExtraTokens(uint256(24));
   } else {
     uint256 mmFundBalance = this.balance.sub(foundationBalanceCapWei);
   }
 }

Contracts only accept ether through methods marked as payable. In the LifCrowdsale the only methods marked as payable are the fallback function and buyTokens. Since the fallback function calls buyTokens all contributions are correctly accounted for.

But there are methods like a contract selfdestruct or setting a contract address as the miner of a block to increase the balance without the contract being aware of it.

This can cause an inconsistency between this.balance and weiRaised. We found no direct exploit to this inconsistency. But when the total is close to the next target it can be manipulated to make some parts believe it has reached it when it is not. But in all cases, the contract has to be funded already.

We recommend to only use weiRaised for consistency in all the checks and only use this.balance when doing the final transfer ensuring that you have at least this.balance >= weiRaised.

Fixed on commit 6d8adb453a24a68011da8d20d5a71acbb44605d5

Double Spend

The function approveData in LifToken.sol is vulnerable to a double spend attack. This attack occurs when a user A approves user B a limit to transfer of X tokens. Then, the same user A wants to approve a Y amount of tokens to user B. If B monitors unmined transactions can quickly create a transaction to spend the X tokens previously approved. If B transaction is mined first, then B would be able to spend Y tokens even though B just spend X. In this example, A wanted to change the approved value from X to Y but B can finally spend X+Y.

The approveData is not protected against this attack:

function approveData(address spender, uint256 value, bytes data) whenNotPaused returns (bool) {
   require(spender != address(this));

   allowed[tx.origin][spender] = value;

This should be modified to

function approveData(address spender, uint256 value, bytes data) whenNotPaused returns (bool) {
   require(spender != address(this));
   require((allowed[tx.origin][spender] == 0) || (value == 0));

   allowed[tx.origin][spender] = value;

Protection for unintended token transfers

The contract LifToken.sol and LifCrowdsale.sol are not protected against accidental transfers of tokens.

In the past, a user was able to unintentionally send tokens or ether to a contract and if the contract is not prepared to refund them they will remain blocked in the contract.

Now solidity has payable modifier that prevents a contract from receiving ether from an unintentional transaction. But there’s no such mechanism for a transaction involving token transfer.

We recommend implementing a mechanism that allows the contract owner to refund tokens unintentionally sent to the LifCrowdsale or LifToken.

   function refundTokens(address _token, uint _value) onlyOwner {
       ERC20 token = ERC20(_token);
       token.transfer(msg.sender, _value);

       RefundTokens(_token, msg.sender, _value);
   }

Required solidity version

Reviewed contracts require solidity version 0.4.13, but this version of the compiler is known to have two important vulnerabilities:

  • DelegateCallReturnValue “The return value of the low-level .delegatecall() function is taken from a position in memory, where the call data or the return data resides. This value is interpreted as a boolean and put onto the stack. This means if the called function returns at least 32 zero bytes, .delegatecall() returns false even if the call was successuful.”
  • ECRecoverMalformedInput “The ecrecover precompile does not properly signal failure for malformed input (especially in the ‘v’ argument) and thus the Solidity function can return data that was previously present in the return area in memory.”

We suggest requiring at least version 0.4.15 of solidity compiler.

Fixed on commit c5630888c9331cb952218bfce81dce167a2b118f

Low severity

Unassigned result

In method addPrivatePresaleTokens of LifCrowdsale.sol line 241, the accumulation of presale amount is not stored correctly

 function addPrivatePresaleTokens(
   address beneficiary, uint256 weiSent, uint256 rate
 ) onlyOwner {
   // ...
   totalPresaleWei.add(weiSent);

It should say:

   totalPresaleWei = totalPresaleWei.add(weiSent)

Since this value is only used for statistical purposes it is no serious.

Fixed on commit 3ac1d25391d42957938e1f6538d062172280b92c.

Complex Finalize

The finalize operation creates several contracts to manage Founders, Foundation and Developers tokens and depending on the total raised it can create a LifMarketValidationMechanism contract.

Deploying a contract is an expensive operation, and deploying several contracts will require a large amount of gas. In the case of a congested network, it can delay the crowdsale end.

We saw in the repository that there are a couple of tests of crowdsale finalization. We suggest adding more tests for the several possible outcomes, and to make sure that the finalize function can be called with the gas limit on mainnet.

A possible solution in the case there is not enough gas is to move the creation of some contracts to the deployment and set only configuration parameters on the finalization step.

Missing Transfer events

The additional methods transferData and transferDataFrom in LifToken do not generate Transfer() event. The methods transfer and transferFrom in the ERC20 specification and implemented by OpenZeppelin does generate an event for a transaction.

For example in LifToken.sol line 76, method transferData does not generate an event when the transfer is done:

 function transferData(address to, uint value, bytes data) whenNotPaused returns (bool) {

   require(to != address(this));

   // If transfer have value process it

   if (value > 0) {
     balances[tx.origin] = balances[tx.origin].sub(value);
     balances[to] = balances[to].add(value);
   }

   // ...

This could result in wallets or block explorers (such as Etherscan) relying on Transfer events to miss these transactions. Accordingly, users of these services may complain since they will not see these token movements nor their balances updated correctly.

Similarly, the burn method in LifToken.sol line 133 does not generate a Transfer event.

 function burn(uint256 _value) public whenNotPaused {
    require(_value > 0);
 function burn(uint256 _value) public whenNotPaused {    
   require(_value > 0);

   address burner = msg.sender;

   balances[burner] = balances[burner].sub(_value);
   totalSupply = totalSupply.sub(_value);

   Burn(burner, _value);
 }

It is useful for some wallets and block explorers to also signal a Transfer event with destination address null (0x0), o they can show the correct balance in such circumstances.

Fixed on commit 4e0daabf3826b9cbb9bba28a6989d328e4ac85f0

Missing access methods

Explicitly set the methods access property, they can be external, public, internal or private. By default solidity assumes methods are public. This can cause a serious issue if the developer forgets to add the ‘internal’ in the declaration. It is recommended to always declare methods explicitly with the minimum required access.

For example in Crowdsale.sol line 208, buyTokens is not clearly marked: 

   function buyTokens(address beneficiary) payable {

Also, in LifToken.sol line 107 transferDataFrom 

   function transferDataFrom(address from, address to, uint value, bytes data) whenNotPaused returns (bool) {

In LifMarketValidationMechanism.sol function calculateDistributionPeriods line 119

 function calculateDistributionPeriods() {

Fixed on commit 197bf77f8b46a91f9d4039ae6d7bc9760d5b6ab3

Notes

Arbitrary calls

Any user can use method approveData to make arbitrary calls on behalf of the token contract, it is not necessary to have a positive balance. Users with a positive balance can call transferData and transferDataFrom to also make arbitrary calls.

For example, approveData in LifToken.sol line 50 does the following:

 function approveData(address spender, uint value, bytes data) whenNotPaused returns (bool) {

   require(spender != address(this));
   allowed[tx.origin][spender] = value;

   if (spender.call(data)) {
     ApprovalData(tx.origin, spender, value, data);
     return true;
   } else {
     return false;
   }
 }

The spender.call(data) allows users to call any other contract on behalf of the LifToken contract so target contract will see LifToken as sender.

We recommend limiting calls to the accepted addresses or to follow ERC223 recommendation to call only a specific method. Also, approveData should be limited only to users with a positive balance at least.

Enhancements

  • The value of Burn event is indexed when it is not needed. Removing indexed to value parameter will save some gas. In LifToken.sol line 142 we have:

    event Burn(address indexed burner, uint indexed value);

    Fixed on commit e796dfedef11df65dc365aa237743ad82b5a77ed

  • Some functions don’t have an explicit visibility qualifier. Adding them minimizes the chance of mistakes in function declarations and security issues. E.g: all functions in the Líf token contract except for burn.
  • LifCrowdsale, LifToken and LifMarketValidationMechanism are well documented and are using Natspec (Ethereum Natural Specification Format), but VestedPayment comments and documentation do not follow the same standard.
    This issue was fixed on commit 29cfc30d7abd92c4ca75b336c05938700769f78c.
  • Consider converting some of the validation methods in LifCrowdsale.sol into modifiers. For example, validPurchase, hasEnded and funded are only used as modifiers in other methods.
  • To have a total of the reimbursed ether in MarketValidationMechanism. It has the total of tokens burned but lacks total of ether reimbursed.

          Fixed on commit 4c26cb8e41f7be32e9f8102020683c1505f49404

  • Generate an explicit event when tokens are reimbursed in MarketValidationMechanism. Currently, a Burn events are generated, but since the burns method is public it is not necessarily from the MarketValidationMechanism. Same when part of the ether is claimed by the Foundation.

          Fixed on commit fcc844164d69cbc50a9dbc150768edc3d228fc13

Summary

The code has very good documentation, is extensively commented and has several unit tests for most use cases.

It uses the well know OpenZeppelin contracts as the base for its token and the safe arithmetic operation.

Most issues pointed in this document were addressed by the development team. We suggest being very careful with the Market Validation Mechanism to avoid the reported issues.

References

You may also like