###### tags: `Final Report`
Ubeswap Audit
===
> Copyright © 2022 by Verilog Solutions. All rights reserved.
> March 20, 2022
> by **Verilog Solutions**
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
[TOC]
---
## 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](https://github.com/Ubeswap/limit-order-protocol/tree/bl/uob), specifically, commit hash [**b277e09610b85b3adc960a7730d71a47c7a95634**](https://github.com/Ubeswap/limit-order-protocol/tree/b277e09610b85b3adc960a7730d71a47c7a95634).
Our auditing service for Ubeswap includes the following two stages:
- Pre-Audit Consulting Service
- Audit Service
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.
3. **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**](#Findings-amp-Improvement-Suggestions).
___
## Ubeswap Architecture
These are the major smart contracts in Ubeswap:
- `LimitOrderProtocol.sol`: Core contract of limit order protocol v2, borrowed some idea from 1inch
- `OrderBook.sol`: Internal base order book
- `OrderBookRewardDistributor`: A permissioned reward distribution module to accompany an order book
- `OrderBookWithFee.sol`: Public order book that accepts fees
- `OrderMixin.sol`: Regular limit order mixin
- `OrderRFQBook.sol`: Internal order book for `OrderRFQ`
- `OrderRFQMixin.sol`: RFQ limit order mixin
- `PublicOrderRFQBook.sol`: Public order book for `OrderRFQ`
___
## 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
####
<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 | 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` <span class='minor'>minor</span>
**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](https://github.com/Ubeswap/limit-order-protocol/commit/533e9b7522f085531aaba32a4c62721e557ec425).
2. Reentrancy risk at `OrderBookWithFee.sol` function `broadcastOrder()`. <span class='minor'>minor</span>
**Description**: Arbitrary contract `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 receive benefits 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 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](https://github.com/Ubeswap/limit-order-protocol/commit/533e9b7522f085531aaba32a4c62721e557ec425).
3. 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 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.
```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 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`. <span class='info'>Informational</span>
**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. <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**: Fix in commit [533e9b7522f085531aaba32a4c62721e557ec425](https://github.com/Ubeswap/limit-order-protocol/commit/533e9b7522f085531aaba32a4c62721e557ec425).
3. 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.
4. `notificationTarget` cannot be notified if `fee` and `feeRecipient` not been set. <span class='info'>Informational</span>
```solidity=
/// @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](https://github.com/Ubeswap/limit-order-protocol/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.
Sign in with Wallet
Connect another wallet