###### 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
Sign in with Wallet
Connect another wallet