Skip to main content
Dedaub

xToken Origination Terminal Audit

Smart Contract Security Assessment

Mar 4, 2022

xToken

SUMMARY


ABSTRACT

Dedaub was commissioned to perform a security audit on xToken’s Origination Terminal contracts, at commit hash 62978c91e32178d269a1fc4927b1e07b624cf3c5. The code can be found here.


SETTING & CAVEATS

xToken’s Origination Terminal aims to provide a straightforward token launchpad service that is focused on ease-of-use. At a high level, it is a system that allows users to sell their own tokens by creating listings. These listings are highly parameterizable - more specifically, users can specify (among others):

  • The duration of the sale
  • The starting and ending price of the token (price linearly interpolated over time)
  • The vesting period
  • The minimum amount of tokens that must be sold for the sale to be completed successfully
  • The purchase token and the token being offered
  • And more…

The creators of a listing are also able to only allow a select set of whitelisted addresses to buy the listed tokens, via a Merkle tree based mechanism.

As an exchange for this service, xToken receives a flat fee in native currency for the listing creation, plus a percentage fee on the total amount of purchase tokens received during the sale.

Finally, the listing contracts are upgradeable (using the TransparentProxy pattern) and their owners may optionally upgrade them to a newer version issued by the project developers.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues that affect the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

Issue resolution includes “dismissed”, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

[No critical severity issues]


HIGH SEVERITY

H1

The token owner can call initiateSale multiple times

H1HIGH

The token owner can call initiateSale multiple times
resolved

The OriginationPool::initiateSale is the function that is called by the sale owner in order to kick start the sale. The actions being performed during its execution can be summarized at a high level as follows:

    • Takes custody of the tokens that are being offered for sale.

    • Sets the saleInitiated flag to true

    • Sets the sale initiation timestamp to the current block.timestamp. This piece of information is really important, as it determines the sale end timestamp and also plays an important role in calculating the current offering price.

      function initiateSale() external onlyOwnerOrManager {
      // Dedaub: No check to see if saleInitiated has been already set
      offerToken.transferFrom(msg.sender, address(this), totalOfferingAmount);
      saleInitiated = true;
      saleInitiatedTimestamp = block.timestamp;

      emit InitiateSale(saleInitiatedTimestamp);
      }

    Such a function should typically only be callable once in the lifecycle of a token sale, but the current state of the code has no safeguards that prevent a malicious seller from calling it multiple times. Doing so could have multiple adverse effects, including extending the sale, resetting the price curve, cause users to be unable to claim their vesting entries etc. It should be noted that this list of issues is not exhaustive, just a small collection of program behavior that can arise from such manipulations.

    Note that each call of initiateSale requires the sale owner to re-deposit a totalOfferingAmount of tokens. This is certainly a roadblock for a malicious seller, but it might not be sufficient to deter this kind of attack (after all, the seller owns the token, preventing the sale might be worth the cost of locking some tokens in the pool). It is highly recommended that a check is put into place in initiateSale, which checks that the saleInitiated flag has not been set already.



MEDIUM SEVERITY

M1

The token owner can abuse the whitelist to prevent the sale from being completed

M1MEDIUM

The token owner can abuse the whitelist to prevent the sale from being completed
resolved 1

Although the sale parameters cannot be modified after the sale starts, the whitelist can be modified at any time. The owner can even enable the whitelist for a sale having no initial whitelist. This can be abused by an owner who, for any reason, does not want the sale to be completed (even if there is enough interest to reach the reserveAmount). The owner can simply set an empty whitelist, anytime before the reserveAmount is reached, and essentially freeze the sale. The buyers have no guarantee that the sale will be completed, even if a whitelist is initially not set.

For sales without a whitelist, a solution is to forbid enabling the whitelist at a later time (guaranteeing that the sale can be completed).

For sales with a whitelist, the need to modify it is understandable. A possible solution could be to only allow extending the whitelist, by adding more addresses, but not removing any. This can be achieved by extending the merkle proof tree, and providing a proof that the old whitelist is included in the new one! The proof can be verified in setWhitelist.



LOW SEVERITY

L1

Incorrect storage field initialization

L1LOW

Incorrect storage field initialization
resolved 2

In OriginationPool.sol, a storage field is being initialized as follows:

contract OriginationPool {
// Dedaub: This initial value will not be "visible" in the Proxy
uint256 public vestingID = 1;
// [...]
}

While this initialization pattern is fine when the contract is going to be used directly, it’s not really the case when the contract is going to be used via a proxy (like in this case). This is because the state of each proxy will have 0 as the initial value of vestingID, as it is not being initialized in the initializer function.

While this doesn’t seem to be exploitable, the current state of the code does not follow the intended design implied by the source code i.e. vestingID having an initial value of 1.

It is recommended that either vestingID be initialized in the initializer function, or the intended initial value of vestingID be changed to 0, if deemed desirable by the protocol developers.

L2

Incorrect vestingID in CreateVestingEntry event

L2LOW

Incorrect vestingID in CreateVestingEntry event
resolved

In OriginatorPool::_createVestingEntry, the CreateVestingEntry event is emitted with vestingID being one higher than the one used in the vesting entry.

userVestingEntries[_sender].push(vestingID++);
amountVested += _offerTokenAmount;

emit CreateVestingEntry(_sender, vestingID, saleInitiatedTimestamp + saleDuration, _offerTokenAmount);

It should be emitted before incrementing vestingID (it is also recommended to increment vestingID in a separate line, to make the code more readable and avoid such issues).

L3

ERC20 transfer without the use of SafeERC20

L3LOW

ERC20 transfer without the use of SafeERC20
resolved

In OriginationPool::_purchase and OriginationPool::initiateSale, token transfer are being performed without the use of the SafeERC20 library:

function _purchase(uint256 _contributionAmount) internal {
require(saleInitiated, "Sale not open");
require(block.timestamp <= saleInitiatedTimestamp + saleDuration, "Sale ended");
require(_contributionAmount > 0, "Must contribute");

address sender = msg.sender;
if (address(purchaseToken) == address(0)) {
// eth = purchase token
require(msg.value == _contributionAmount);
} else {
// Dedaub: Transfer done without the use of SafeERC20
purchaseToken.transferFrom(sender, address(this), _contributionAmount);
}
// [...]
}

function initiateSale() external onlyOwnerOrManager {
// Dedaub: Transfer done without the use of SafeERC20
offerToken.transferFrom(msg.sender, address(this), totalOfferingAmount);
saleInitiated = true;
saleInitiatedTimestamp = block.timestamp;

emit InitiateSale(saleInitiatedTimestamp);
}

While most tokens should be fully compliant with the ERC20 specification, there are instances of tokens that deviate from the ERC20 standard by not returning a boolean success value using returndata (USDT is the most popular such token), which would make this part of the code unable to be executed.

While the rest of the code makes correct use of the SafeERC20 library to support such tokens, this is the only instance where a plain transferFrom is being performed.

It is highly recommended that a safeTransferFrom be performed instead of a plain transfer, in order to fully support purchase tokens that don’t fully comply with the ERC20 standard.

It should be noted that a similar issue could apply to OriginationCore::claimFees. However, the fee token is considered to be trusted and vetted by the developers, and thus is less of a concern.



OTHER/ ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend addressing them.

A1

Inconsistent sale end checks

A1ADVISORY

Inconsistent sale end checks
resolved

In OriginationPool.sol, functions claimVested, claimTokens and claimPurchaseToken are executable only after the sale has ended. This is enforced by a require statement at the beginning of each functions’ body:

function claimVested(...) {
// Dedaub: Check here is exclusive
require(block.timestamp > saleInitiatedTimestamp +saleDuration, "Sale has not ended");
// [...]
}

function claimTokens() external nonReentrant {
// Dedaub: Check here is inclusive
require(block.timestamp >= saleInitiatedTimestamp + saleDuration, "Sale has not ended");
// [...]
}

function claimPurchaseToken() external onlyOwnerOrManager {
// Dedaub: Check here is exclusive
require(block.timestamp > saleInitiatedTimestamp + saleDuration, "Sale has not ended");

While both checks essentially achieve the same functionality, they differ in that one is inclusive (claimTokens) and the other two are exclusive (claimVested, claimPurchaseToken). It recommended that they are all made exclusive, to improve code consistency and not have any intersection on the decision boundary (“sale ongoing” checks are all inclusive).

A2

Gas optimization: Subexpression can be precomputed and stored.

A2ADVISORY

Gas optimization: Subexpression can be precomputed and stored.
resolved

In OrigniationPool.sol, there are multiple instances of conditions, which check whether the sale has ended or not. In all these checks, the following subexpression is being calculated: saleInitiatedTimestamp + saleDuration.

Due to the frequency of this calculation, evaluating it once during sale initiation and storing it in a relevant storage field e.g. saleEndTimestamp could help save some gas.

A3

Gas optimization: VestingEntry.initTimestamp is duplicated

A3ADVISORY

Gas optimization: VestingEntry.initTimestamp is duplicated
resolved

In OriginationPool.sol, VestingEntry.initTimestamp is always being set to saleInitiatedTimestamp + saleDuration. Removing it from the VestingEntry struct and storing it in a singleton field could help save some gas.

A4

Gas optimization: Require can be factored out of loop

A4ADVISORY

Gas optimization: Require can be factored out of loop
resolved

In OriginationPool::claimVested, there is a require that is performed on each iteration:

function claimVested(uint256[] calldata _vestingEntries) external nonReentrant {
require(block.timestamp > saleInitiatedTimestamp + saleDuration, "Sale has not ended");

if (purchaseTokensAcquired >= reserveAmount) {
for (uint256 i = 0; i < _vestingEntries.length; i++) {
VestingEntry memory entry = vestingEntries[_vestingEntries[i]];
require(entry.user == msg.sender, "User not owner of vest id");
// Dedaub: This check is done on each loop
require(entry.initTimestamp + cliffPeriod < block.timestamp, "Not past cliff period");

uint256 offerTokenPayout = calculateClaimableVestedOfferToken(entry);
entry.offerTokenAmountClaimed += offerTokenPayout;

offerToken.safeTransfer(msg.sender, offerTokenPayout);
amountVested -= offerTokenPayout;

vestingEntries[_vestingEntries[i]] = entry;

emit ClaimVested(msg.sender, offerTokenPayout);
}
}
}

While it seems that this condition is dependent on entry, taking into account the observation in A3, this require is invariant on entry and the code can be refactored to check this condition once at the beginning of the function.

A5

Missing sanity check on origination fee

A5ADVISORY

Missing sanity check on origination fee
resolved

In OriginationCore.sol, there are no sanity checks performed when setting the origination fee:

function initialize(
uint256 _listingFee,
uint256 _originationFee,
address _originationPoolImplementation,
IxTokenManager _xTokenManager
) external initializer {
__Ownable_init();
__ReentrancyGuard_init();

listingFee = _listingFee;
// Dedaub: No sanity checks on the provided parameter
originationFee = _originationFee;
// [...]
}

Since this is a percentage field, it is good practice to check that the value is in the range [0, 1e18], to protect against human error.

A6

Rebase tokens are not compatible with the project

A6ADVISORY

Rebase tokens are not compatible with the project
dismissed 3

In OriginationPool, the per-sale fees are accumulated in the state variable originationCoreFees. This accumulated value is then subtracted from the total purchase token amount gathered by the pool, and the remaining tokens are sent to the pool owner/admin as profits from the sale. The rest of the tokens (originationCoreFees amount) are sent to Core as commission:

claimAmount = purchaseToken.balanceOf(address(this)) - originationCoreFees;
purchaseToken.safeTransfer(owner(), claimAmount);
purchaseToken.safeTransfer(originationCore, originationCoreFees);

While this logic works just fine for “normal” tokens, in the case of rebase tokens this operation might be inaccurate or even not work at all. More specifically, rebase tokens have balances that fluctuate passively - the balance of an account can change from one block to another, without any tokens being transferred from/to it. This dynamic nature of rebase token balances means that the calculation of claimAmount could be inaccurate if a rebase has caused the balance to increase, or worse, revert due to an underflow in case a rebase caused the balance to be less than the accumulated fee value.

While this is a rare scenario, users of the protocol should be advised about this potential issue, and use the “wrapped” versions of rebase tokens which have stable balances and are designed for such use cases.

A7

receive functions can be improved

A7ADVISORY

receive functions can be improved
resolved

Both OrinationPool and OrinationCore contain receive functions, with the following check (which prevents some forms of accidental deposits, but clearly not all).

    	require(msg.sender != tx.origin, "Errant ETH deposit");

First, OrinationPool::receive seems to be redundant, all ether is received via other functions. It should be removed, to make the code clearer and prevent accidental deposits more effectively.

Second OriginationCore::receive seems to be needed to receive fees from the pool. A better approach would be to remove it, and replace it by some specific method, eg OriginationCore::receiveFees. Again, this would make the code clearer, and prevent accidental deposits more effectively.

A8

Memory copy can be avoided

A8ADVISORY

Memory copy can be avoided
resolved

In OrinationPool::claimVested, the memory copy of entry can be avoided. This would save some gas, but also make the code consistent with _createVestingEntry, in which a similar entry is used without a copy.

A9

Floating pragma

A9ADVISORY

Floating pragma
resolved

The floating pragma “pragma solidity ^0.8.0;” is used in the contracts, allowing them to be compiled with any of the 0.8.0 - 0.8.12 versions of the Solidity compiler. Although the differences between these versions are small, floating pragmas should be avoided and the pragma should be fixed to the version that will be used in the contract deployment (Solidity version 0.8.4).

A10

Compiler known issues

A10ADVISORY

Compiler known issues
info

The contracts were compiled with the Solidity compiler v0.8.4 which, at the time of writing, has one known bug. Upon inspection, we concluded that the subject code is unaffected.



DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.


ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.


Dedaub

Footnotes

  1. Whitelist can only be changed before the sale has been initiated

  2. Initial value changed to 0

  3. The developers will make it clear to the users that the protocol is not compatible with rebase tokens