xToken Origination Terminal Audit
Smart Contract Security Assessment
Mar 4, 2022
![xToken](/audits/img/logos/xtoken.png)
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:
- 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.
- 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
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
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
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.
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).
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.
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).