xToken Origination Terminal Audit

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.

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.

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.

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).

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.

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.

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.

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.

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.

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.

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.

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).

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.