---
# System prepended metadata

title: Ubeswap limit order Audit -- Preliminary Issue Analysis
tags: [Preliminary Report]

---

###### 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

[TOC]


---

## Priviliged Roles
## Findings & Improvement Suggestions
#### 
<html></html>
<style>
    .info {
    background-color:mediumseagreen;
    font-size: 12px;
    color: white; 
    border-radius:4px;  
    padding: 1px 4px;
    font-weight: 500;
    display: inline-block;
    margin: 2px;
    letter-spacing: 0.3px}
</style><style>
    .minor {
    background-color: #698999;
    font-size: 12px;
    color: white; 
    border-radius:4px;  
    padding: 1px 4px;
    font-weight: 500;
    display: inline-block;
    margin: 2px;
    letter-spacing: 0.3px}
</style><style>    
    .medium {
    background-color: #FFCA0F;
    color: #121212;
    font-size: 12px;
    border-radius:4px;  
    padding: 1px 4px;
    font-weight: 500;
    display: inline-block;
    margin: 2px;
    letter-spacing: 0.3px}
</style><style>    
    .major{
    background-color: #FF6B4A;
    color: white;
    font-size: 12px;
    border-radius:4px;  
    padding: 1px 4px;
    font-weight: 500;
    display: inline-block;
    margin: 2px;
    letter-spacing: 0.3px}
</style><style>    
    .critical{
    background-color: #FF0000;
    color: white;
    font-size: 12px;
    border-radius:4px;  
    padding: 1px 4px;
    font-weight: 500;
    display: inline-block;
    margin: 2px;
    letter-spacing: 0.3px}
</style>

<span class='info'>Informational</span><span class='minor'>Minor</span><span class='medium'>Medium</span><span class='major'>Major</span><span class='critical'>Critical</span>
    
|               | Total | Acknowledged | Resolved |
| ------------- | ----- | ------------ | -------- |
| Critical      |   0   |      0       |     0    |
| Major         |   1   |      1       |     1    |
| Medium        |   1   |      1       |     1    |
| Minor         |   1   |      1       |     1    |
| Informational |   3   |      3       |     0    |



<script type="text/javascript"> 
    var i = document.getElementsByClassName('critical');
    document.getElementById('critical').innerHTML; 
</script>

### Critical
none ;)

### Major
1. Unlimited `broadcastOrder()` function calls at `OrderBookWithFee.sol` <span class='major'>Major</span> 
    **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    
1. Reentrancy Risk at `OrderBookWithFee.sol` function `broadcastOrder()`. <span class='medium'>medium</span> 
    **Description**:  `address _notificationTarget` parameter can be passed by users. 
    ```solidity=
    /// @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. 
    ```solidity=
    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
1. Missing reentrancy protection at     `OrderMixin._fillOrderTo()`.  <span class='minor'>minor</span>  
    **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.
    
    ```solidity 
        // 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
1.  `OrderBookWithFee.sol` does not include functionalites in `OrderMixin.sol` and `OrderRFQMixin.sol`. <span class='info'>Informational</span>
    **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.
    
2. Do not transfer reward if the contract has insufficient token amount. <span class='info'>Informational</span>
    **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
    
3.  Informational</span> Duplicate code. <span class='info'>Informational</span>
    **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
    
