IllumineX

Upon initiating interactions with the IllumineX protocol, a user is required to submit a message containing instructions for execution (e.g., swapping token A on chain A for token B on chain B through a specific token path) to Sapphire for encryption. Alongside this information, the user includes an integer nonce. This nonce is crucial for both uniquely identifying the order and for preserving user privacy, particularly when their unwrap request is executed from the buffered queue. To ensure these properties, the nonce should be random, with the emphasis on its uniqueness, an easily verifiable property. Currently, there is no verification in place to ensure the uniqueness of the nonce, and a poorly chosen nonce could compromise user privacy or even block the execution of their order, while their funds remain locked in the system contracts.

While the IllumineX team has indicated that the frontend will supply a random and secure nonce for every user interaction/order, we have not scrutinized the exact nonce generation procedure employed by the frontend. Consequently, we cannot assert its safety, nor can we be certain that even if the nonces produced by the frontend are sufficiently random and secure, they cannot be maliciously obtained. In our security analysis, we operate under the assumption that the nonces provided by the user, whether through the frontend or another source, are unique and challenging to guess.

The IllumineX protocol, leveraging the Sapphire network, prioritizes user privacy by deploying private copies of ERC-20 tokens. Specifically during swaps, it's crucial that all tokens in the user-specified swap path are private to prevent leakage of exchanged amounts, ensuring comprehensive user privacy.

While the contract verifies that the input and output tokens offer the privacy guarantees, intermediate tokens within the swap path lack the same verification. Although the protocol team acknowledges this in a note within the SapphireEndPoint::_handleProxyPass function, the contracts do not actually implement this verification to intermediate tokens.

The current expectation that users will carefully select private token paths or that the frontend restrictions will limit paths to private tokens might be optimistic. Therefore, we suggest adding a simple contract check that all tokens within the swap path are private.

The BalanceRegistry::getHeldTokens function takes as arguments two integers, the offset and the limit. It is intended to return the _result array of length limit - offset, which will hold the entries of the _register[msg.sender] array at positions [offset, limit-1]. However, in the for loop below, we observe that we can simplify the _result index from _result.length - (limit - offset - i) to i, and then notice that this index i runs from offset to limit. All indices starting from limit - offset (if this is larger than offset) to limit are out of bounds, leading to the execution reverting. The correct loop index for the _result is i - offset, not just i.

_result[_result.length - (limit - offset - i)] = TokenData(
// Dedaub: _result.length - (limit - offset - i) should be replaced by i - offset
    token.balanceOf(_msgSender()),
    address(token),
    token.name(),
    token.symbol(),
    token.decimals()
);

CondifentialRouter::addLiquidityROSE accepts msg.value ROSE from its caller to add as liquidity to the appropriate pool. However, the final amount (amountB) provided as liquidity might be less than msg.value, depending on the amount of the other token of the liquidity pair. If that is the case, the difference msg.value - amountB is not returned to the user but is left trapped inside the ConfidentialRouter contract. In the vast majority of cases we expect this difference to be small (even 0), as the liquidity amounts should have been computed off-chain prior to calling the smart contract. Still, the contract should ensure that any excess amount is returned to the user at the end.

A similar issue arises in the function PrivateWrapperFactory::wrap. The smart contract currently validates that the value sent with the transaction (msg.value) is greater than or equal to the specified wrapping amount. While the contract successfully wraps the designated amount, it fails to return any surplus ROSE to the user in cases where msg.value exceeds the specified wrapping amount. The same is true for PrivateWrapperFactory::unwrapInQueueBatch.

The function SapphireEndpoint::prepareEncryptedParams does not require that the length of the params.outputs array is greater than 0. However, this is required by the function _handleProxyPass of SapphireEndpoint that will inevitably be called when the encrypted message reaches Sapphire. If this happens the execution will revert and further progress will be impossible to achieve, while the user’s funds will be locked in the source chain. Even though the IllumineX front-end, which users are advised to always use, performs this check, the check should also be implemented in the SapphireEndpoint smart contract.

Illuminex allows users to perform cross-chain swaps, enabling the specification of multiple destination addresses on the destination chain for the distribution of the output amount. Users can also define the amount each of these addresses should receive. Prior to the token swap, the tokens are wrapped in a private equivalent. Users have the option to request immediate unwrapping of the private tokens or to place them in a buffer queue, where unwrapping occurs only when a sufficient number of requests accumulate, thereby enhancing privacy. However, if the user opts not to request a buffer unwrap, an issue arises: all recipients, except for the first one, are ignored during the processing, as indicated in the following code.

According to the protocol developers, the frontend, which is expected to be the primary gateway for interacting with the protocol, does not support multiple receivers when the user hasn't requested a buffer unwrap. However, we do not consider it safe to solely rely on the assumption that users will exclusively use the frontend and refrain from direct interaction with the contracts. In such cases, the issue may still arise.

if (bufferUnwrap) {
  // Dedaub: code omitted for brevity.
} else {
  (uint64 dstChainId, address dstAddress, uint256 amount,)
    =_decodeProxyPassCommand(entries[0]);
  _finalizeOutput(
    keccak256(abi.encodePacked(_nonce, dstChainId, dstAddress, uint(0))),
    _token,
    amount,
    dstChainId,
    dstAddress
  );
}

The functions proxyPass of the MultichainEndpoint and SapphireEndpoint contracts accept the native chain’s currency as well as ERC20 tokens as deposit tokens. At the same time, the user/caller is expected to pay for their transaction’s fees in the source chain’s native currency. Specifically, the fees will be (1) equal to the msg.value minus the deposited amount (msg.value - amount) of native currency or (2) equal to the msg.value if the deposit is made in an ERC20 token (and not the native token). However, the protocol incorrectly assumes that in both cases the amount provided by the user for the fees payment is equal to msg.value, essentially overestimating the amount available to be spent on fees in the case that native currency is deposited. Lower than expected fee amounts could lead to the transaction not getting broadcasted via the operators of the Celler Inter-chain Messaging protocol or not being picked up by the PrivateWrapperFactory off-chain executor.

In the MultichainEndPoint::executeMessageWithTransferFallback() function, the _preprocessPayloadData function is called to obtain the address (sender) to which the tokens should be sent. However, unlike in the corresponding function of the SapphireEndPoint contract, the MultichainEndPoint::_preprocessPayloadData always returns the zero address. Consequently, if this fallback is triggered, the tokens will be sent to the zero address.

This fallback scenario can only be triggered when the MultichainEndpoint is used instead of its extension, the SapphireEndpoint. In this setup, the endpoint contract is deployed on a destination chain that is not Sapphire. The fallback is activated only in a receive action, where the MultichainEndPoint::executeMessageWithTransfer() function executes the else if part, calling _handleReceive. While this function typically transfers tokens to the destination address, and is unlikely to fail, there exists a theoretical possibility of failure, leading to the aforementioned scenario. To resolve this issue, we recommend fixing the preprocessPayloadData function of the MultichainEndpoint to return the actual sender and address this problem.

The function IXToken::_mintPrivateSaleVestedTokens creates a vesting contract (stored in variable _vesting) for the staked IXToken, but instead of calling its computeNextVestingScheduleIdForHolder function calls the corresponding function of the vesting contract (stored in storage variable vesting) for the IXToken. Obviously, this would be problematic if the two calls did not return the same vesting schedule id, due to the two contracts accidentally having the same state.

The ERC2721Context::_msgSender() function, when called by the trusted forwarder, returns the actual msg.sender. This address is added by the trusted forwarder at the last 20 bytes of the calldata. To implement this functionality, the function uses EVM assembly, as shown in the code snippet below.

sender := shr(96, calldataload(sub(calldatasize(), 20)))

The issue arises from the lack of verification that the calldata has a size larger than 20 (i.e., the size of an address). When subtracting 20 from the calldatasize(), since assembly code is being used, it will not revert, but it may still overflow, returning a huge number. The subsequent calldataload will attempt to access this location, and the result will either be 0 or lead to running out of gas. We recommend adding a sanity check to ensure that calldatasize() is greater than or equal to 20.

The MerkleVestingSplitter is utilized for distributing vested tokens to users. The contract deployer predefines the addresses eligible for token distribution and the respective claimable amounts, encoding all this information in a Merkle tree. Each leaf of the tree consists of pairs (address, limit), indicating that the specified address is entitled to claim up to the specified limit of tokens from the contract. The deployer provides the root of this tree during construction.

Users can claim their tokens by providing a proof that they are part of the Merkle tree. The issue lies in the absence of information about the vesting schedule associated with this token distribution. While a user with a valid proof for a Merkle tree related to one vesting schedule cannot claim tokens from another vesting schedule due to access control restrictions, which permit only one MerkleVestingSplitter to interact with each vesting contract, we believe that it would be more robust if the vestingID were part of the leaf and thus factored into the computation of the root.

By incorporating the vestingID into the leaves, it becomes a contributing factor in the root calculation. Consequently, even for identical distribution programs (same addresses and limits) for two distinct vesting schedules, different roots and valid proofs are generated. This approach eliminates reliance on the vesting contract's access control, enhancing overall security and preventing potential cross-vesting schedule issues.

The CrossChainVaultApp contract is designed to receive messages from the Celer IM, providing instructions on its actions. These messages are generated by the corresponding copies of the CrossChainVaultApp contract on other chains. To prevent the CrossChainVaultApp from executing messages from arbitrary addresses on other chains, the owner of the contract has to specify one address for each chain that the CrossChainVaultApp is allowed to receive messages from. The relevant function is as follows.

function setAllowedSenders(SetAllowedSender[] calldata senders) public onlyOwner {
  for (uint i = 0; i < senders.length; i++) {
    // We can't add new senders for same chain id otherwise it would be dangerous
    require(
      !allowedSenderSetup[senders[i].srcChainId], "Sender is already setup"
    );

    emit SetAuthorisedSender(
      senders[i].sender, senders[i].srcChainId, senders[i].isAllowed
    );
    allowedSenders[senders[i].sender][senders[i].srcChainId] =
      senders[i].isAllowed;
     // Dedaub: the assigned value should be senders[i].isAllowed and not always true.
     allowedSenderSetup[senders[i].srcChainId] = true;
  }
}

The issue arises when the owner passes sender information to the function for a chainId not yet configured (i.e., allowedSenderSetup[chainID] == false), but the isAllowed variable is accidentally misconfigured to false. In such a scenario, the function sets allowedSenderSetup[chainId] to true, even though the sender is not allowed, and the require statement prevents the owner from calling the function again for the same chainId to configure it correctly.

Instead of always setting allowedSenderSetup[senders[i].srcChainId] to true, the correct action would be to set it to senders[i].isAllowed.

Illuminex contracts currently use the .transfer method for native token transactions. This method forwards a fixed amount of gas (2300) and that was considered to be a measure against reentrancy. However, this approach assumed constant gas costs, which is not the case, since gas costs are subject to change. Any contract using .transfer takes a hard dependency on gas costs and could break after a future gas costs update. Moreover, the use of .transfer hampers interactions with other protocols that require multiple actions or adjustments to accounting variables upon receiving tokens from IllumineX. Additionally, it poses a challenge for IllumineX's interaction with accounts using account abstraction.

Since Illuminex already employs the checks-effects-interactions pattern for security, we believe that it is safe to replace .transfer with .call. This switch would not only eliminate gas cost dependencies but also improve interactions with other protocols.

The protocol owners’ multisig can retrieve the otherwise private ring keys that are used to encrypt all user messages/orders, i.e., a malicious or compromised owner could front-run users or reveal their identity and actions. According to the IllumineX developers (compliance docs) this feature (fetchRevealedKeys) exists for compliance reasons and will only be used if they are required by law to provide information to government and law enforcement agencies. A more sophisticated approach would allow revealing a key only when all the messages encrypted with it have been executed. As such an approach might not be efficient to implement on-chain, certain compromises might have to be made, e.g., enforce that the last N keys that have been generated cannot be revealed and a new key is generated at least after ringKeyUpdateInterval seconds after the previous one.

After the audit suggestions a delay of 1 key was added, i.e., all keys can be revealed except for the currently active one at the moment a user interacts with the Sapphire endpoint to encode their action. Also, a separate access control role, the one of the compliance manager, was created, which is the only one who is able to reveal the ring keys.

Functions addLiquidity and addLiquidityRose of the ConfidentialRouter do not check if there exist private versions of the swapped tokens before calling swapRouter.precalculateAmounts with those as parameters, which would lead to failure if they do not exist.

(amountA, amountB) = swapRouter.precalculateAmounts(
  address(wrapperFactory.wrappers(tokenA)),
  address(wrapperFactory.wrappers(tokenB)),
  amountADesired,
  amountBDesired,
  amountAMin,
  amountBMin
);

It is understood that the protocol team will create private counterparts for all popular tokens exchanged through the ConfidentialRouter and that users should check that a private wrapper for their token exists prior to using the ConfidentialRouter. Nevertheless, the ConfidentialRouter should appropriately handle such user errors.

The caller of the MerkleSplitter,MerkleVestingSplitter::claim functions passes as arguments the amount they want to withdraw and the limit, i.e., the maximum amount they can claim in total. This limit is a part of the Merkle tree leaf and is verified by the Merkle proof; hence, the user has to submit the real one and cannot provide false information. Therefore, the mapping maxLimit, which holds the limit for each user, and limitSet, which records whether the limit for the user has already been set, are redundant.

• CrossChainVault:

  • The lock function could require that amount > 0.

  • The unlock function could require that allowedAssets[asset] == true and amount > 0.

  • SapphireEndpoint:

  • The _handleProxyPass could require that the command type is MultichainCommandType.ProxyPass

The following variables are only set in constructor, therefore could be declared immutable:

• ERC2772Context: _trustedForwarder
• PrivateERC20: name, symbol, decimals
• PrivateWrapperFactory: balanceRegistry, migratedFromWrapper
• CrossChainERC20: name, symbol, decimals, originalAddress, originalChainID

The function _handleProxyPass of the SapphireEndpoint contract approves the PrivateWrapperContract to spend _totalAmount of _token tokens in order to wrap them before checking that _token is not actually a private wrapped token.

IERC20(_token).approve(address(wrapperFactory), _totalAmount);
if (address(wrapperFactory.tokenByWrapper(_token)) == address(0)) {
  // Dedaub: the approve call should be moved inside the if block as
  // it is only needed if the wrapping happens.
  wrapperFactory.wrapERC20(_token, _totalAmount, address(this));
}

The DelegateMulticall contract is not used and should be removed from the codebase.

The failure parameter of the MultichainEndpoint::_handleReceive function is set to false in all its calls and thus could be turned into a constant.

When the user calls SapphireEndpoint.prepareEncryptedParams(), they receive a pair (encoded, keyIndex), where the first element is the encoded message containing the instructions they want to be executed, and the second element is the index of the key used for encryption. However, based on the subsequent line, it is expected that the user submits to proxyPass() the abi.encode of keyIndex followed by encoded, rather than the more intuitive order of encoded followed by keyIndex.

function _decrypt(bytes memory _keyData)
  private view returns (uint256 ringKeyIndex, bytes memory output)
{
  (uint256 _ringKeyIndex, bytes memory _encryptedData) =
    abi.decode(_keyData, (uint256, bytes));
  // Dedaub: Code omitted for brevity.
}

Private ERC20 tokens have an onTransfer() function/hook within the BalanceRegistry contract. This function is responsible for updating the list of private tokens held by both the sender and receiver following a private token transfer, minting, or burning, based on transaction details. If the sender's balance falls below a dustThreshold value set by the contract owner, the token is removed from the sender's list.

While the dustThreshold is initialized to 0, the contract owner can assign any value to it by calling the setDustThresholdValue() function. If the owner sets a high value for this threshold, even users with substantial token amounts may observe that the token is not listed when calling the BalanceRegistry::getHeldTokens() function. Although users can still view their token balance directly by calling token.balanceOf(hisAddress) and execute transfers, we believe this could potentially lead to user confusion regarding token holdings. It's worth noting that the setDustThreshold() function introduces flexibility, acknowledging the challenge of defining a uniform threshold for tokens with varying prices.

While no specific security concerns have been identified for direct interactions with ChainVaultApp, we propose restricting access to the CrossChainVault::lock function solely to the CrossChainVaultApp contract, to align with the intended usage.

The feesCollector variable within the FeesCollector contract is never set or used and should be removed.

The floating version pragma solidity ^0.8.0 allows contracts to be compiled with any version of the Solidity compiler ranging from 0.8.0 to 0.9.0. Even though versions might not differ drastically, floating pragmas should be avoided and the pragma should be fixed to the version that will be used for the contracts’ deployment.

The code can be compiled with Solidity versions ^0.8.0. According to the foundry.toml file of the codebase, version 0.8.17 is currently used which has some known bugs, which we do not believe affect the correctness of the contracts.