The DEFAULT_ADMIN_ROLE role is the only one allowed to add and remove operators for the system. The operator role-plays an integral part in managing ERC20 assets in CeFiSmartTransfer and DeFiSmartTransfer. The executeErc20Transfer function on both contracts allows operators to transfer ERC20 tokens from the contract to any address. This function is restricted to operators only, ensuring that only authorized addresses with this specified role entities can execute ERC20 transfers.

However, this design introduces a substantial risk regarding the safety and integrity of ERC20 tokens held by the CeFiSmartTransfer contract and approved to the DeFiSmartTransfer contract. Since operators have the exclusive authority to initiate transfers, the system's security heavily relies on the trustworthiness and security of these operator accounts. If an operator's account is compromised or if an operator acts maliciously, they could potentially redirect ERC20 tokens to unauthorized addresses, leading to a loss of assets.

Additionally, executeErc20Transfer functions on both contracts do not implement checks on the destination addresses or limits on the amount of tokens that can be transferred, providing operators with unrestricted access to move tokens.

Several remedial strategies can be employed, including but not limited to:

Implement multi-signature control: Requiring multiple operators to approve a transfer before it can be executed adds a layer of security by distributing trust. This approach makes it much harder for a single compromised or malicious actor to execute unauthorized transfers.

Whitelist/blacklist mechanisms: Implement mechanisms to restrict token transfers to known, safe addresses, or to prevent transfers to addresses identified as risky.

Transfer limits: Introduce daily or transactional limits on the amount of ERC20 tokens that can be transferred. These limits could be configurable and subject to multi-signature approval for adjustments.

Introduce a time-lock mechanism: Implement a time-lock feature that delays the execution of token transfer requests by a certain period (e.g., 24-48 hours). This delay may allow operators and administrators of the contracts to review and potentially handle suspicious transfer requests before they are executed.

Remediation Plan

NOT APPLICABLE: According to the Mesh Connect team, this finding was addressed in their off-chain logic:

As recommended in the audit, we have implemented a time-lock mechanism to mitigate the risks associated with immediate token transfer executions by operators too. This time-lock feature in our off-chain logic, delays the execution of token transfer requests by a certain period. [...]. This changes will apply in off-chain logic.

The prepareNativeTransfer() function from the CeFiSmartTransfer contract allows for operators to set the NativeTransfer struct, which contains all the crucial information to the native asset transfer that will be executed when the contract receives assets via the receive() method. Nevertheless, this function lacks input validation for the input parameters.

Similarly, the executeNativeTransfer() function from the DeFiSmartTransfer contract allows any address to transfer native tokens to a receiver and a feeReceiver but lacks input validation for these parameters.

Such oversight on both functions can result in a potential loss of funds if either the receiver or feeReceiver are inadvertently set to address(0).

function testReceiveNativeTransferWhenFeeReceiverIsZeroAddress(
    address ethSender,
    string memory _id,
    uint256 _amount,
    uint256 _fee,
    address payable _receiver
  ) public {
    /* ------------------------------- preparation ------------------------------ */
    _notFoundryReservedAddress(_receiver);
    vm.assume(_receiver != address(0));
    vm.assume(_receiver != address(ceFiSmartTransfer));
    vm.assume(_amount != 0);
    _fee = bound(_fee, 0, _amount);

    /* -------------------------------- execution ------------------------------- */
    testPrepareNativeTransfer(_id, _amount, _fee, payable(address(0)), _receiver);
    hoax(ethSender, _amount);
    (bool ok, ) = (address(ceFiSmartTransfer)).call{value: _amount}("");
    assert(ok);
    assertEq((address(0)).balance, _fee);
}

function testReceiveNativeTransferWhenReceiverIsZeroAddress(
    address ethSender,
    string memory _id,
    uint256 _amount,
    uint256 _fee,
    address payable _feeReceiver
  ) public {
    /* ------------------------------- preparation ------------------------------ */
    _notFoundryReservedAddress(_feeReceiver);
    vm.assume(_feeReceiver != address(0));
    vm.assume(_feeReceiver != address(ceFiSmartTransfer));
    vm.assume(_amount != 0);
    _fee = bound(_fee, 0, _amount);

    /* -------------------------------- execution ------------------------------- */
    testPrepareNativeTransfer(_id, _amount, _fee, _feeReceiver, payable(address(0)));
    hoax(ethSender, _amount);
    (bool ok, ) = (address(ceFiSmartTransfer)).call{value: _amount}("");
    assert(ok);
    assertEq((address(0)).balance, _amount - _fee);
}

  function testExecuteNativeTransferToReceiverZeroAddress(
    address ethSender,
    string memory _id,
    uint256 _amount,
    uint256 _fee,
    address payable _feeReceiver
  ) public {
    /* ------------------------------- preparation ------------------------------ */
    vm.assume(_feeReceiver != address(deFiSmartTransfer));
    vm.assume(_feeReceiver != address(ethSender));
    vm.assume(_feeReceiver != address(0));
    vm.assume(ethSender != address(0));
    vm.assume(_amount != 0);
    _fee = bound(_fee, 0, _amount);
    _notFoundryReservedAddress(_feeReceiver);
    uint256 balanceReceiverBefore = address(0).balance;
    uint256 balanceFeeReceiverBefore = address(_feeReceiver).balance;
    uint256 amountForReceiver = _amount - _fee;

    deFi.Transfer memory _transfer = deFi.Transfer({
      id: _id,
      amount: _amount,
      fee: _fee,
      feeReceiver: _feeReceiver,
      receiver: payable(address(0)),
      payer: ethSender,
      token: address(0)
    });

    /* -------------------------------- execution ------------------------------- */
    hoax(ethSender, _amount);
    assertEq(ethSender.balance, _amount);
    deFiSmartTransfer.executeNativeTransfer{value: _amount}(_transfer);
    assertEq(ethSender.balance, 0);

    /* ------------------------------ verification ------------------------------ */
    uint256 balanceReceiverAfter = address(0).balance;
    uint256 balanceFeeReceiverAfter = address(_feeReceiver).balance;

    assertEq(balanceReceiverAfter, balanceReceiverBefore + amountForReceiver);
    assertEq(balanceFeeReceiverAfter, balanceFeeReceiverBefore + _fee);
  }

  function testExecuteNativeTransferToFeeReceiverZeroAddress(
    address ethSender,
    string memory _id,
    uint256 _amount,
    uint256 _fee,
    address payable _receiver
  ) public {
    /* ------------------------------- preparation ------------------------------ */
    vm.assume(_receiver != address(deFiSmartTransfer));
    vm.assume(_receiver != address(ethSender));
    vm.assume(_receiver != address(0));
    vm.assume(ethSender != address(0));
    vm.assume(_amount != 0);
    _fee = bound(_fee, 0, _amount);
    _notFoundryReservedAddress(_receiver);
    uint256 balanceReceiverBefore = address(_receiver).balance;
    uint256 balanceFeeReceiverBefore = address(0).balance;
    uint256 amountForReceiver = _amount - _fee;

    deFi.Transfer memory _transfer = deFi.Transfer({
      id: _id,
      amount: _amount,
      fee: _fee,
      feeReceiver: payable(address(0)),
      receiver: _receiver,
      payer: ethSender,
      token: address(0)
    });

    /* -------------------------------- execution ------------------------------- */
    hoax(ethSender, _amount);
    assertEq(ethSender.balance, _amount);
    deFiSmartTransfer.executeNativeTransfer{value: _amount}(_transfer);
    assertEq(ethSender.balance, 0);

    /* ------------------------------ verification ------------------------------ */
    uint256 balanceReceiverAfter = address(_receiver).balance;
    uint256 balanceFeeReceiverAfter = address(0).balance;

    assertEq(balanceReceiverAfter, balanceReceiverBefore + amountForReceiver);
    assertEq(balanceFeeReceiverAfter, balanceFeeReceiverBefore + _fee);
  }

Add a check to ensure that the receiver and the feeReceiver are not address(0).

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commits 76c823f and cea7f6f by following the mentioned recommendation and adding a check to verify that the receiver and the feeReceiver are not address(0).

Both CeFiSmartTransfer and DeFiSmartTransfer smart contracts allow users to transfer a specific amount of assets to a receiver and a fee amount to a feeReceiver.

Nevertheless, these contracts lack input validation for the amount and fee parameters. Such oversight can result in the submission of transactions with invalid amounts, such as an amount set to 0, or a fee being greater than the amount to transfer, which would revert the transactions.

The affected functions are:

• CeFiSmartTransfer: prepareNativeTransfer(), executeErc20Transfer() and receive().

• DeFiSmartTransfer: executeErc20Transfer() and executeNativeTransfer().

Additionally, there is no threshold for minimum and/or maximum amounts, which can allow malicious actors to congest the network with 0 amounts of dust transactions, particularly via DeFiSmartTransfer::executeErc20Transfer(), given that any user could initiate any number of transactions with msg.value = 0.

function testReceiveNativeTransferFailsWithFeeIsHigherThanAmount(
    address ethSender,
    string memory _id,
    uint256 _amount,
    uint256 _fee,
    address payable _feeReceiver,
    address payable _receiver
  ) public {

    vm.assume(_feeReceiver != address(ceFiSmartTransfer));
    vm.assume(_receiver != address(ceFiSmartTransfer));
    vm.assume(_amount != 0);
    vm.assume(_amount != type(uint256).max);
    _fee = bound(_fee, _amount + 1, type(uint256).max);
    _notFoundryReservedAddress(_receiver);
    _notFoundryReservedAddress(_feeReceiver);

    assert(_fee > _amount);

    testPrepareNativeTransfer(_id, _amount, _fee, _feeReceiver, _receiver);

    hoax(ethSender, _fee);
    vm.expectRevert();
    (bool ok, ) = address(ceFiSmartTransfer).call{value: _amount}("");
    ok;
}

function testPrepareNativeTransfer(
    string memory _id,
    uint256 _amount,
    uint256 _fee,
    address payable _feeReceiver,
    address payable _receiver
  ) public {
    vm.startPrank(operator1);
    assertEq(uint8(ceFiSmartTransfer.nativeTransferLock()), uint8(CeFi.Lock.Unlocked));

    CeFi.NativeTransfer memory _nativeTransfer = CeFi.NativeTransfer({
      id: _id,
      amount: _amount,
      fee: _fee,
      feeReceiver: payable(_feeReceiver),
      receiver: payable(_receiver)
    });

    ceFiSmartTransfer.prepareNativeTransfer(_nativeTransfer);

    (string memory id_, uint amount_, uint fee_, address feeReceiver_, address receiver_) = ceFiSmartTransfer.nativeTransfer();

    assertEq(_id, id_);
    assertEq(_amount, amount_);
    assertEq(_fee, fee_);
    assertEq(_feeReceiver, feeReceiver_);
    assertEq(_receiver, receiver_);

    assertEq(uint8(ceFiSmartTransfer.nativeTransferLock()), uint8(CeFi.Lock.Locked));

    vm.stopPrank();
}

Add checks to ensure that the fee is lower than the amount to transfer, and that the amount to transfer is greater than 0.

Also, consider disallowing transactions below a certain threshold to maintain efficiency and prevent denial of service through dust spamming.

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commits a790ac9 and cea7f6f by following the mentioned recommendations.

The receive() function within the CeFiSmartTransfer contract is designed to automatically forward all received native tokens to addresses specified in the nativeTransfer struct. This process is initiated when an address sends native assets to the contract, thereby triggering the receive() function. Crucially, this function relies on the prior execution of prepareNativeTransfer() by an operator to set up the transfer parameters.

The contract's current design exposes it to a risk of front-running. An operator could strategically (by monitoring pending transactions in the mempool) or accidentally execute prepareNativeTransfer() before the receive function has been triggered. This would allow the operator to modify the nativeTransfer data, potentially redirecting funds to unauthorized addresses and resulting in a direct financial loss for the legitimate transaction sender. Consider the following scenario:

• Alice sends ETH to the contract to execute a transfer based on current nativeTransfer values.

• Operator A calls prepareNativeTransfer() with a higher gas fee to execute the transaction prior to Alice's, modifying the of the receiver and feeReceiver to its address.

• All ETH sent by Alice goes to the malicious operator address.

  function testFrontrunReceiveNativeTransfer(
    string memory _id,
    uint256 _amount,
    uint256 _fee,
    address payable _feeReceiver,
    address payable _receiver
  ) public {
    /* ------------------------------- preparation ------------------------------ */
    vm.assume(_feeReceiver != address(ceFiSmartTransfer));
    vm.assume(_receiver != address(ceFiSmartTransfer));
    vm.assume(_feeReceiver != address(operator1));
    vm.assume(_receiver != address(operator1));
    vm.assume(_amount != 0);
    _fee = bound(_fee, 0, _amount);
    _notFoundryReservedAddress(_receiver);
    _notFoundryReservedAddress(_feeReceiver);
    uint256 balanceReceiverBefore = address(_receiver).balance;
    uint256 balanceFeeReceiverBefore = address(_feeReceiver).balance;

    /* -------------------------------- execution ------------------------------- */
    testPrepareNativeTransfer(_id, _amount, _fee, _feeReceiver, _receiver); // Original native transfer
    _operator1FrontrunAlice(_amount); // Operator1 modifies native transfer
    hoax(alice, _amount);
    (bool ok, ) = (address(ceFiSmartTransfer)).call{value: _amount}("");
    assert(ok);

    /* ------------------------------ verification ------------------------------ */
    uint256 balanceReceiverAfter = address(_receiver).balance;
    uint256 balanceFeeReceiverAfter = address(_feeReceiver).balance;
    assertEq(balanceReceiverAfter, balanceReceiverBefore);
    assertEq(balanceFeeReceiverAfter, balanceFeeReceiverBefore);

    assertEq(address(operator1).balance, _amount);
  }

  function _operator1FrontrunAlice(uint256 _amount) internal {
    testPrepareNativeTransfer("Frontrunning", _amount, 0, payable(operator1), payable(operator1));
  }

  function testPrepareNativeTransfer(
    string memory _id,
    uint256 _amount,
    uint256 _fee,
    address payable _feeReceiver,
    address payable _receiver
  ) public {
    vm.startPrank(operator1);

    CeFi.NativeTransfer memory _nativeTransfer = CeFi.NativeTransfer({
      id: _id,
      amount: _amount,
      fee: _fee,
      feeReceiver: payable(_feeReceiver),
      receiver: payable(_receiver)
    });

    ceFiSmartTransfer.prepareNativeTransfer(_nativeTransfer);

    (string memory id_, uint amount_, uint fee_, address feeReceiver_, address receiver_) = ceFiSmartTransfer.nativeTransfer();

    assertEq(_id, id_);
    assertEq(_amount, amount_);
    assertEq(_fee, fee_);
    assertEq(_feeReceiver, feeReceiver_);
    assertEq(_receiver, receiver_);

    assertEq(uint8(ceFiSmartTransfer.nativeTransferLock()), uint8(CeFi.Lock.Locked));

    vm.stopPrank();
  }

A possible recommendation to solve this issue would be to introduce a time lock mechanism that enforces a minimum delay between the execution of prepareNativeTransfer() and its subsequent transactions. This delay would provide a window for any irregularities to be identified and addressed before the transfer is executed.

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commit eda3810 by creating a timelock mechanism that waits for at least 1 block between preparation and execution of native transfers.

The CeFiSmartTransfer and the DeFiSmartTransfer contracts assume that all ERC20 token transfers are compliant with the IERC20 standard, which is not accurate. For example, USDT's transfer and transferFrom functions do not return a bool:

/**
* @dev transfer token for a specified address
* @param _to The address to transfer to.
* @param _value The amount to be transferred.
*/
function transfer(address to, uint value) public onlyPayloadSize(2 * 32) {
    uint fee = (_value.mul(basisPointsRate)).div(10000);
    if (fee > maximumFee) {
        fee = maximumFee;
    }
    uint sendAmount = _value.sub(fee);
    balances[msg.sender] = balances[msg.sender].sub(_value);
    balances[_to] = balances[_to].add(sendAmount);
    if (fee > 0) {
      balances[owner] = balances[owner].add(fee);
      Transfer(msg.sender, owner, fee);
    }
    Transfer(msg.sender, _to, sendAmount);
}

/**
* @dev Transfer tokens from one address to another
* @param _from address The address which you want to send tokens from
* @param _to address The address which you want to transfer to
* @param _value uint the amount of tokens to be transferred
*/
function transferFrom(address _from, address _to, uint _value) public onlyPayloadSize(3 * 32) {
    var _allowance = allowed[_from][msg.sender];

    uint fee = (_value.mul(basisPointsRate)).div(10000);
    if (fee > maximumFee) {
        fee = maximumFee;
    }
    if (_allowance < MAX_UINT) {
       allowed[_from][msg.sender] = _allowance.sub(_value);
    }
    uint sendAmount = _value.sub(fee);
    balances[_from] = balances[_from].sub(_value);
    balances[_to] = balances[_to].add(sendAmount);
    if (fee > 0) {
        balances[owner] = balances[owner].add(fee);
        Transfer(_from, owner, fee);
    }
    Transfer(_from, _to, sendAmount);
}

This and any other non-compliant tokens may become locked, as any transfer attempt via the executeErc20Transfer() function will revert the transaction because of the missing return value.

Besides this common example, there are tokens that revert on transfers with 0 amounts, and fee-on-transfer tokens, that causes the received amount to be lesser than the accounted amount. For example, DGX (Digix Gold Token) and CGT (CACHE Gold) tokens apply transfer fees, and the USDT (Tether) token also has a currently disabled fee feature. For more reference, see here.

contract USDTSimplified {
  mapping(address => uint) public balances;

  constructor(uint256 _amount) {
    balances[msg.sender] = _amount;
  }

  function balanceOf(address _owner) public view returns (uint balance) {
    return balances[_owner];
  }

  function transfer(address _to, uint _value) public {
    balances[msg.sender] -= _value;
    balances[_to] += _value;
  }
} 

function testExecuteErc20TransferWithNoBooleanReturnFails(
    string memory _id,
    uint256 _amount,
    uint256 _fee,
    address payable _receiver,
    address payable _feeReceiver
  ) external {
    vm.assume(_amount != 0);
    vm.assume(_receiver != address(0));
    vm.assume(_feeReceiver != address(0));

    _fee = bound(_fee, 0, _amount);
    _notFoundryReservedAddress(_receiver);
    _notFoundryReservedAddress(_feeReceiver);

    vm.startPrank(deployer);
    USDTSimplified usdt = new USDTSimplified(_amount);
    assertEq(usdt.balanceOf(deployer), _amount);

    usdt.transfer(address(ceFiSmartTransfer), _amount);
    assertEq(usdt.balanceOf(deployer), 0);
    assertEq(usdt.balanceOf(address(ceFiSmartTransfer)), _amount);

    CeFi.Erc20Transfer memory _erc20Transfer = CeFi.Erc20Transfer({
      id: _id,
      amount: _amount,
      fee: _fee,
      feeReceiver: payable(alice),
      receiver: payable(bob),
      token: address(usdt)
    });

    vm.expectRevert();
    ceFiSmartTransfer.executeErc20Transfer(_erc20Transfer);
}

It is recommended to use OpenZeppelin’s SafeERC20 library to handle most edge cases among ERC20 tokens, including the return value check, to avoid silently failing transfers that could result in a partial or total loss of the users' investment.

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commit 284e2ab by using OpenZeppelin’s SafeERC20 library and its proper implementation.

Comments in the code inside the CeFiSmartTransfer and DeFiSmartTransfer contracts declare that the id field in the NativeTransfer , Erc20Transfer and Transfer structs represents a unique identifier for the transfer.

string id; // A unique identifier for the transfer.

This is not accurate, as the contents of the nativeTransfer variable can be replaced anytime by an operator calling the prepareNativeTransfer function, effectively making it possible to have two different sets of nativeTransfer data sets with the same id.

Similarly, the contents of Erc20Transfer in CeFiSmartTransfer and Transfer in DeFiSmartTransfer are declared via input when calling executeErc20Transfer, making it possible to re-use a previously used id.

This could affect the data collected when monitoring the aforementioned contracts, since the id field is used in the emission of SmartTransferReplaced , SmartTransferComplete and SmartTransferReady events.

Keep a record of declared and/or used IDs and place a check to ensure that an ID cannot be reused in different transfers.

Remediation Plan

ACKNOWLEDGED: The Mesh Connect team acknowledged this finding, stating: We will apply off-chain considerations to ensure that transfer IDs are unique.

The receive() function within the smart contract allows users to transfer to forward the native asset to the designated receiver address and handling the transfer of the fee to the feeReceiver. Following these operations, the contract sets the nativeTransferLock state variable to Unlocked, to allow for a new native transfer to be initiated. However, the nativeTransferLock value is set to Unlocked after the transfer has been made, which is a state-changing operation.

This implementation does not follow the recommended Check-Effects-Interactions pattern. According to this pattern, any modifications to the contract's state should precede calls to external contracts or addresses. While the current usage of the native transfer method mitigates the immediate risk of reentrancy attacks, substituting transfer with the lower-level call could introduce such vulnerabilities. Hence, adherence to the Check-Effects-Interactions pattern remains a best practice, ensuring enhanced security and future-proofing the contract against potential reentrancy threats.

Update the nativeTransferLock to an Unlocked state before executing any transfers. This ensures compliance with the Check-Effects-Interactions pattern, enhancing contract security.

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commit 2e5eef7 by following the recommendation and updating the state prior to executing transfers.

In Solidity, using strings is not the most gas efficient way to handle data, since they are stored as a dynamic array.

If the length is 32 bytes or longer, the slot in which they are defined stores the length of the string * 2 + 1, while their actual data is stored elsewhere (the keccak256 hash of that slot).

However, if a string is less than 32 bytes, the length * 2 is stored at the least significant byte of it’s storage slot and the actual data of the string is stored starting from the most significant byte in the slot in which it is defined.

Consider replacing the id field of the NativeTransfer struct to a bytes32 type.

Storing and manipulating data in bytes32 is more gas-efficient than using string, which involves dynamic storage allocation.

Alternatively, the id field could be stored as a uint96 type value, to be able to pack the value with an address of the same struct (feeReceiver or receiver) and reduce the usage of a storage slot.

Nevertheless, limit the size of the string input to keep it under 32 bytes.

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commit 2e5eef7 by following the recommendation and storing the id field into a bytese32 type.

In the constructor of CeFiSmartTransfer.sol, there is an initialization attempt for the nativeTransfer variable. This initialization is unneeded and impractical, since this NativeTransfer values will not and can not be used until the prepareNativeTransfer() function is called, where the operators will be able to set the NativeTransfer values.

Additionally, the values of the nativeTransfer variable initialized in the constructor should not be valid params, having the amount and fee be 1 wei and both the feeReceiver and receiver being the address(1).

Follow-up assessment:

In the follow-up assessment, it was identified that there is a redundant initialization in the constructor of the CeFiSmartTransfer contract, where the newly declared isLocked variable is initialized to false. This is unnecessary, as boolean type variable are initialized as false by default in Solidity.

Additionally, in the executeNativeTransfer function from the DefiSmartTransfer smart contract, there is a check to ensure the fee is valid:

if ((transfer.fee < 0) || (transfer.fee > transfer.amount)) revert InvalidFeeAmount({fee: transfer.fee, amount: transfer.amount});

Here, the (transfer.fee < 0) code fragment can be omitted, given that the this condition will never be true because uint256 type variables can't store negative values.

Remove the aforementioned fragments of code.

Remediation Plan (1st assessment)

ACKNOWLEDGED: The Mesh Connect team acknowledged this finding and stated that: Maintaining nativeTransfer initialization for gas use consistency will reduce gas consumption across different test scenarios.

Remediation Plan (2nd assessment)

ACKNOWLEDGED: The Mesh Connect team has solved the finding by following the mentioned recommendations.

There is an instance of an unoptimized for loop that may incur in higher gas costs than necessary.

Optimize the instance of a for loop, by not initializing i to its default value of 0, using the pre-increment operator and unchecked code blocks for the loop counter.

for (uint256 i; i < loopAmount;) {
// code logic
unchecked { ++i; }
}

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commit 2e5eef7 by following the mentioned recommendation.

The Operator.sol contract currently defines the addOperator() and removeOperator() functions with public visibility, even though they are not called from within the smart contract, resulting in higher gas costs than necessary.

Modify the aforementioned functions with the external visibility modifier.

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commit 2e5eef7 by following the mentioned recommendation.

Contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Lock the pragma version to the same version used during development and testing.

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commit 2e5eef7 by following the mentioned recommendation.

In Solidity smart contract development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

Consider replacing all revert strings with custom errors. For more reference, see here.

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commit 2e5eef7 by following the mentioned recommendation.

Indexed event fields make the data more quickly accessible to off-chain tools that parse events, and adds them to a special data structure known as "topics" instead of the data part of the log.

However, indexing more fields increases the gas cost for each event emitted. Therefore, extensive indexing is recommended when the advantages of easier data retrieval outweigh the higher gas expenses, particularly in cases where gas efficiency is less of a concern.

Additionally, it's worth noting that when you attempt to index dynamic data types like string in Solidity, which is the case for the contracts in scope, they don't get stored in their original form. Instead, it is stored the Keccak-256 hash of these data types, so to search for a specific string in the logs, developers would have to hash their desired string using Keccak-256 and then search for that resultant hash among the indexed parameters.

If the value to emit in an event is fix-sized, it is recommended to add the indexed keyword when declaring events. For more reference, see the Solidity documentation.

Remediation Plan

ACKNOWLEDGED: The Mesh Connect team has made a business decision to acknowledge this finding and not alter the contracts.

The payer field in the Transfer struct is not used when calling the executeNativeTransfer() function. Additionally the token field is required to be the address(0) to ensures that the transfer is for a native token.

Implement a different struct type for native token transfers that does not require a token field or a payer field.

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commit 2e5eef7 by following the mentioned recommendation.

Throughout the codebase, the approach to integrating external code involved importing full files, eg:

import "./Operator.sol";

It's considered best practice to follow named imports instead of importing full files, for example:

import {Operator} from "./Operator.sol";

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commit 1685f45 by following the mentioned recommendation.

The receive() function from CeFiSmartTransfer and the executeNativeTransfer() function from DeFiSmartTransfer use the native transfer() method to send native assets (e.g. ETH) calling from the smart contract.

The transfer() and send() functions forward a fixed amount of 2300 gas. Historically, it has often been recommended to use these functions for value transfers to guard against reentrancy attacks. However, the gas cost of EVM instructions may change significantly during hard forks which may break already deployed contract systems that make fixed assumptions about gas costs. For example, EIP 1884 broke several existing smart contracts due to a cost increase of the SLOAD instruction.

Avoid the use of transfer() and send() and do not otherwise specify a fixed amount of gas when performing calls. Use .call.value(...)("") instead. Use the checks-effects-interactions pattern and/or reentrancy locks to prevent reentrancy attacks to the system when executing these calls. For more reference see here.

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commit 2e5eef7 by following the mentioned recommendation.

The compiler for Solidity 0.8.20 switches the default target EVM version to Shanghai, which means that the generated bytecode will include PUSH0 opcodes. Be sure to select the appropriate EVM version in case you intend to deploy on a chain other than mainnet like L2 chains that may not support PUSH0, otherwise deployment of your contracts will fail.

Make sure to specify the target EVM version when using Solidity 0.8.20, especially if deploying to L2 chains that may not support the PUSH0 opcode. Stay informed about the opcode support of different chains to ensure smooth deployment and compatibility.

Remediation Plan

ACKNOWLEDGED: The Mesh Connect team acknowledged this finding stating: We will apply off-chain considerations to ensure the target EVM supports PUSH0.

The receive() function of CeFiSmartTransfer and the executeNativeTransfer() function of DeFiSmartTransfer lack checks to verify that the sender of the asset and the receiver and/or feeReceiver are not the same.

Add a check to verify that the address of the transaction sender is not the same as receiver and feeReceiver.

Remediation Plan

ACKNOWLEDGED: The Mesh Connect team has made a business decision to acknowledge this finding and not alter the contracts.

It has been identified that there is inconsistency in the uint256 type declaration in DeFiSmartTransfer and CeFiSmartTransfer contracts. Some of these variables are declared using the uint alias, while others are declared using uint256. While this won't affect the functionality of the contracts, it is a good practice to use the same explicit type declaration for all variables.

Change all uint type declarations to uint256 type declarations.

Remediation Plan

SOLVED: The Mesh Connect team solved this finding in commit 54d9498 by following the mentioned recommendation.