Skip to main content
Illuminex - Jan 19, 2024

IllumineX

Smart Contract Security Assessment

January 19, 2024

Oasis

SUMMARY

ID
Description
STATUS
P1
Lack of uniqueness verification for user-provided nonces
info
P2
Missing verification that all the tokens in a swapPath are private
info
M1
In certain cases BalanceRegistry::getHeldTokens will revert with an index out of bounds exception
fixed
M2
CondifentialRouter::addLiquidityROSE and PrivateWrapperFactory::wrap,unwrapInQueueBatch will not return excess ROSE to the caller
fixed
M3
SapphireEndpoint::prepareEncryptedParams should require that outputs.length > 0
fixed
M4
In cross-chain swap requests with multiple receivers and no buffer queue usage, only the first request is processed
fixed
M5
Function proxyPass may overestimate the native currency amount that can be spent to cover different execution fees
fixed
M6
Tokens are sent to the zero address instead of the sender when callback is triggered on receive
acknowledged
L1
IXToken::_mintPrivateSaleVestedTokens calls the wrong vesting contract
fixed
L2
Missing calldata size validation
acknowledged
L3
The vestingID should be part of the leaves in the MerkleVestingSplitter
acknowledged
L4
CrossChainVaultApp::setAllowedSenders misses some edge case configurations
acknowledged
L5
Consider using .call instead of .transfer
acknowledged
N1
The owner can reveal the ring keys used to encrypt user messages/orders
partially fixed
A1
ConfidentialRouter::addLiquidity* functions assume that the tokens have a wrapped counterpart
info
A2
In MerkleSplitter and MerkleVestingSplitter the mappings limitSet and maxLimit could be removed
info
A3
Several contracts could implement stricter input sanitization checks
info
A4
Several variables could be immutable
info
A5
SapphireEndpoint might needlessly approve the PrivateWrapperFactory to use its tokens
info
A6
The DelegateMulticall contract is not used
info
A7
MultichainEndpoint::_handleReceive function’s failure parameter is always set to false
info
A8
Asymmetric encrypt and decrypt are error-prone
info
A9
A possible high value of dustThreshold can cause confusion
fixed
A10
Restrict access to the CrossChainVault::lock function solely to CrossChainVaultApp
info
A11
Unused variable in FeesCollector contract
fixed
A12
Floating version pragma in contracts
info
A13
Compiler version and possible bugs
info

ABSTRACT

Dedaub was commissioned to perform a security audit of the IllumineX protocol, which is going to be deployed on a number of chains including Oasis Sapphire.

IllumineX is an exchange protocol that leverages the product rule of Uniswap V2. What sets it apart is its capability to facilitate both cross-chain exchanges and private transactions. To achieve these distinctive features, IllumineX integrates the Celer Inter-chain Messaging protocol for cross-chain compatibility and the Sapphire ParaTime of the Oasis Network to ensure privacy.

A user of the IllumineX protocol can initiate the exchange of a token A from a source chain for a token B on a (potentially different) target chain. The user gathers all relevant data for this transaction (including token A, token B, destination chain, amount in, minimum amount out, and receiver address) and interacts with a contract deployed on Sapphire to encrypt this information. Subsequently, the user sends the encrypted message, along with the specified amount of token A, to a contract on the source chain. This contract locks the tokens and emits a message for Celer, detailing the precise actions to be executed. Celer then forwards this message to Sapphire. Simultaneously, the tokens are bridged, creating cross-chain copies corresponding to the locked ones.

Sapphire operates as a confidential EVM chain, where all transactions are private, and even full nodes cannot decrypt the data of a transaction (public storage variables are excluded). Within Sapphire, IllumineX allows users to wrap their standard (cross-chain) ERC20 tokens into private tokens, ensuring that even the balance of each address remains hidden from any non-approved party. These private tokens can be exchanged in a Uniswap V2 clone contract deployed in Sapphire. The final output can be either immediately unwrapped and sent to the destination address through Celer IM or placed in a buffered queue. In the latter case, the unwrapping is triggered by an approved executor when a sufficient number of requests accumulate, enhancing user privacy by reducing the time correlation between the user's initial interaction with the IllumineX protocol and its finalization.


SETTING & CAVEATS

The audit report covers commit hash d6ae0ef7e5ad0ad6e5eb554e547db559aa17bbde of the at the time private repository. Audited suggested fixes were also reviewed up to commit hash 81bf7b398648d43ebd2968314e11688f65ac6e54. Two auditors worked on the codebase for 14 days.

The codebase is not accompanied by a test suite. According to the protocol developers, the protocol has been through several phases of end-to-end testing. There have also been public testnet releases of considerable duration.

The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than the regular use of the protocol. Functional correctness (i.e. issues in "regular use") is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e. full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.

The full list of audited files is:

contracts
  • confidentialERC20
    • BalanceRegistry.sol
    • ERC2771Context.sol
    • LuminexPrivacyPolicy.sol
    • PrivateERC20.sol
    • PrivateWrapperFactory.sol
    • PrivateWrapper.sol
  • econ
    • IMintableERC20.sol
    • IXToken.sol
    • LuminexFarming.sol
    • MerkleSplitter.sol
    • MerkleVestingSplitter.sol
    • StakedIXToken.sol
    • Vesting.sol
  • illuminex
    • chainvault
      • CrossChainERC20.sol
      • CrossChainVaultApp.sol
      • CrossChainVault.sol
      • ICrossChainVault.sol
    • op
      • ConfidentialRouter.sol
      • IMultichainEndpoint.sol
      • MultichainEndpoint.sol
      • SapphireEndpoint.sol
  • libraries
    • Bitmask.sol
    • DelegateMulticall.sol
    • FeesCollector.sol
    • LuminexLibrary.sol
    • Multicall3.sol
    • UQ112x112.sol
  • swap
    • LuminexV1Factory.sol
    • LuminexV1PairIX.sol
    • LuminexV1Pair.sol
    • LuminexV1Router.sol

PROTOCOL-LEVEL CONSIDERATIONS

P1

Lack of uniqueness verification for user-provided nonces

PROTOCOL-LEVEL-CONSIDERATION
info
P1
Lack of uniqueness verification for user-provided nonces

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.

P2

Missing verification that all the tokens in a swapPath are private

PROTOCOL-LEVEL-CONSIDERATION
info
P2
Missing verification that all the tokens in a swapPath are private

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.



VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues affecting 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” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

[No critical severity issues]


HIGH SEVERITY

[No high severity issues]


MEDIUM SEVERITY

M1

In certain cases BalanceRegistry::getHeldTokens will revert with an index out of bounds exception

MEDIUM
fixed
M1
In certain cases BalanceRegistry::getHeldTokens will revert with an index out of bounds exception

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.

BalanceRegistry::getHeldTokens():88-94
_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()
);

M2

CondifentialRouter::addLiquidityROSE and PrivateWrapperFactory::wrap,unwrapInQueueBatch will not return excess ROSE to the caller

MEDIUM
fixed
M2
CondifentialRouter::addLiquidityROSE and PrivateWrapperFactory::wrap,unwrapInQueueBatch will not return excess ROSE to the caller

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.

M3

SapphireEndpoint::prepareEncryptedParams should require that outputs.length > 0

MEDIUM
fixed
M3
SapphireEndpoint::prepareEncryptedParams should require that outputs.length > 0

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.

M4

In cross-chain swap requests with multiple receivers and no buffer queue usage, only the first request is processed

MEDIUM
fixed
M4
In cross-chain swap requests with multiple receivers and no buffer queue usage, only the first request is processed

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.

SapphireEndpoint::_handleProxyPass()
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
);
}

M5

Function proxyPass may overestimate the native currency amount that can be spent to cover different execution fees

MEDIUM
fixed
M5
Function proxyPass may overestimate the native currency amount that can be spent to cover different execution fees

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.

M6

Tokens are sent to the zero address instead of the sender when callback is triggered on receive

MEDIUM
acknowledged
M6
Tokens are sent to the zero address instead of the sender when callback is triggered on receive

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.



LOW SEVERITY

L1

IXToken::_mintPrivateSaleVestedTokens calls the wrong vesting contract

LOW
fixed
L1
IXToken::_mintPrivateSaleVestedTokens calls the wrong vesting contract

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.

L2

Missing calldata size validation

LOW
acknowledged
L2
Missing calldata size validation

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.

ERC27721Context::_msgSender():24
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.

L3

The vestingID should be part of the leaves in the MerkleVestingSplitter

LOW
acknowledged
L3
The vestingID should be part of the leaves in the MerkleVestingSplitter

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.

L4

CrossChainVaultApp::setAllowedSenders misses some edge case configurations

LOW
acknowledged
L4
CrossChainVaultApp::setAllowedSenders misses some edge case configurations

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.

CrossChainVaultApp::setAllowedSenders()
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.

L5

Consider using .call instead of .transfer

LOW
acknowledged
L5
Consider using .call instead of .transfer

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.



CENTRALIZATION ISSUES

It is often desirable for DeFi protocols to assume no trust in a central authority, including the protocol’s owner. We list issues that could arise if the protocol owner abuses their powers below. (These issues should be considered in the context of usage/deployment, as they are not uncommon. Several high-profile, high-value protocols have significant centralization threats.)

N1

The owner can reveal the ring keys used to encrypt user messages/orders

CENTRALIZATION
partially fixed
N1
The owner can reveal the ring keys used to encrypt user messages/orders

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.



OTHER / ADVISORY ISSUES

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

A1

ConfidentialRouter::addLiquidity* functions assume that the tokens have a wrapped counterpart

ADVISORY
info
A1
ConfidentialRouter::addLiquidity* functions assume that the tokens have a wrapped counterpart

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.

ConfidentialRouter::addLiquidity():94-101
(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.

A2

In MerkleSplitter and MerkleVestingSplitter the mappings limitSet and maxLimit could be removed

ADVISORY
info
A2
In MerkleSplitter and MerkleVestingSplitter the mappings limitSet and maxLimit could be removed

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.

A3

Several contracts could implement stricter input sanitization checks

ADVISORY
info
A3
Several contracts could implement stricter input sanitization checks
  • 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

A4

Several variables could be immutable

ADVISORY
info
A4
Several variables could be immutable

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

A5

SapphireEndpoint might needlessly approve the PrivateWrapperFactory to use its tokens

ADVISORY
info
A5
SapphireEndpoint might needlessly approve the PrivateWrapperFactory to use its tokens

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.

SapphireEndpoint::_handleProxyPass():327-330
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));
}

A6

The DelegateMulticall contract is not used

ADVISORY
info
A6
The DelegateMulticall contract is not used

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

A7

MultichainEndpoint::_handleReceive function’s failure parameter is always set to false

ADVISORY
info
A7
MultichainEndpoint::_handleReceive function’s failure parameter is always set to false

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

A8

Asymmetric encrypt and decrypt are error-prone

ADVISORY
info
A8
Asymmetric encrypt and decrypt are error-prone

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.

SapphireEndpoint::_decrypt()
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.
}

A9

A possible high value of dustThreshold can cause confusion

ADVISORY
fixed
A9
A possible high value of dustThreshold can cause confusion

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.

A10

Restrict access to the CrossChainVault::lock function solely to CrossChainVaultApp

ADVISORY
info
A10
Restrict access to the CrossChainVault::lock function solely to CrossChainVaultApp

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.

A11

Unused variable in FeesCollector contract

ADVISORY
fixed
A11
Unused variable in FeesCollector contract

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

A12

Floating version pragma in contracts

ADVISORY
info
A12
Floating version pragma in contracts

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.

A13

Compiler version and possible bugs

ADVISORY
info
A13
Compiler version and possible bugs

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.



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.