Zeppelin Blog
Published in

Zeppelin Blog

Fuel Token Audit

The Vanbex team asked us to review and audit their Fuel Token (FUEL) and crowdfund contracts. We looked at the code and now publish our results.

The audited contract is in the etherparty/FUEL-Contracts repository. The version used for this report is the commit .

Here’s our assessment and recommendations, in order of importance.

Update: The Vanbex team has followed most of our recommendations and updated the contracts. The new version is at commit .

Critical Severity

Leftover presale supply can be left unrecoverable

When is deployed, the total supply of 1 billion is divided into allocations destined for various purposes, including presale and crowdfund. The intention is for all of the supply to be eventually sold. Out of the presale and crowdfund allocations, tokens which aren’t sold during the corresponding stage are meant to be eventually transferred to the platform address, which would later sell them via the Etherparty platform.

When the presale is finalized (via ), its remaining allocation is transferred to the crowdfund. When the crowdfund is finalized (via ), its remaining allocation is finally transferred to the platform. There is an implied ordering between these two calls which is not enforced in the smart contract code. As a consequence, the crowdfund could be mistakenly finalized before the presale, and any remaining tokens allocated for the presale would then be unrecoverable (for they would be transferred to the crowdfund address with no way to get them out anymore).

To prevent this, add the precondition to the function.

Update: Fixed in .

High Severity

Error-prone code duplication

There is an extremely high level of code duplication in the contracts. Although it is not causing a problem in the current version of the code, it is highly likely that small revisions will create bugs.

For example, there are six different implementations of transfer (for example in and ), and there are other places where balances are modified with confusing semantics. In the case of transfers, consider using the contract’s function directly.

A different case of code duplication is the interface used by the contract. It could easily get out of sync with the actual contract. Consider importing the file instead, and directly using the actual contract.

In general, be on the lookout for code duplication and consider refactoring the current code by creating internal helper functions with clearer semantics.

Update: Regarding transfers, the team replied: “The and methods have requirements not available in the method. The crowdfund must be able to transfer before the tokens are able to transferred by the public, and the vanbex team tokens are not being decremented from a balance. We prefer this solution to cluttering up the transfer method itself.” The interface issue was fixed in .

Presale balance may not be deliverable after crowdfund starts

The function requires that the receiver have a balance of zero. There is a comment which states that all individual contributions will be aggregated, so that for each contributor the function will only be called once.

However, in the same way as the critical issue we reported, it is implied that the presale balance will be distributed before the crowdfund even begins. It could be the case that a user buys tokens both in the presale and in the crowdfund, and if the presale balance is not delivered before the latter, the presale delivery would fail. Furthermore, since presale deliveries will be made in batches, it would likely be very difficult to debug the cause of one batch failing.

Consider enforcing that all presale deliveries happen before the crowdfund starts, or lifting the requirement that an account should have no balance before transferring presale tokens to it.

Update: Fixed in .

Medium Severity

Unclear semantics for Transfer

The ERC20 token standard specifies the event to log transfers. The specification is informal, and there is not an exact characterization of when the event should be emitted. However, since this event is relied on by applications, it is in the interest of the project to try to respect the semantics that applications expect. This will pay off in a better user experience, and probably lower operational costs.

The event is used throughout in unexpected and inconsistent ways. When vested tokens are released (via ), a transfer from the token contract itself is logged. This is conflicting with the expected semantics of the event, because the token contract didn’t have those tokens as part of its balance to begin with. In , the event is emitted with the crowdfund as the source, but there was never a event with the crowdfund as the destination.

Consider emitting this event with the zero address as the field, which is the accepted way to log creation of tokens.

Update: Fixed in and .

Possible erroneous Transfer event in transferFrom

In it is not checked that the source account has enough balance. This is not severe because, in general, the safe subtraction will revert the transaction if more than the available balance wants to be transferred. However, in the special case that someone calls with the source and destination as the same account, the same won’t happen because the balance is first incremented, and only then decremented. The resulting balance will remain the same, but it would be possible for anyone to emit a event with themselves as destination and an arbitrary amount. This might be misinterpreted for a real transfer by an application or individual. Consider swapping the lines so that the balance is first decremented and then incremented, in order for such a transaction to fail.

Update: Fixed in .

Unchecked return value

The function is defined to return a boolean value indicating success. However, this return value is ignored when the function is called. Since the return value is currently always , consider removing it altogether. Otherwise, always check the return value.

Update: The return value was removed in .

Duplicate data

The token contract has a state variable which is hardcoded to be the same value as the ’s state variable. This duplication of the value is error-prone, as it could easily get out of sync. Consider getting the value directly from the crowdfund, as .

Semantics of totalAllocatedTokens

It is unclear what the state variable of means. At the moment the crowdfund is finalized, this variable will get out of sync with the amount of tokens actually distributed, because the platform will have all of the remaining supply to sell. Consider removing the variable altogether, or calling when the final remaining tokens are transferred to the platform in .

Update: The variable was removed in .

High coupling between token and crowdfund contracts

The token contract does a lot more than handle balances, transfers, and allowance. It has special code for crowdfund-originated “transfers”, for the Vanbex team vesting, and for the presale. It has variables and events to track crowdfund and presale state. As a result of all this, the code is very hard to follow and to ensure it is bug-free. At this stage, it would be a high risk to redesign the contracts, so we will not recommend that. However, the team should keep in mind to follow software engineering principles such as separation of concerns, single responsibility, low coupling, etcetera, for any new contracts they write.

Low Severity

Rejecting non-zero transfers is not ERC20 compliant

The ERC20 specification states that “transfers of 0 values MUST be treated as normal transfers”. In these are rejected. Consider removing the modifier from and .

Update: Fixed in .

Notes & Additional Information

  • Consider using OpenZeppelin’s implementation to build on top of.
  • Make sure the of is set to a secure address, preferably a multi-signature wallet. This address has the ability of changing the address where the funds are sent.
  • The “price” of a token usually means how much ETH is necessary to buy one unit of it. In the contract it is used to mean the amount that can be bought with 1 ETH. This concept is usually referred to as “rate”.
    (Update: Renamed to “rate” in .)
  • This documentation comment in is incomplete.
  • The fallback function in has the modifier added. This can be circumvented by a user by calling directly. Consider placing the modifier in instead. (The transaction would fail nonetheless because won’t accept an amount of zero tokens.) (Update: Fixed in .)
  • The function of doesn’t itself require that it be called after the crowdfund period ends. It calls in , however, which does fail and revert the whole transaction in that case. This is a strange assignment of responsibilities. It should be the crowdfund contract itself which rejects the transaction before the end of the period. (Update: Fixed in .)
  • The state variable of is merely the sum of and , which is a constant value. It is not used for anything. Consider removing it as well. (Update: removed in 4215e8c.)
  • It is undocumented and unclear what the field of the and events should mean. In the implementation the value used is the amount left over from the crowdfund and presale allocations respectively. Consider choosing a better name, such as , and documenting its meaning. (Update: Fixed in .)
  • The empty fallback function is not needed because Solidity will by default reject value transfers. (Update: Fixed in .)
  • Constant values can be declared as state variables and initialized in the declaration itself (as opposed to in the constructor). Consider doing this for , , , etcetera.
  • Consider writing token amounts parameterized by the state variable. For this, it can be helpful to define a variable . Afterwards, you can write, for example, . This is clearer and more maintainable.
  • The variable is not used anywhere. (Update: Removed in .)

Conclusion

One critical severity and two high severity issues were found and explained, along with recommendations on how to fix them. Some changes were proposed to follow best practices and reduce potential attack surface.

Note that as of the date of publishing, the above review reflects the current understanding of known security patterns as they relate to the Fuel Token contract. We have not reviewed the related Etherparty project. The above should not be construed as investment advice. For general information about smart contract security, check out our thoughts here.

Be part of the community

--

--

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store
Zeppelin

We build software to grow and protect the core infrastructure of an open, global economy, powered by blockchain technologies.