tags: `Preliminary Report`

Ubeswap limit order Audit – Preliminary Issue Analysis

Copyright © 2022 by Verilog. All rights reserved.
Mar 20, 2022
by Verilog Audit

This is the preliminary audit report to summarize the most urgent and critical issues found by the Verilog Audit. We will list the major concerns and findings here. Once the issues below are all resolved, we can proceed to a lower-level security and improvement analysis.

Table of Content

Ubeswap limit order Audit – Preliminary Issue Analysis

Priviliged Roles

Findings & Improvement Suggestions

InformationalMinorMediumMajorCritical

	Total	Acknowledged	Resolved
Critical	0	0	0
Major	1	1	1
Medium	1	1	1
Minor	1	1	1
Informational	3	3	0

Critical

none ;)

Major

Unlimited broadcastOrder() function calls at OrderBookWithFee.sol Major
Description: Users can call broadcastOrder() function for unlimited times given they provide correct _order and _signature. Non-users can also observe exisiting _order and _signature, and then perform mimicry attack.
Recommendation: Adding a mapping to check whether this order has been created or not.
Result: Discussed with project team, a mapping to protect re-using order will be considered.

Medium

Reentrancy Risk at OrderBookWithFee.sol function broadcastOrder(). medium
Description: address _notificationTarget parameter can be passed by users.




























/// @notice Logs an order/signature pair. Also executes the order placement fee.
/// @param _order The order to broadcast
/// @param _signature A valid signature of _order
/// @param _notificationTarget A notification target. Disable by providing the 0 address
function broadcastOrder(
    LimitOrderProtocol.Order memory _order,
    bytes calldata _signature,
    address _notificationTarget
) public {
    if (feeRecipient != address(0) && fee > 0) {
        uint256 feeAmount = _order.makingAmount.mul(fee).div(
            PCT_DENOMINATOR
        );
        if (feeAmount > 0) {
            IERC20(_order.makerAsset).safeTransferFrom(
                msg.sender,
                feeRecipient,
                feeAmount
            );
        }

        if (_notificationTarget != address(0)) {
            IOrderNotificationReceiver(_notificationTarget)
                .notifyOrderBroadcasted(_order, msg.sender);
        }
    }
    _broadcastOrder(_order, _signature);
}

Thus, users can pass an attacker contract address such as below to get benefit from the rewards.














contract Attacker is IOrderNotificationReceiver{
    //malicious version
    function notifyOrderBroadcasted(
        LimitOrderProtocol.Order memory _order,
        address _rewardRecipient
    ) public { 
        broadcastOrder(order, sig, address(distributor)); 
        // pass ligit reward distributor's address 
        // to earn profit from reward
        broadcastOrder(order, sig, address(this)); 
        // pass attacker's contract address 
        // to trigger a new reenter stack.
    }
}

**Call stack presentation:**

    broadcastOrder(Attacker)
    └─ Attacker.notifyOrderBroadcasted()
       ├─ broadcastOrder(Distributor) (Line 7)
       │  └─Distributor.notifyOrderBroadcasted() <= earn reward
       └─ broadcastOrder(Attacker) (Line 10)
          └─ Attacker.notifyOrderBroadcasted()
             ├─ broadcastOrder(Distributor) (Line 7)
             │  └─Distributor.notifyOrderBroadcasted() <= earn reward
             └─ broadcastOrder(Attacker) (Line 10)
                ... loop

Recommendation: Protect broadcastOrder() with nonReentrant modifier.
Result: Discussed with project team, nonReentrancy protection will be added, and they have no intention of giving out higher reward than fee.

Minor

Missing reentrancy protection at OrderMixin._fillOrderTo(). minor
Description: missing reentrancy protection at OrderMixin._fillOrderTo(). The function _fillOrderTo() transfers asset from maker to target first. Then have a function call with data passed by msg.sender. It calls the function notifyFillOrder of the address decoded from the data, which gives oppotunities to the reentrancy actions.

    // Maker => Taker
    _makeCall(
        order.makerAsset,
        abi.encodePacked(
            IERC20.transferFrom.selector,
            uint256(uint160(order.maker)),
            uint256(uint160(target)),
            orderAmounts.makingAmount,
            order.makerAssetData
        )
    );

    // Handle external extraInteraction
    if (extraInteraction.length >= 20) {
        // proceed only if interaction length is enough to store address
        (address interactionTarget, bytes memory interactionData) = extraInteraction.decodeTargetAndCalldata();
        InteractiveNotificationReceiver(interactionTarget).notifyFillOrder(
            msg.sender, order.makerAsset, order.takerAsset, orderAmounts.makingAmount, orderAmounts.takingAmount, interactionData
        );
    }
    
    // Taker => Maker
    _makeCall(
        order.takerAsset,
        abi.encodePacked(
            IERC20.transferFrom.selector,
            uint256(uint160(msg.sender)),
            uint256(uint160(order.receiver == address(0) ? order.maker : order.receiver)),
            orderAmounts.takingAmount,
            order.takerAssetData
        )
    );

Recommendation: transfer msg.sender's token to maker first or add a nonReentrant modifier to protect this function.
Result: Acknowledged without action due to low risk

Informational

OrderBookWithFee.sol does not include functionalites in OrderMixin.sol and OrderRFQMixin.sol. Informational
Description: The broadcast and fee payment mechanism is stand-alone and does not include OrderFill() functions. In this case, users can bypass the fee payment and use order book functions solely.
Recommendation: Discussion needed
Result: Discussed with project team, and this design is made intentionally.
Do not transfer reward if the contract has insufficient token amount. Informational
Description: In OrderBookRewardDistributor.sol L83, notifyOrderBroadcasted() function does not transfer reward if the contract has insufficient token amount.
Recommendation: Transfer all the remaining tokens as reward.
Result: Acknowledged without action
Informational Duplicate code. Informational
Description: In Permitable.sol, the _permit() and _permitMemory() functions are duplicates. Permit memory is never used.
Recommendation: Replace _permitMemory() with _permit(). Permit calldata is more gas-efficient.
Result: Acknowledged without action due to low risk

tags: Preliminary Report

Ubeswap limit order Audit – Preliminary Issue Analysis

Table of Content

Priviliged Roles

Findings & Improvement Suggestions

Critical

Major

Medium

Minor

Informational

Read more

BendDAO Audit

Vesta Finance Audit

YuzuSwap Audit

Fountain Protocol Audit

tags: `Preliminary Report`