tags: Final Report

Ubeswap Audit

Copyright © 2022 by Verilog Solutions. All rights reserved.
March 20, 2022
by Verilog Solutions

Ubeswap-COVER

This report presents our engineering engagement with Ubeswap, a mobile-first DeFi exchange on the Celo network. Ubeswap provides decentralized exchange and automated market marker protocol for Celo assets. Ubeswap is recently adding new features including limit order.

Table of Content

Project Summary

Ubeswap is a decentralized exchange on the Celo blockchain, utilizing a constant function market-making (CFMM) algorithm. Some parts of Ubeswap’s source code are based on Uniswap, Sushiswap, Synthetix, and Compound. Verilog is invited to audit Ubeswap’s latest update, which adds new features including limit order.

Service Scope

Our review focused on the bl/uob branch, specifically, commit hash b277e09610b85b3adc960a7730d71a47c7a95634.

Our auditing service for Ubeswap includes the following two stages:

  1. Pre-Audit Consulting Service

    As a part of the pre-audit service, the Verilog team worked closely with the Ubeswap development team to discuss potential vulnerability and smart contract development best practices in a timely fashion. Verilog team is very appreciative of establishing an efficient and effective communication channel with the Ubeswap team, as new findings are often exchanged promptly and fixes were deployed quickly, during the preliminary report stage.

  2. Audit Service

    The Verilog team conducted a thorough study of the Ubeswap code, with the Ubeswap architecture graph presented below in the Ubeswap Architecture section. The list of findings, along with the severity and solution, is available under the section Findings & Improvement Suggestions.

Ubeswap Architecture

Ubeswap-Architecture

These are the major smart contracts in Ubeswap:

Privileged Roles

  1. OrderBookRewardDistributor.sol:
    a. Owner can changeRewardRate(), changeRewardCurrency, rescueERC20(), addToWhitelist(), removeFromWhitelist().
    b. only whitelisted address can interact with notifyOrderBroadcasted()

  2. OrderBookWithFee.sol:
    Owner can changeFee(), changeFeeRecipient().

Findings & Improvement Suggestions

InformationalMinorMediumMajorCritical

Total Acknowledged Resolved
Critical 0 0 0
Major 0 0 0
Medium 0 0 0
Minor 3 3 3
Informational 4 4 2

Critical

none ;)

Major

none ;)

Medium

none ;)

Minor

  1. Unlimited broadcastOrder() function calls at OrderBookWithFee.sol minor
    Description: Users can call broadcastOrder() function for unlimited times given they provide correct _order and _signature. Non-users can also observe existing _order and _signature, and then perform mimicry attacks.
    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 is considered. Fix in commit 533e9b7522f085531aaba32a4c62721e557ec425.

  2. Reentrancy risk at OrderBookWithFee.sol function broadcastOrder(). minor
    Description: Arbitrary contract 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 receive benefits 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 the project team, ReentrancyGuard protection is added. The project team stated they have no intention of giving out a higher reward than the fee. Fix in commit 533e9b7522f085531aaba32a4c62721e557ec425.

  3. Missing reentrancy protection at OrderMixin._fillOrderTo(). minor
    Description: Missing reentrancy protection at OrderMixin._fillOrderTo(). The function _fillOrderTo() transfers asset from maker to taker first, followed by a function call with data passed by msg.sender. It calls the function notifyFillOrder of the address decoded from the data, which allows reentrancy attacks.

    // 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 the maker first or add a nonReentrant modifier to protect this function.
    Result: Acknowledged without action due to low risk.

Informational

  1. 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() function. In this case, users can bypass the fee payment and use orderbook functions solely.
    Recommendation: Discussion needed.
    Result: Discussed with project team, and this design is made intentionally.

  2. 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: Fix in commit 533e9b7522f085531aaba32a4c62721e557ec425.

  3. 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.

  4. notificationTarget cannot be notified if fee and feeRecipient not been set. Informational

    


























    /// @notice Broadcasts a signed order. Also charges 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 nonReentrant {
    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);
}

    Recommendation: This change is based on business logic, if there is a need to receive data updates when fee and feeRecipient are not set, then move this if condition outside the outer if clause.
    Result: Fix in commit 533e9b7522f085531aaba32a4c62721e557ec425.

Disclaimer

Verilog receives compensation from one or more clients for performing the smart contract and auditing analysis contained in these reports. The report created is solely for Clients and published with their consent. As such, the scope of our audit is limited to a review of code, and only the code we note as being within the scope of our audit detailed in this report. It is important to note that the Solidity code itself presents unique and unquantifiable risks since the Solidity language itself remains under current development and is subject to unknown risks and flaws. Our sole goal is to help reduce the attack vectors and the high level of variance associated with utilizing new and consistently changing technologies. Thus, Verilog in no way claims any guarantee of security or functionality of the technology we agree to analyze.

In addition, Verilog reports do not provide any indication of the technologies proprietors, business, business model, or legal compliance. As such, reports do not provide investment advice and should not be used to make decisions about investment or involvement with any particular project. Verilog has the right to distribute the Report through other means, including via Verilog publications and other distributions. Verilog makes the reports available to parties other than the Clients (i.e., “third parties”) – on its website in hopes that it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.