During the security review, it was noted that the code of the contract UniversalSigValidator was extracted from the AmbireTech GitHub repository:

// Copy-paste from https://github.com/AmbireTech/signature-validator/blob/main/contracts/EIP6492.sol
// with minimal modifications

As part of the security tests, the past audit of the reference code was checked, see: https://github.com/AmbireTech/signature-validator/blob/main/ERC6492-Hunter-Security-Audit-Report-V1.0.pdf

While there were two medium-risk findings that were marked as solved, it was noted that both Ambire's and Odos' implementations were still being affected. After further investigation and contacting the Ambire team (also author of the ERC-6492), it was noted that the vulnerabilities were reintroduced mistakenly. See the following commit: https://github.com/AmbireTech/signature-validator/commit/085b2027f12842a4240c3520eb5314a5544ff8d5

As a general best-security practice, it is highly advisable to consistently review all third-party code being utilized and meticulously assess the accompanying security audits.

For this particular case, we suggest examining the enhancements outlined in this report, along with Ambire's updates in their GitHub repository, to ensure alignment with the latest version.

Remediation Plan

SOLVED: The Odos team updated the implementation of the UniversalSigValidator contract in line with the reference code.

It was identified that the OdosLimitOrderRouter contract inherited from OpenZeppelin's Ownable library. Ownership of the contracts that are inherited from the Ownable module can be lost, as the ownership is transferred in a single-step process. The address that the ownership is changed to should be verified to be active or willing to act as the owner. Ownable2Step is safer than Ownable for smart contracts because the owner cannot accidentally transfer smart contract ownership to a mistyped address. Rather than directly transferring to the new owner, the transfer only completes when the new owner accepts ownership.

The following Foundry test was used in order to prove the aforementioned issue:

function testTransferOwnership() public {
  ROUTER.transferOwnership(address(1));
}

To run it, just execute the following forge command:

forge test --mt testTransferOwnership -vvvv

Observe that the test passes and the new owner is now the invalid address(1).

[PASS] testTransferOwnership() (gas: 10355)
Traces:
  [10355] OdosLimitOrdersTests::testTransferOwnership()
    ├─ [7240] OdosLimitOrderRouter::transferOwnership(0x0000000000000000000000000000000000000001)
    │   ├─ emit OwnershipTransferred(previousOwner: OdosOwner: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], newOwner: 0x0000000000000000000000000000000000000001)
    │   └─ ← [Stop] 
    └─ ← [Stop] 

To mitigate the risks associated with single-step ownership transitions and enhance contract security, it is recommended to adopt a two-step ownership transition mechanism, such as OpenZeppelin's Ownable2Step. This approach introduces an additional step in the ownership transfer process, requiring the new owner to accept ownership before the transition is finalized. The process typically involves the current owner calling a function to nominate a new owner, and the nominee then calling another function to accept ownership.

Implementing Ownable2Step provides several benefits:

1. Reduces Risk of Accidental Loss of Ownership: By requiring explicit acceptance of ownership, the risk of accidentally transferring ownership to an incorrect or zero address is significantly reduced.

2. Enhanced Security: It adds another layer of security by ensuring that the new owner is prepared and willing to take over the responsibilities associated with contract ownership.

3. Flexibility in Ownership Transitions: Allows for a smoother transition of ownership, as the nominee has the opportunity to prepare for the acceptance of their new role.

By adopting Ownable2Step, contract administrators can ensure a more secure and controlled process for transferring ownership, safeguarding against the risks associated with accidental or unauthorized ownership changes.

Remediation Plan

SOLVED: The Odos team solved the issue by implementing the recommendations described above.

During the security review, it was observed that users of the OdosLimitOrderRouter can pre-sign an order by using the following external function:

/// @notice Sets a pre-signature for the specified order hash
/// @param orderHash EIP712 encoded order hash of single or multi input limit order
/// @param preSigned True to set the order as enabled for filling with pre-sign, false to unset it
function setPreSignature(
  bytes32 orderHash,
  bool preSigned
)
external
{
  preSignedOrders[msg.sender][orderHash] = preSigned;
  emit OrderPreSigned(orderHash, msg.sender, preSigned);
}

A malicious user could pre-sign an order using one smart contract wallet (SCW) of their own, and sell the account afterward for the amount of assets it holds. Then, once the transaction has taken place, the pre-signed Odos limit order can still be filled without the approval of the current SCW owner.

This security finding has been rated as low risk because part of the risk is the responsibility of the user purchasing the SCW. Additionally, as indicated in the "Centraliztion Risks" finding, Odos can decide whether to fill an order or not, so it could still perform some off-chain validations to avoid executing transactions in such circumstances.

The following Foundry test can be used to illustrate a potential attack scenario where:

1. The attacker owns a smart contract wallet (SCW) and uses it to pre-sign a valid order

2. Sells the SCW to a victim (transferring the ownership)

3. Filling the order afterwards

function test_PreSign_changeOwner() public {
    address accountAddress = address(SCW);

    // mint & approve input token
    MockERC20(defaultOrder.input.tokenAddress).faucet(accountAddress, defaultOrder.input.tokenAmount);
    vm.prank(accountAddress);
    MockERC20(defaultOrder.input.tokenAddress).approve(address(ROUTER2), defaultOrder.input.tokenAmount);

    // encode account address to 20 bytes
    bytes memory encodedSignature = abi.encodePacked(accountAddress);

    // orderOwner still not retrievable because the order has not been pre-signed yet
    vm.expectRevert();
    address orderOwner = ROUTER2.exposed_getOrderOwnerOrRevert(
        orderHash, encodedSignature, SignatureValidator.SignatureValidationMethod.PreSign
    );

    // pre-sign the order using the SCW
    vm.prank(accountAddress);
    ROUTER2.setPreSignature(orderHash, true);

    // pre-signature working correctly now to retrieve the orderOwner (SCW address)
    orderOwner = ROUTER2.exposed_getOrderOwnerOrRevert(
        orderHash, encodedSignature, SignatureValidator.SignatureValidationMethod.PreSign
    );

    assertEq(accountAddress, orderOwner);

    // Sell & transfer the ownership of the SCW
    SCW.transferOwnership(VICTIM_ADDRESS);

    // The preSign still works
    orderOwner = ROUTER2.exposed_getOrderOwnerOrRevert(
        orderHash, encodedSignature, SignatureValidator.SignatureValidationMethod.PreSign
    );

    assertEq(accountAddress, orderOwner);

    SignatureValidator.Signature memory sig = SignatureValidator.Signature(encodedSignature, SignatureValidator.SignatureValidationMethod.PreSign);
    OdosLimitOrderRouter.LimitOrderContext memory context = getDefaultContext(defaultOrder.output.tokenAmount);

    // Whitelist a filler to fill the order
    ROUTER2.addAllowedFiller(address(this));

    // Allowed filler fills the pre-signed order
    ROUTER2.fillLimitOrder(defaultOrder, sig, context);
}

Implement a system to avoid scammers from executing pre-signed limit orders of smart contract accounts they do not own anymore. Odos could check whether the owner of the SCW has changed since the pre-signature was registered.

Remediation Plan

RISK ACCEPTED: The Odos team accepted the risk of this finding.

The built-in EVM precompile ecrecover is susceptible to signature malleability, which could lead to replay attacks. While this is not immediately exploitable, this may become a vulnerability if used elsewhere.

The ecrecover function is used in the isValidSigImpl() function to recover the address from the signature.

Consider using OpenZeppelin's ECDSA library instead of the built-in function.

Remediation Plan

SOLVED: The Odos team solved the issue by implementing the recommendations described above.

It was identified that the OdosLimitOrderRouter contract inherited from OpenZeppelin's Ownable library. In the Ownable contracts, the renounceOwnership() function is used to renounce the Owner permission. Renouncing ownership before transferring would result in the contract having no owner, eliminating the ability to call privileged functions.

/**
 * @dev Leaves the contract without owner. It will not be possible to call
 * `onlyOwner` functions. Can only be called by the current owner.
 *
 * NOTE: Renouncing ownership will leave the contract without an owner,
 * thereby disabling any functionality that is only available to the owner.
 */
function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
}

The following Foundry test was used in order to prove the aforementioned issue:

function testRenounceOwnership() public {
  ROUTER.renounceOwnership();
}

To run it, just execute the following forge command:

forge test --mt testRenounceOwnership -vvvv

Observe that the test passes and the ownership has been renounced (address(0)).

[PASS] testRenounceOwnership() (gas: 5291)
Traces:
  [8073] OdosLimitOrdersTests::testRenounceOwnership()
    ├─ [7052] OdosLimitOrderRouter::renounceOwnership()
    │   ├─ emit OwnershipTransferred(previousOwner: OdosOwner: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], newOwner: 0x0000000000000000000000000000000000000000)
    │   └─ ← [Stop] 
    └─ ← [Stop] 

It is recommended that the Owner cannot call renounceOwnership() without first transferring ownership to another address. In addition, if a multi-signature wallet is used, the call to the renounceOwnership() function should be confirmed for two or more users.

Remediation Plan

SOLVED: The Odos team solved the issue by implementing the recommendations described above.

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.

It is recommended to replace hard-coded revert strings in require statements for custom errors, which can be done following the logic below.

1. Standard require statement (to be replaced):

require(condition, "Condition not met");

2. Declare the error definition to state

error ConditionNotMet();

3. As currently is not possible to use custom errors in combination with require statements, the standard syntax is:

if (!condition) revert ConditionNotMet();

More information about this topic in Official Solidity Documentation.

Remediation Plan

SOLVED: The Odos team solved the issue by implementing the recommendations described above.

It was identified that the contracts in scope are using or accepting a Solidity version greater or equal than [0.8.20](https://github.com/ethereum/solidity/releases/tag/v0.8.20), with its default Shanghai EVM version. The Shanghai fork introduced the PUSH0 opcode that is still not supported on all EVM chains.

Furthermore, by reviewing the Foundry configuration (foundry.toml file), it was observed that the contracts would be deployed using the Solidity compiler version 0.8.24:

solc-version = "0.8.24"

Given that the contracts in scope are meant to be deployed to multiple chains (some of them not yet supporting the new opcode PUSH0), it is recommended to consider fixing the pragma Solidity version to a version that does not use PUSH0.

Remediation Plan

SOLVED: The Odos team solved the issue by fixing the Solidity version to 0.8.19 on all contracts.

All the functions to fill limit orders in the provided contracts are functionalities that involve the transfer of ERC20 tokens to external addresses. However, none of these functions implement any reentrancy protection mechanism. While the Odos team indicated that the fulfillment of limit orders will be executed by them, so they could prevent certain behaviors, it is always recommended to have a reentrancy protection built-in.

Even with the limited risk, consider implementing re-entrancy protection on all external and public functions with external interactions.

To protect against cross-function reentrancy attacks, it may be necessary to use a mutex. By using this lock, an attacker can no longer exploit the withdraw function with a recursive call. OpenZeppelin has its own mutex implementation called ReentrancyGuard which provides a modifier to any function called nonReentrant that guards the function with a mutex against Reentrancy attacks.

Remediation Plan

ACKNOWLEDGED: The Odos team acknowledged this finding.

The approve() function defined by the ERC20 token standard can be vulnerable to race-condition attacks if not protected carefully. The main problem is that the function overwrites the current allowance without considering whether the spender has already utilized it, potentially leading to unintended token transfers beyond the token owner's intention. For more information, see: https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729

As shown below, the OdosLimitOrderRouter contract is making use of the insecure approve() function, and it is not checking the returned value:

// Approve spending
IERC20(tokensIn[i]).approve(inputReceivers[i], amountsIn[i]);

To circumvent ERC20's approve functions race-condition vulnerability, use OpenZeppelin's SafeERC20 library's safe{Increase|Decrease}Allowance() functions.

Remediation Plan

SOLVED: The Odos team solved the issue by removing the use of the approve() function.

The contracts in scope use multiple pragma versions with unlocked versions. Contracts should be deployed with the same compiler version and flags with which they were thoroughly tested. Locking the pragma helps to ensure that contracts are not accidentally deployed using another pragma, for example, either an outdated pragma version that could introduce bugs that negatively affect the contract system or a recently released pragma version that has not been extensively tested.

**Reference: https://github.com/ethereum/solidity/releases**

In the Solidity GitHub repository, there is a json file where are all the bugs found in the different compiler versions.

**Reference: https://github.com/ethereum/solidity/blob/develop/docs/bugs_by_version.json**"

Consider locking the pragma version. When possible, do not use floating pragmas. Specifying a fixed compiler version ensures that the bytecode produced does not vary between builds. This is especially important if you rely on bytecode-level verification of the code.

Remediation Plan

SOLVED: The Odos team solved the issue by fixing the Solidity version to 0.8.19 on all contracts.

In Solidity, when working with arrays, it is common to use a loop to iterate over the elements. However, if the array length is not cached outside the loop, the compiler will read the length of the array during each iteration. This can result in additional gas consumption and reduced efficiency.

After thorough investigation, it was noted that none of the instances were using arrays from storage but from calldata. Therefore, this finding has been marked as 'Not Applicable' because the gas savings no longer apply.

Remediation Plan

NOT APPLICABLE: The Odos team stated that the finding is not applicable.

The constructor of the OdosLimitOrderRouter contract lacks protection against using the address(0) value for the parameter named _odosRouterV2. Consequently, if called with a value of 0, the ODOS_ROUTER_V2 state variable could remain uninitialized, rendering most functionalities unusable.

constructor(address initialOwner, address _odosRouterV2)
  EIP712("OdosLimitOrderRouter", "1")
  Ownable(initialOwner) {
    ODOS_ROUTER_V2 = _odosRouterV2;
}

Additionally, the transferRouterFunds() function contained a dest address parameter that is never checked against address(0) to avoid burning router funds mistakenly.

It is recommended to make sure all addresses are properly initialized (avoiding the zero address) as this would break critical functionalities and, given that ODOS_ROUTER_V2 is an immutable variable, the situation would not be reversible.

Remediation Plan

SOLVED: The Odos team solved the issue by implementing the recommendations described above.

The SignatureValidator contract declares the following constant, but it is never used.

// bytes4(keccak256("isValidSignature(bytes32,bytes)")
bytes4 internal constant EIP1271_MAGICVALUE = 0x1626ba7e;

Unused variables should be cleaned up from the code if they have no purpose. Clearing these variables can reduce gas usage during the deployment of contracts, and also increases the readability of the code.

Remediation Plan

SOLVED: The Odos team solved the issue by removing the unused constant.

The following comment was found in the UniversalSigValidator contract:

// The order here is striclty defined in https://eips.ethereum.org/EIPS/eip-6492

The word striclty above should be spelled as strictly instead.

To maintain clarity and trustworthiness, it is essential to rectify any typographical errors present within the contracts. Correcting such errors minimizes the likelihood of confusion and reinforces confidence in the accuracy and integrity of the documentation.

Remediation Plan

SOLVED: The Odos team solved the issue by implementing the recommendations described above.