6.1 Proxy orders use the owner’s deposited funds instead of the proxy’s funds
The placeOrderByProxy(...) function allows an approved proxy address to place orders on behalf of a user (order_owner). This functionality is intended for Request-for-Quote (RFQ) systems, where a market maker acts as the proxy to execute a trade for a user. The expected behavior is that the proxy (msg.sender) provides the funds for the order it initiates.
However, the underlying payment logic in the _handleTokenTransfer(...) function does not distinguish between a proxy order and a regular order. When sourcing funds, the function first depletes the order_owner’s internal token balance (traderBalances) before requesting any funds from the proxy.
The placeOrderByProxy(...) function calls the internal _placeOrder(...) function, which in turn calls _handleTokenTransfer(...) to manage the movement of assets.
function placeOrderByProxy(
// ...
address order_owner,
// ...
) public payable /* ... */ {
// ...
// @audit Calls _placeOrder with the user as `order_owner` but msg.sender as the proxy.
ExecutionResult memory result = _placeOrder(
// ...
order_owner
);
// ...
}
The core issue lies within _handleTokenTransfer(...), where client is the order_owner. The logic first checks and uses the client’s deposited funds.
function _handleTokenTransfer(
address client, // @audit This is the `order_owner`
...
) internal {
// ...
// processing token_x
if (x_to_send < x_to_receive) {
// ...
uint128 clients_shares = traderBalances[client].token_x;
// @audit It the `order_owner` has enough balance then funds are taken from their balance
if (clients_shares >= shares_to_receive) {
traderBalances[client].token_x -= shares_to_receive;
// ...
}
}
// ...
if (actual_token_x_to_receive > 0) {
_safeTransferFromWithBalanceCheck(
token_x,
msg.sender, // @audit If user's balance doesn't cover the full order, proxy pays the rest
address(this),
actual_token_x_to_receive
);
}
// ...
}
Instead of the proxy covering the full cost of the order, the user’s already existing funds in the OnchainCLOB contract are used first. As an example attack, this vulnerability can be exploited by a malicious user with proxy permissions who can use the placeOrderByProxy(...) function to create spread in the orderbook, and then create orders on behalf of the user that match with their own, moving back and forth within the created spread, to effectively arbitrage funds out of the user and into the malicious proxy account.
Recommendation
Consider modifying the token transfer logic to differentiate between orders placed directly by a user and those placed by a proxy.
Resolution
Fixed in commit 32439ce7. The function placeMarketOrderWithTargetValueByProxy has also been implemented.
6.2 OnchainCLOB markets with non-native token pairs can be halted
The OnchainCLOB contract supports ERC20 token pairs where one of the tokens can be a wrapped native asset (e.g., Wrapped Ether). This is controlled by the storage variable supports_native_eth, which, if set to true, indicates that the market accepts the chain’s native assets as payment. Internally, the native asset is converted into the wrapped ERC20 token during deposits and converted back into the native asset during withdrawals.
Token inflow and outflow for order execution are handled in the _handleTokenTransfer function. It determines the net inbound or outbound transfer amount for each token, performs conversions between wrapped and native assets as needed, and executes the transfers. At the end of the function, a balance check is performed to convert any remaining native assets into wrapped assets. This is shown below:
function _handleTokenTransfer(...) internal {
// ...
// Depositing WETH
if (address(this).balance > 0) {
weth.deposit{value: address(this).balance}();
}
}
There are multiple checks in the contract to ensure that native asset value cannot be transferred into the contract unless a wrapped native asset token is supported. These checks are present in all deposit, order, and fallback receive functions. However, an attacker can bypass these protections by using the selfdestruct opcode to forcefully transfer native assets to the contract. If this is done to a market contract that does not support native assets, the balance check will still execute. Assuming the attacker has forcefully deposited native assets using selfdestruct, the balance will read as non-zero, and the market will attempt to call deposit on the weth address variable. The weth variable is defined below:
IWETH weth = is_token_x_weth ? IWETH(address(token_x)) : IWETH(address(token_y));
A market that does not support native assets would have regular ERC20 tokens for token_x and token_y, so the weth variable would be an address that does not fully support the IWETH interface—particularly the deposit function. With native assets present on the contract, the check will still execute and the deposit call will fail, causing the transaction to revert.
The _handleTokenTransfer function is an internal function used by many other external functions. As a result, if native assets are forcefully sent to a market that does not support them, the following external functions will permanently revert: placeOrder, placeMarketOrderWithValue, claimOrder, and batchClaim. This means that users cannot make, claim, or cancel orders, effectively halting the market.
Recommendation
Consider adding an additional requirement to the balance check to ensure that supports_native_eth is true. This will ensure that the weth.deposit call only executes when the weth token is guaranteed to support the deposit function.
Resolution
Fixed in commit 51067f20.
6.3 ERC20 Permit support
The OnchainCLOB contract supports tokens with the EIP-2612 standard, allowing users to provide signatures for allowances. There are two other notable permit implementations which are not supported by the OnchainCLOB contract: The Uniswap Permit2 and the DAI-style permit.
// EIP-2612
permit(address owner, address spender, uint value, uint deadline, uint8 v, bytes32 r, bytes32 s)
// Permit2
permit(address owner, PermitSingle memory permitSingle, bytes calldata signature)
// DAI-style
permit(address holder, address spender, uint256 nonce, uint256 expiry, bool allowed, uint8 v, bytes32 r, bytes32 s)
Recommendation
No changes are required, this finding simply notes that there may be some permit based tokens which have a different standard to what is supported by the OnchainCLOB contract. When creating a new market for ERC20 tokens, consider their permit style.
Resolution
No changes are required. We support EIP-2612 permits and will add other standards like Permit2 if needed.
6.4 Order claiming allows token transfers and is open by default
There are two functions that can be used to claim an order: claimOrder and batchClaim. The claimOrder function is designed to be called by a user, using the msg.sender as the order owner. The batchClaim function allows an array of addresses to be passed, meaning that the caller does not have to be the order owner in order to claim an order on behalf of another user. batchClaim will call the internal _calculateAndSendOrderPayout function with the transfer_tokens argument hardcoded to true, as shown below:
function batchClaim(address[] memory addresses, ...) external ... {
// ...
for (uint i = 0; i < addresses.length; i++) {
_calculateAndSendOrderPayout(addresses[i], order_ids[i], only_claim, true);
}
}
The OnchainCLOB contract allows for two ways to manage assets when creating and claiming orders. Users can deposit and withdraw tokens on each order to maintain custody of their assets, or, for a more gas-efficient approach which could suit entities placing frequent orders, it is possible to leave tokens in the contract and use internal accounting. By using internal accounting, token transfers are not required, which can save gas costs for these high-frequency traders.
A user can only have their order claimed on their behalf by another address if their claimable status is set to true, which is the default state when their trader ID is created. This makes it possible to claim tokens on behalf of another user and forcefully move the tokens into the user’s account. For traders frequently placing orders and making use of the internal accounting system to save gas, this opens the opportunity to "grief" these power users by forcing them to manually deposit the tokens again, requiring the user to submit token approvals and increasing their gas costs while interacting with the protocol.
Recommendation
Given that the default behavior of allowing orders to be claimable suits general users, consider allowing power users to have granular control over whether their tokens should be transferred or remain in the contract during a claim.
Resolution
User can set desired behavior with function setTraderConfig. We removed function batchClaim, supposed using batchExecute with calls of claimOrder instead. In claimOrder if msg.sender != order_owner contract will ignore provided params.
6.5 Post-only orders can be griefed via front-running
The _placeOrder(...) function allows users to submit post-only orders. These orders are designed to be passive, adding liquidity to the order book without executing against existing orders. The contract enforces this by checking if the order’s price would cross the spread. If it does, the order is considered aggressive, and the function returns without placing it.
An attacker can exploit this mechanism by front-running a user’s post-only order. By observing the transaction in the mempool, the attacker can submit their own small order that shifts the best price on the book just enough to make the victim’s subsequent post-only order appear aggressive.
// ...
if (post_only) {
bool aggressive_trade = false;
if (isAsk) {
// @audit An attacker can front-run to place a bid, making this condition true.
// Same applies for ask
if (price_id <= bidTrie.best_offer()) {
aggressive_trade = true;
}
}
// @audit If front-run, aggressive_trade becomes true.
if (aggressive_trade) {
if (msg.value > 0) {
_sendETH(msg.sender, msg.value);
}
// ...
// @audit The function returns, causing the user's order to fail.
return execution_result;
}
}
// ...
When the victim’s transaction is executed, the aggressive_trade check will pass, causing the function to return early. The user’s order is never placed, their transaction effectively becomes a no-op, and they are forced to waste gas.
Recommendation
Consider documenting the behavior clearly for users intending to use post_only orders.
Resolution
Fixed in commit 793e0b2d.
6.6 Proxy orders refund dust from msg.value to the order owner
When a proxy places an order via placeOrderByProxy(...) and provides native ETH through msg.value, there might be a small amount of "dust" leftover. This occurs because the msg.value may not be a perfect multiple of the required token amount after scaling calculations. The protocol correctly calculates this dust but refunds it to the wrong party.
The _handleTokenTransfer(...) function handles the distribution of funds. It calculates the leftover ETH but sends it to the client address, which, in a proxy transaction, is the order_owner. The funds should be returned to msg.sender, the proxy who supplied the ETH in the first place.
function _handleTokenTransfer(
address client, // @audit This is the `order_owner` in a proxy trade.
// ...
bool withdraw_as_native_eth
) internal {
// ...
if (msg.value > 0) {
if (is_token_x_weth) {
uint128 extra_x = (msg.value / scaling_factor_token_x).toUint128();
uint256 actual_value = _convertToActualTokenXAmount(extra_x);
// ...
// @audit`rounded_value` is the dust from the proxy's msg.value.
uint256 rounded_value = msg.value - actual_value;
// @audit This dust amount is queued to be sent.
actual_token_x_to_send = rounded_value;
} else {
// ... similar logic for token Y
}
}
// ...
// actual erc20 transactions
if (actual_token_x_to_send > 0) {
if (supports_native_eth && is_token_x_weth) {
if (withdraw_as_native_eth) {
// @audit Dust is sent to `client` (the order_owner), not msg.sender (the proxy).
_sendETH(client, actual_token_x_to_send);
} else {
token_x.safeTransfer(client, actual_token_x_to_send);
}
}
// ...
}
// ...
}
This leads to a situation where the proxy consistently loses a small amount of ETH on every native currency trade, while the order_owner unexpectedly receives these small refunds.
Recommendation
Consider updating the logic to correctly refund the dust amount to the supplier of the order.
Resolution
The described behavior is the intended design. The logic is based on the protocol’s expectation that all funds provided to the placeOrderByProxy function originate from the client (order_owner). Therefore, any residual dust from the transaction is correctly refunded to the source of the funds, the order_owner.
6.7 The function getAccumulatedFees returns a scaled value
The function getAccumulatedFees is an external view function intended to provide information about the total fees collected by the protocol. However, the function returns the raw storage value of accumulated_fees, which is a scaled-up integer. It does not scale this value down by the wad factor (1e18) to represent the actual token amount.
function getAccumulatedFees() external view returns (uint256) {
return (accumulated_fees - 1);
}
This is inconsistent with how fees are handled internally. For instance, the _transferFees() function correctly divides the accumulated_fees by wad to get the real amount of fees to be distributed.
function _transferFees() internal {
// @audit The value is correctly scaled down here.
uint256 total_fees = (accumulated_fees - 1) / wad;
accumulated_fees -= total_fees * wad;
// ...
}
As a result, anyone that relies on getAccumulatedFees() will receive a a misleading value, potentially causing incorrect calculations, confusion or misrepresentation of the protocol’s performance.
Recommendation
Consider changing getAcculumatedFees to return the actual accumulated fee amount instead of the scaled amount.
Resolution
Fixed in commit 759a74eb. We have fixed the issue as recommended by returning the actual token amount after proper scaling. However, we also preserved the raw value for transparency and backward compatibility. Both values are now returned to ensure external integrations can properly work with the fee data.
