Internal Exchange Re-Assessment - dappOS

The setFeeTier function allows the assignment of fee tier 1 to any account. This tier is reserved for VW_FEE_TIER, automatically assigned when _isVirtualWallet returns true. Allowing manual assignment undermines the logic and integrity of the reserved fee tier, leading to potential conflicts in fee calculations.

• Restrict assignment of fee tier 1: Add a validation to reject manual setting of VW_FEE_TIER:

  solidity require(_feeTier != VW_FEE_TIER, "Fee tier 1 is reserved for virtual wallets");

• Documentation: Clarify in the contract and function comments that fee tier 1 is reserved for virtual wallets and should not be set manually.

• Testing: Verify that attempts to assign fee tier 1 to accounts manually result in contract reverts.

RISK ACCEPTED: The design is such that for ordinary users, the level is 0 and for vw users, the level is 1. If a level has been assigned to an address, then this assigned level will be considered as authoritative.

The wrap and unwrap functions assume a 1:1 correspondence between originToken and the WrappedToken amounts, but there is no check to ensure that the decimals of the originToken and the WrappedToken match. This can lead to unintended and incorrect token amounts being minted or burned, potentially causing financial discrepancies.

1. Perform a check in the initWToken function to ensure the decimals of originToken match the DECIMALS value set during initialization.

2. Extract the decimals directly from the originToken using its decimals() function and not rely on external parameters.

RISK ACCEPTED: This problem does exist. However, when setting the token, the administrator will pay attention to the parameters.

The setFeeInfo function in OrderBookConfig does not validate whether the maker, taker, and exclusiveFill fee values are within a valid range. Without these checks, setting a fee value less than 0 or greater than FEE_RATE_BASE (e.g., -0.1 or 1.1 relative to FEE_RATE_BASE) will cause severe issues:

1. Underflow in Multipliers: Functions like getMakerFeeMultiplier, getTakerFeeMultiplier, and getExclusiveFillFeeMultiplier rely on subtracting the fee from FEE_RATE_BASE. Invalid values will cause these calculations to underflow or overflow, resulting in corrupted multiplier values.

2. Denial of Service (DoS): The corrupted fee multipliers can cascade through the system, causing critical arithmetic errors, invalid token transfer calculations, or reverts in downstream contract operations, effectively halting the order book.

npx hardhat test --grep "When maker fee is less than 0"

Diff:

diff --git a/test/e2e/basic.test.ts b/test/e2e/basic.test.ts
index 8f417d9..1c51abb 100644
--- a/test/e2e/basic.test.ts
+++ b/test/e2e/basic.test.ts
@@ -507,8 +507,13 @@ describe("Fill Order", () => {

       const FEE_RATE_DECIMALS = await OrderBookConfig.FEE_RATE_DECIMALS();
       const feeInfo = {
-        maker: parseUnits("-0.1", FEE_RATE_DECIMALS),
-        taker: parseUnits("0.1", FEE_RATE_DECIMALS),
+        maker: parseUnits("1.1", FEE_RATE_DECIMALS),
+        taker: parseUnits("0.90", FEE_RATE_DECIMALS),
+        // 0.90 will panic as the difference being 0.10 with FEE_RATE_BASE will
+        // cause the complement of the underflowed 1-1.1 to overflow when checking for
+        // makerFeeMultiplier + takerFeeMultiplier
+        // However, 0.95 will not panic, any value lower or equal than 0.90, will panic
+        // taker: parseUnits("0.95", FEE_RATE_DECIMALS),
         exclusiveFill: 0,
       };

@@ -527,6 +532,10 @@ describe("Fill Order", () => {
         feeTier
       );

+      console.log(feeInfo)
+      console.log(await OrderBookConfig.getMakerFeeMultiplier(buyer.address));
+      console.log(await OrderBookConfig.getTakerFeeMultiplier(seller.address));
+
       /**
        * open order
        * direction: buy (USDT -> WETH)

1. Ensure that the maker, taker, and exclusiveFill fee values are within the range -100 to 100 (relative to FEE_RATE_BASE):

require(fee.maker >= -FEE_RATE_BASE && fee.maker <= FEE_RATE_BASE, "Invalid maker fee");

require(fee.taker >= -FEE_RATE_BASE && fee.taker <= FEE_RATE_BASE, "Invalid taker fee");

require(fee.exclusiveFill >= -FEE_RATE_BASE && fee.exclusiveFill <= FEE_RATE_BASE, "Invalid exclusiveFill fee");

1. Add Comprehensive Unit Tests:

   • Test edge cases for fee values, including values at the lower and upper bounds of the range.

   • Test with invalid values (e.g., < -100 or > 100) to confirm the contract reverts as expected.

2. Update Documentation:

   • Document the valid range of fee values (-100 to 100 relative to FEE_RATE_BASE) for developers and integrators.

By enforcing these checks, the contract can prevent underflows and maintain the stability of the order book system.

RISK ACCEPTED: The fee is set by the administrator, who ensures that the maximum negative fee rate for the maker plus the maximum positive fee rate for the taker is always greater than 0.

The setVwManager function in OrderBookConfig does not prevent duplicate entries in the registeredVwManagers array, which could lead to redundant operations or unexpected behavior. Moreover, the function does not validate whether the provided virtual wallet manager addresses comply with the expected EIP-165 interface or implement the walletOwner function. This could result in adding invalid or non-functional managers, potentially causing runtime errors or logical inconsistencies when interacting with them.

1. Prevent Duplicates: Before adding a new entry to the registeredVwManagers array, check if it is already present. Reject duplicates to ensure the list contains unique entries.

2. Validate Compliance with EIP-165 Interface: Use the supportsInterface function to ensure that each provided address implements the expected interface for a virtual wallet manager.

3. Verify walletOwner Functionality: Perform a test call to the walletOwner function on each manager to confirm its presence and functionality. Revert the transaction if the call fails or the function is not implemented.

These validations should be implemented before updating the state or emitting the VwManagerSet event to ensure data integrity and operational reliability.

RISK ACCEPTED: This problem does exist. However, the administrator will pay attention to the parameters.

The addPairInfo function fails to validate and enforce critical constraints:

• Token Equality: No check ensures that token0 and token1 are distinct.

• Token Standardization: Tokens are not ordered consistently (e.g., lexicographically or by asset type).

• Duplicate Pairs: Multiple identical trading pairs (e.g., ETH/USD) can be added due to lack of uniqueness checks.

• Validate Token Equality: Add a requirement that token0 and token1 are distinct.

    require(token0 != token1, "Token0 and Token1 must be different");

• Enforce Standardized Token Order: Ensure token0 is always the smaller address or the "base" token by predefined logic.

    if (token0 > token1) {
        (token0, token1) = (token1, token0);
    }

• Prevent Duplicate Pairs: Use a mapping to ensure each pair (token0, token1) is unique.

    mapping(address => mapping(address => bool)) public pairExists;

    require(!pairExists[token0][token1], "Trading pair already exists");
    pairExists[token0][token1] = true;

These changes ensure the integrity and consistency of trading pair configurations.

RISK ACCEPTED: This problem does exist. However, the administrator will pay attention to the parameters.

The WrappedTokenUpgradeable contract allows initWToken and initSupply to be called in any order, which could lead to unintended behavior. Additionally, these functions can be executed before the initialize function, potentially causing logical inconsistencies, especially with the isOriginChain value. This can result in an improperly initialized contract with discrepancies in its state.

1. Ensure initWToken can only be called after initSupply by adding a check for isInitSupply == true in initWToken.

2. Ensure initSupply and initWToken cannot be called before initialize by using _getInitializedVersion to confirm the contract's initialization state.

3. Refactor the logic to enforce proper order and consistency during initialization.

SOLVED: The code is now checking for isInitSupply.

The OrderBook contract does not include a call to _disableInitializers in its constructor. This omission allows the implementation contract to potentially be initialized, which can lead to unexpected behavior or security vulnerabilities if the implementation is accidentally or maliciously used as a deployable contract.

Add a call to _disableInitializers in the constructor of the OrderBook contract. This will prevent any unintended or unauthorized initialization of the implementation contract, ensuring it cannot be used directly. The constructor should look as follows:

constructor() {
    _disableInitializers();
}

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

The adminWithdraw and adminRefundAndWithdraw functions use the transfer method for ETH transfers. This method imposes a gas limit of 2300 on the recipient, which is insufficient for contracts with complex fallback or receive functions. If the recipient is a contract (e.g., a vault or multisig wallet), the transfer will fail, potentially locking ETH in the OverBid contract.

Replace the use of transfer with a safe alternative, such as call. This allows for greater flexibility by providing an adjustable gas stipend, ensuring the transaction does not fail due to gas limitations. The implementation can be as follows:

(bool success, ) = to.call{value: amount}("");
require(success, "ETH transfer failed");

This approach ensures compatibility with recipient contracts that require more than 2300 gas for their execution while maintaining safety through the use of explicit checks on the success flag.

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

The batchCancelThenFill function invokes batchFillThenOpen, which may fail under certain conditions. However, the approvals set by forceApprove on the tokens remain active even after a failure. This creates a potential risk where the approved amounts can be misused by unintended parties or remain unnecessarily high, exposing the contract to potential vulnerabilities.

To enhance security and prevent misuse, reset the approvals for all tokens to 0 after a failed batchFillThenOpen call. This can be done within the catch block, where the failure is handled. Updating the approval ensures that no lingering approvals exist, reducing the attack surface and maintaining the integrity of the contract.

For example:

1. After emitting the BatchFillThenOpenFailed event, iterate over the tokens array.

2. Reset the approval for each token to 0 using IERC20(tokens[i]).forceApprove(orderBookVault, 0).

This measure will improve security and prevent unintentional consequences on leftover approvals.

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

The create2Proxy function leverages the initImpl (an ERC1967Upgrade contract) to perform an upgrade through upgradeToAndCall after deploying a TransparentUpgradeableProxy. This approach bypasses the standard admin flow enforced by TransparentUpgradeableProxy, where only the _proxyAdmin can call upgradeToAndCall. As a result, the admin functionality is temporarily shifted to the caller of create2Proxy, which allows the caller to set the initial implementation and execute a call without relying on the _proxyAdmin.

This setup introduces a few key issues:

1. Temporary Admin Responsibility: The caller of create2Proxy has direct control over the upgradeToAndCall function, effectively acting as the admin before transferring control to the _proxyAdmin.

2. Opaque Admin Address: The create2Proxy implementation does not expose or store the _proxyAdmin address of the proxy. This makes it difficult to verify or interact with the admin later without relying on events (AdminChanged or OwnershipTransferred) or manual tracking.

3. Decoupling of ERC1967Upgrade: The use of ERC1967Upgrade as a trampoline contract is misleading, as it becomes irrelevant after the create2Proxy function completes. Future interactions with the proxy must go through the _proxyAdmin.

To address these issues:

1. Modify the create2Proxy implementation to ensure that admin functionality adheres strictly to the _proxyAdmin logic of TransparentUpgradeableProxy. This can be achieved by avoiding the trampoline contract and ensuring the upgradeToAndCall function is invoked through the _proxyAdmin.

2. Expose the _proxyAdmin address in a predictable manner, such as emitting it during deployment or storing it in a mapping for reference.

3. Clearly document the role of the initImpl to ensure its temporary nature is well-understood. Alternatively, consider removing the trampoline contract entirely and interacting directly with the proxy's admin logic.

These changes will enhance transparency and maintainability while ensuring the admin flow is consistent with standard practices.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

The transfer, adminWithdraw, and adminRefundAndWithdraw functions allow the administrator or whitelisted plugins to transfer funds arbitrarily from the OverBid contract. This poses significant centralization risks:

• Arbitrary Transfers: The admin can transfer any amount of funds to any address, including personal wallets, without oversight.

• Plugin Whitelisting: The admin can whitelist any contract or address via modifyPlugin, enabling indirect, arbitrary transfers.

• No Governance or Safeguards: There are no mechanisms, such as multi-signatures, to limit or monitor admin actions, leaving user funds exposed to misuse.

Mitigate the centralization risk by implementing the following safeguards:

• Multi-Signature Approval: Require multi-signature authorization for sensitive actions such as modifyPlugin, adminWithdraw, and adminRefundAndWithdraw. Use a trusted wallet solution like Gnosis Safe.

• Withdrawal Limits: Introduce limits on withdrawal amounts, such as daily caps, to prevent draining the contract entirely in a single transaction.

• Direct User Withdrawals: Allow users to withdraw their own funds directly without admin intervention where possible, reducing dependency on centralized control.

• Time-Lock Mechanism: Require a time-lock for sensitive actions, allowing users to react or withdraw their funds in case of malicious behavior by the admin.

• Enhanced Transparency: Emit detailed events for every withdrawal, plugin modification, or administrative action. Include parameters such as recipient, amount, and reason for the transfer.

• Periodic Audit and Monitoring: Regularly audit the whitelist of plugins and all admin actions. Publish results to maintain transparency and user trust.

Implementing these controls will help address the centralization risk and safeguard user assets against potential abuse.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

The setIntentAsset function in OrderBookConfig allows the addition of tokens to the intentAssets set without verifying if the provided token addresses implement the required IIntentToken interface. This could lead to the inclusion of invalid tokens that do not conform to the expected behavior of IntentTokens. Such tokens may lack the getBaseAsset function or fail to meet interface standards, potentially resulting in runtime errors or unexpected behavior in downstream operations that rely on intentAssets.

Add a validation step to ensure that each token in the tokens array adheres to the IIntentToken interface. This can be achieved by:

1. Using the EIP-165 interface detection standard (supportsInterface) to confirm compliance with IIntentToken.

2. Alternatively, perform a test call to the getBaseAsset function on each token and revert if the function call fails.

Integrating such validation ensures the integrity of the intentAssets set and avoids runtime inconsistencies.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

The WrappedTokenUpgradeable contract uses hardcoded default values "Test" for the name and symbol of the ERC20 token. These values are not configurable through the initializer, which can lead to unexpected or inappropriate token names during deployment.

Update the initialize function to accept name and symbol parameters, allowing the deployer to set meaningful values for the ERC20 token attributes at deployment.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

The SignLibrary contract contains debugging instrumentation, such as console.log, which is part of the Foundry framework. These calls are typically used during development and testing to output debugging information. However, their presence in production code introduces unnecessary bytecode, potentially increases gas costs, and may expose sensitive information if debug output mechanisms are inadvertently activated in a live environment.

Remove all debugging instrumentation, including console.log calls, from the SignLibrary contract and any other contracts before deploying them to a production environment. If debugging is required post-deployment, utilize off-chain logging systems or event emission for controlled and secure observability.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

The decoding functions in the VWCode library (chainidsAndExpTime, splitCode, getFlag) return values using incorrect type sizes. For example, dstChainId is declared as uint256 but should be uint32 as per the type used in the genCode function. This mismatch can lead to confusion and possible bugs when integrating with other components expecting the correct types.

1. Update the return types of these functions to match the original sizes defined in the genCode function (uint32 for dstChainId, srcChainId, and time, and uint16 for action and flag).

2. Explicitly cast the extracted values to their expected types to ensure type safety.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

The executeDstOrder function relies on getMatchDstAmount from the OrderBookConfig contract to compute destination amounts. This function assumes that:

1. All srcChainIds are sorted.

2. allSrcChainIds is a subset of srcChainIds.

If these conditions are violated, getMatchDstAmount could return incorrect or duplicated amounts, leading to:

• Over-crediting or under-crediting the order, which could disrupt the cross-chain asset balance.

• Allowing manipulation or exploitation of the mismatch to divert funds or destabilize the order execution process.

This lack of validation introduces a significant vulnerability to the integrity of the cross-chain execution mechanism.

In executeDstOrder, add explicit validation to ensure that:

1. Sorted Chain IDs: Verify that srcChainIds is sorted before passing it to getMatchDstAmount. This can be achieved by iterating through the array and ensuring each element is greater than or equal to the previous one.

2. Subset Verification: Validate that allSrcChainIds is indeed a subset of srcChainIds. This can be achieved using a mapping to track srcChainIds and checking the existence of each allSrcChainIds entry.

Additionally, update getMatchDstAmount in OrderBookConfig to handle unsorted or improperly structured input gracefully, potentially reverting the transaction if assumptions are not met.

These validations will ensure that executeDstOrder processes orders reliably and securely, preventing potential inconsistencies or exploit scenarios.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

The _matchOrders function performs computations to determine fill amounts, even when the unfilled amount for either makerOrder or takerOrder is zero. This results in unnecessary calls to getSendingAmount and other logic that could be skipped. These redundant operations add unnecessary overhead to the function execution.

Introduce an early return in _matchOrders to skip processing when either makerOrder or takerOrder has no remaining unfilled amount.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

The getSendingAmount function, declared in OrderBookConfig, is exclusively used within the OrderBook contract. This creates an unnecessary external call each time the function is used, incurring additional gas costs. Since the function does not rely on the state of OrderBookConfig, it could be implemented as an internal function or moved to a library to optimize gas usage.

Refactor the getSendingAmount logic into a library or implement it directly as an internal function in the OrderBook contract.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

The batchExchangeUnderlyingAssetToIntentAsset function calls exchangeUnderlyingAssetToIntentAsset, and both include a onlyRole(BOT_ROLE) modifier. This results in unnecessary duplicated role checks for every iteration in the loop. Similarly, this issue exists for batchExchangeIntentAssetToUnderlyingAsset and exchangeIntentAssetToUnderlyingAsset, as well as for batchCancelThenFillByIds and batchCancelThenFill.

This redundancy increases gas usage unnecessarily, especially in loops.

Refactor the affected functions by creating internal functions without the onlyRole modifier. The external functions with the onlyRole modifier can then call the internal function in the loop. This avoids duplicated checks while maintaining functionality.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

The OrderBookVault contract includes separate setter functions, such as setOrderBook and setOrderBookConfig, to configure its dependencies. However, these could be directly initialized in the initialize function to streamline the deployment process and reduce operational overhead. Additionally, these setters lack validation checks, such as verifying that the provided addresses are not zero or ensuring compliance with ERC165 standards. This could result in misconfiguration or interaction with incompatible contracts, potentially leading to runtime errors.

Combine the configuration of OrderBook and OrderBookConfig into the initialize function to simplify contract setup and improve deployment efficiency. Add validation checks in the setters to ensure the provided addresses are non-zero and compatible with the expected interfaces. For instance, use Address.isContract() to confirm the addresses are contracts and verify ERC165 compliance using IERC165.supportsInterface.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

In the createSrcOrder and rejectSrcOrder functions of the OrderBook contract, the transfer logic uses isIntentAsset to determine whether token amounts should be adjusted using the intent token conversion rate. If the isIntentAsset state of a token changes between these two operations, it may result in inconsistent token transfer amounts. Specifically, the TransferHelper.safeTransferBaseFrom function could calculate incorrect values due to a mismatch in the expected base token amount and the actual intent token state. This edge case can lead to operational discrepancies but requires administrative oversight for exploitation, as it depends on a protocol admin changing the isIntentAsset state mid-operation.

Protocol administrators should ensure that the isIntentAsset state of tokens remains consistent and unchanged during the lifecycle of orders, especially between createSrcOrder and rejectSrcOrder. This can be enforced by:

1. Introducing strict administrative policies to prevent modifications to isIntentAsset for tokens in active orders.

2. Implementing an internal tracker or snapshot mechanism that locks the isIntentAsset state of tokens used in orders until their lifecycle is completed. While this adds complexity, it ensures operational consistency.

3. Logging and monitoring all administrative changes to isIntentAsset to identify and address any discrepancies proactively.

Although no direct implementation change is immediately required, raising awareness of this potential issue is crucial for maintaining protocol integrity.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

The setOrderBookVault and setOrderBookConfig functions in the OrderBook contract allow the admin to change critical components of the protocol: the vault managing asset transfers and the configuration managing trading pairs, fees, and other parameters. If these functions are improperly used or maliciously exploited, they can result in a complete loss of protocol integrity. For example:

• Replacing the OrderBookVault with a malicious or invalid implementation could lead to unauthorized asset transfers.

• Replacing the OrderBookConfig could introduce fraudulent or unintended trading pairs, price manipulation, or fee changes.

These operations, if unrestricted or mishandled, represent a high-risk attack vector for the protocol.

Restrict or harden the conditions under which setOrderBookVault and setOrderBookConfig can be executed to maintain protocol integrity. This can be achieved by:

1. Validation: Ensure that new contract addresses implement the expected interfaces (e.g., using ERC165 or by verifying specific function selectors).

2. Access Control: Limit these functions to a trusted role and implement additional authorization checks or multi-signature validation for critical updates.

3. Safety Locks: Add a time-lock mechanism or governance approval process to allow the community to review and contest any proposed changes before they take effect.

4. Initialization Check: Prevent updates to these configurations if they would disrupt already initialized components or orders in progress.

5. Auditability: Emit detailed events with new contract addresses and justification for the change to ensure transparency and accountability.

By implementing these safeguards, the protocol can reduce the risk of catastrophic failures or malicious exploitation stemming from these functions.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

In the exchangeIntentAssetToUnderlyingAsset function, the require statement for checking if callParam.node exists in the nodeList whitelist is placed after some potentially unnecessary computations. This could result in avoidable gas consumption if the node is not whitelisted, as these computations serve no purpose when the function execution is reverted.

Move the require(nodeList[callParam.node], "node not in whitelist"); statement to the beginning of the function. This ensures that the whitelist check is performed before any other operations, thereby optimizing gas usage by reverting early if the condition is not met.

ACKNOWLEDGED: The dappOS team acknowledged this issue.

In the batchCancelThenFill function, balances are calculated, and some may be set to zero during processing. These zero balances are then passed to the batchFillThenOpen function, which could unnecessarily handle them. This behavior leads to inefficient gas usage and potential redundancy.

Before calling the batchFillThenOpen function, filter out the zero balances from the tokens and balances arrays. This can be achieved by implementing a logic similar to the _getNonZeroBalancesInBase function, ensuring that only tokens with non-zero balances are included in the final call.

ACKNOWLEDGED: The dappOS team acknowledged this issue.