tags: `Final Report`

WOOFi Swap Audit

Copyright © 2021 by Verilog Solutions. All rights reserved.
October 17, 2021
by Verilog Solutions

Image Not Showing Possible Reasons

The image was uploaded to a note which you don't have access to
The note which the image was originally uploaded to has been deleted

This report presents our engineering engagement with WOO Network on their first DeFi protocol WOOFi Swap. Unlike popular Automated Market Making (AMM) or Proactive Market Making (PMM), WOOFi Swap’s Sythethetic Proactive Market Making (sPMM) is a brand new market-making algorithm that can successfully solve the slippage issue in Decentralized Exchange (DEX) by simulating order book structure in Centralized Exchange (CEX).

Table of Content

WOOFi Swap Audit

Project Summary

WOOFi Swap is built and designed by the WOO Network to improve on-chain liquidity and capital efficiency. Under the current EVM development environment, market making is a very challenging problem that requires the design of the system to make trade-offs on various aspects (e.g slippage, market depth) for operation. Due to their abundant experience in centralized exchange market making, WOO Network team has the insight to propose a solution for DEX by using the WOO Network oracle (a.k.a. Wooracle) to simulate the market depth of the CEX on a DEX. Similar to PMM, the Wooracle will feed the DEX with the latest price traded on WOO CEX, the DEX will also load the Chainlink oracle price feed as a cross-reference to make sure the price feed is correct and decentralized. Besides the price feed, WOO Network team tunes the parameters in their sPMM algorithm to enable their DEX to successfully simulate the market depth and slippage for Defi users. Those parameters are calculated off-chain by the quantitative analysis team and tested by the WOO Network team off-chain. In the current smart contract, WOOFi Swap does not allow users to create trading pairs and provide liquidity. Instead, the tokens in DEX are initially deposited by the dedicated market maker (i.e. Kronos Research). In general, the idea behind WOOFi swap is to provide users a closer performance to CEX slippage trading on DEX. Indubitably, WOO Network innovation with sPMM is a valuable differentiator since no other exchanges offer this unique feature in the current market. If executed thoughtfully, Verilog Solutions believes WOOFi Swap has the potential to attract major market share through the sheer product-market fit of their low slippage offerings.

Service Scope

Our review focused on the main branch, specifically, commit hash 57d7f2eb2230360a009461051235ad3ecf16f707.

Our auditing service with WOO Network will include the following two stages:

Pre-Audit Consulting Service
Audit Service

Pre-Audit Consultancy Service

Initially, Verilog Solutions conducted due diligence and inspections of the product's current state with our full team of smart contract auditors. Verilog Solutions then developed a high-level understanding of the project and its required testing methods. After, we proposed to change from the Truffle framework to the Hardhat framework which has better testing support and community recognition. Indeed, some testing methods can only be done by using hardhat which is another strong reason why the framework shift is necessary.

Besides the framework, our team provides many smart contract development conventions. For example, Verilog Solutions has proposed new insights on how to refactor smart contracts as well as their structures. With long-term growth in mind, our proposal helps future researchers and developers to continue studying and proposing new models for WOOFi Swap.
Smart Contract Auditing Service

In total, the audit service between WOOFi Swap and the Verilog Solutions engineering team lasted 2 weeks. The Verilog Solutions engineering team studied the entire project using a detailed-oriented approach to capture fundamentals and proposed meaningful changes to existing code. Details can be found under Findings & Improvement Suggestions.

WOOFi Swap Architecture

WOOFi Swap Architecture

There are four major smart contracts in WOOFi Swap:

Wooracle.sol
WooPP.sol
WooRouter.sol
WooGuardian.sol

Wooracle.sol

Wooracle.sol is an oracle contract managed and developed by the WOO Network team. The WOO Network team uploads their CEX data to the oracle contract to get the latest trading price, middle price, and market spread information on CEX.
WooPP.sol

WooPP.sol is the basic contract that handles the logic operation of DEX, including setting the token info, calculating slippage, calculating the exchange amount, and so on. This contract will store all tokens that are supported by trading on the WOOFi Swap. In this contract it defines quote token and base token:
- Quote Token: the reference token in the contract, only have one.
- Base Token: can be added by the strategist from the WOO Network team and can be more than one.
WooRouter.sol

WooRouter.sol is the router contract. The general public will only need to approve this contract to spend their ERC20. Similar to Uniswap, approval is required to interact with any trading pair on this contract. The router contract will interact with the lower layer WooPP.sol to execute the sell logic and send back the users desired amount of tokens. Router contracts can also interact with 3rd party DEX when the market-making strategy is on pause or the asset is not available for direct exchange on WOOFi Swap.
WooGuardian.sol

WooGuardian.sol is the smart contract that provides the tolerance threshold of token prices given by the oracle. For the swap amounts and prices given by the oracle, the functions (i.e., checkSwapAmount() and checkSwapPrice()) in this smart contract are used to check if the swapping amounts and swapping prices are within the thresholds or not.

WOOFi Swap Logic

In WooPP.sol, the project team will set their desired Quote Token (e.g USDC), and multiple Base Tokens (e.g wBTC, wETH …). There are two routines as indicated in the above pictures. 1. From base token to quote token 2. From one base token to another base token. In short, if a user wants to swap from wBTC (Base Token) to USDC (Quote Token), the WOOFi Swap will swap these two directly. If a user wants to swap from wBTC (Base Token) to wETH (Base Token), the WOOFi Swap will swap wBTC to quote token and then from quote token to wETH. However, when the market-making strategy is put on pause due to various reasons (e.g. maintenance) or the asset is not available, WOOFi Swap will redirect the trade to 3rd party DEX.

The WooRouter.sol UML (after pre-audit & audit):

WooRouter.sol UML

The WooPP.sol UML (after pre-audit & audit):

WooPP.sol UML

The Wooracle.sol UML (after pre-audit & audit):

Wooracle UML

The WooGuardian.sol UML (after pre-audit & audit):

WooGuardian UML

Privilege Roles

There are two main privilege roles: owner and strategist.

owner
- manage the ownership of WooRouter, WooPP, Wooracle
- set new pool for WooRouter
- add and remove whitelist address
- pause and unpause the following functions in WooPP
  - querySellBase
  - querySellQuote
  - sellBase
  - sellQuote
- withdraw ERC20 tokens from WooRouter and WooPP
- set all important variables in Wooracle
strategist(including owner)
- add and remove strategist
- set price oracles for WooPP
- set rewardManager
- add and remove baseToken at WooPP
- tune parameters of baseToken and quoteToken
NOTE: Also there is a rewardManager contract, which will receive the earned lp fee when users interact with the protocol.

Given that the WOO Network team has control over these privilege roles, we suggested for them to transfer the owner to a multisig wallet controlled by the WOO Network team.

Since users will not be able to join as liquidity providers, the current version allows the WOO Network team to deposit assets, set base tokens, and shoulder potential risks of being arbitraged. In the smart contract, there is a price feed comparison between Chainlink Oracle and Wooracle. Verilog Solutions auditors believe that if WOO Network team can properly manage the price feed oracle and gnosis wallet to tune the parameters of the DEX, users will have a smooth experience reaping benefits similar to what is offered through CEX trading with a better slippage.

Findings & Improvement Suggestions

InformationalMinorMediumMajorCritical

	Total	Acknowledged	Resolved
Critical	2	2	2
Major	0	0	0
Medium	1	1	0
Minor	12	12	12
Informational	4	4	4

Critical

<address>.transfer is used for transferring eth. (WooRouter: L122, L141, L259) Critical
```
if (isToETH) {
    realToAmount = pool.sellQuote(toToken, fromAmount, minToAmount, address(this), address(this), rebateTo);
    IWETH(_WETH_ADDRESS_).withdraw(realToAmount);
    to.transfer(realToAmount);
} else {
    realToAmount = pool.sellQuote(toToken, fromAmount, minToAmount, address(this), to, rebateTo);
}
```
Description: Transfer forwards a fixed amount of gas(e.g. 2300). Gas cost is subject to change and the smart contract should not depend on any particular gas cost. It is important to note that sending Ether can also fail because the execution of the recipient contract requires more than the allotted amount of gas.

Recommendation: Use call() with the reentrancy guard and check the return state.

Result: Resolved.
Avoid using selfdestruct(WooRouter: L284-L286)Critical
```
function destroy() external onlyOwner {
    selfdestruct(msg.sender);
}
```
Description: WOOFi Swap was originally designed to use selfdestruct function to disable the WooRouter. However, even if a contract is removed by selfdestruct, it is still part of the history of the blockchain and probably retained by most Ethereum nodes. So using selfdestruct is not the same as deleting data from a hard disk. Besides, even if a contract’s code does not contain a call to selfdestruct, it can still perform that operation using delegatecall or callcode.

Recommendation: Pause or disable the functions in the contract instead of using selfdestruct. When users are sending eth or tokens to the contract, their transactions will be reverted instead of losing their money.

Result: Resolved in commit b042a36301acff42e76aaa1ff12c8f5e4e8a353b.

Medium

Check actual token amount received after transferFrom (WooPP: L343, L373)Medium
```
IERC20(baseToken).safeTransferFrom(from, address(this), baseAmount);
```
```
IERC20(quoteToken).safeTransferFrom(from, address(this), quoteAmount.add(lpFee));
```
Description: All function logic related to deposit, should have a secondary check after transferFrom. Some tokens will deduct some fees when transferring. This case might not happen for most tokens, but it is a safer practice to add another layer of protection to the design.

Recommendation: Have a secondary check after transferFrom.

Result: Talked with WOOFi Swap team, and confirmed only the mainstream tokens (e.g. btc, eth, bnb, etc) will be added. The WOOFi team will further make sure all the base/quote tokens are non-burnable tokens in transfer functions when added into WooPP.

Minor

The quoteToken is not immutable in WooPP (WooPP: L72)Minor
```
address public quoteToken;
```
Description: As a variable that does not change after initialization in the constructor, the immutable keyword should be used to protect the variable from being modified after initialization.

Recommendation: Use the immutable keyword.

Result: Resolved.
swap function: Adding the requirement for non-zero address to fromToken, toToken, to, rebateTo (WooRouter: L97-L103) Minor
```
function swap(
    address fromToken,
    address toToken,
    uint256 fromAmount,
    uint256 minToAmount,
    address payable to,
    address rebateTo
) external payable returns (uint256 realToAmount) {...}
```
Description: In WooRouter.sol, adding require statement for the input arguments fromToken, toToken, to, rebateTo.

Recommendation: Adding the requirement for non-zero address to fromToken, toToken, to, rebateTo.

Result: Resolved in commit. 82d25b10cd7c6358ab7cf22296c01b58840ecced.
externalSwap function: Adding the requirement for non-zero address to approveTarget, swapTarget, fromToken, toToken, to; (WooRouter: L201-L207) Minor
```
function externalSwap(
    address approveTarget,
    address swapTarget,
    address fromToken,
    address toToken,
    uint256 fromAmount,
    address payable to,
    bytes calldata data
) external payable nonReentrant {...}
```
Description: In WooRouter.sol, no non-zero address check for arguments approveTarget, swapTarget, fromToken, toToken, to of function externalSwap.

Recommendation: Including the requirement for non-zero address to the arguments approveTarget, swapTarget, fromToken, toToken, to of function externalSwap.

Result: Resolved in commit. 82d25b10cd7c6358ab7cf22296c01b58840ecced.
setWhiteListed: Adding the requirement for non-zero address to target; (WooRoute: L274)Minor
```
function setWhitelisted(address target, bool whitelisted) external onlyOwner {
    isWhitelisted[target] = whitelisted;
}
```
Description: In WooRouter.sol, no non-zero address check for argument target of function setWhiteListed.

Recommendation: Adding the requirement for non-zero address to argument target of function setWhiteListed.

Result: Resolved in commit 82d25b10cd7c6358ab7cf22296c01b58840ecced.
Add non-zero address check for address arguments in other solidity files including libraries. (WooPP: L143, L248-249, L287, L326, L329-331, L356, L359-361, L385, L398, L411, L421, L437, L443, L498, L523, L529-530, L537)Minor
```
function autoUpdate(
    address baseToken,
    TokenInfo memory baseInfo,
    TokenInfo memory quoteInfo
) internal view {...)
```
Description: No non-zero address check for address argument.

Recommendation: Add zero address check for address argument in other solidity files including libraries.

Result: Resolved in commit. 82d25b10cd7c6358ab7cf22296c01b58840ecced.
Use of actual uint type instead of uint256 for arguments newThreshold, newLpFeeRate and R in tuneParameters (WooPP: L499-L501)Minor

Description: uint256 is used for the arguments type to input parameters newThreshold, newLpFeeRate and R in tuneParameters and convert to their actual uint type inside the function.

Recommendation: Use the actual uint type instead.
Result: Not Resolved.
WooPP_proxy.sol is a flattened file (WooPP_proxy.sol)Minor

Description: Since the proxy contract of WooPP.sol, the file was flattened, we suggested the engineering team of Wootrade to rearrange it.

Recommendation: Refactor the WooPP_proxy.sol.

Result: WOOFi team decided no longer to use proxy contracts as an upgradeable method. So this issue has been resolved. Besides, all liquidity is provided by WOOFi team themselves, so WooPP.sol can be replaced and changed to the new one without a need for an upgradable contract.
The quoteToken is not immutable in WooPP (WooPP: L72)Minor
```
address public quoteToken;
```
Description: As a variable that does not change after initialization in the constructor, the immutable keyword should be used to protect the variable from being modified after initialization.

Recommendation: Use the immutable keyword.

Result: Resolved.
Add require for msg.sender at the receive function in WooRouter(WooRouter: L79)Minor
```
receive() external payable {}
```
Description: No require at receive function which means anyone can send eth to the contract address and the eth cannot be rescued. Adding require for the msg.senderto only allow WETH contract and whitlisted contracts to send eth.

Recommendation: Add require for msg.sender at the receive function.
Result: Resolved.
Require postBalance to be bigger than preBalance at externalSwap (WooRouter: L201-L227)Minor

Description: There is a subtraction postBalance.sub(preBalance) at the event emit of externalSwap. The function will be reverted if an overflow happens. Adding a requirepostBalance > preBalanace is recommended.

Recommendation: Adding a requirepostBalance > preBalanace.
Result: Resolved.
internal functions can be changed to private functions(WooPP: L146, L172, L185, L198, L211, L224, L252, L291, L547)Minor

NOTE: Gas Saving Recommendation
```
 function autoUpdate(    
    address baseToken,
    TokenInfo memory baseInfo,
    TokenInfo memory quoteInfo
 ) internal view {...}
```
Description: internal functions can be accessed within the contract and the contracts deriving from. private functions instead can only be accessed within the contract. In WooPP.sol, some functions are designed to serve the computation purposes within the WooPP.sol. Thus, private function is recommended.

Recommendation: In WooPP.sol, change internal functions to private functions.
```
 function autoUpdate(    
    address baseToken,
    TokenInfo memory baseInfo,
    TokenInfo memory quoteInfo
 ) private view {...}
```
Result: Resolved.

public functions can be changed to external functions (WooPP: L448,L478,L502)Minor

NOTE: Gas Saving Recommendation

 function addBaseToken(
    address baseToken,
    uint256 threshold,
    uint256 lpFeeRate,
    uint256 R,
    address chainlinkRefOracle
 ) public nonReentrant onlyStrategist { .... }

Description: Public functions are part of the contract interface and can be either called internally or via messages. external functions are part of the contract interface, which means they can be called from other contracts and via transactions. Besides, public and external differ in terms of gas usage. The former uses more than the latter when applied to large arrays of data.

Recommendation: Change the function above from public to external to save gas.

function addBaseToken(
    address baseToken,
    uint256 threshold,
    uint256 lpFeeRate,
    uint256 R,
    address chainlinkRefOracle
 ) external nonReentrant onlyStrategist { .... }

Result: Resolved.

Informational

No indexed parameters in the events defined in WooPP and WooRouter (WooRouter: L67, L77; WooPP: L61)Informational
```
event WooRouterSwap(
    SwapType swapType,
    address fromToken,
    address toToken,
    uint256 fromAmount,
    uint256 toAmount,
    address from,
    address to
);
```
Description: The indexed parameters can be used as filters or queried for the logged events. There are no indexed parameters in events in WooPP.

Recommendation: Add indexed keyword to parameters in the events.

Result: Resolved.
Require message reformat (WooRouter: L82, L83, L90, L111, L236, L238, L245, L249; WooPP: L66, L110, L111, L112, L126, L149, L150, L258, L297, L334, L342, L364, L372, L387, L395, L400, L408, L416, L423, L431, L449-452, L455, L469, L480, L503-505, L508, L551, L553, L557)Informational
```
constructor(
    address _quoteToken, 
    address _pool
) public {
    require(_quoteToken != address(0), 'INVALID_QUOTE');
    require(address(_pool) != address(0), 'Pool address cannot be 0');
    quoteToken = _quoteToken;
    pool = IWooPP(_pool);
    emit PoolChanged(_pool);
}
```
Description: In WooRouter.sol / WooPP.sol, the required message should be clearly distinguished and have a uniform format. The format below is what we suggest instead:
```
  WooRouter: ZERO ADDRESS
     ｜		|
  Filename : capitalized warning message
```
This method allows engineers to clearly distinguish which module in the architecture is causing the problem.

Recommendation: Change WooRouter.sol and WooPP.sol require message to the above format.

Result: Resolved in commit 82d25b10cd7c6358ab7cf22296c01b58840ecced.
Create interface for WooPP and WooRouter.

Description:interface provides a clear structure of the code and makes it easy for others to interact with the contract

Recommendation: Creating interface for WooPP and WooRouter.
Result: Resolved.

Using delete in removeBaseToken (WooPP: L478)Informational

function removeBaseToken(
    address baseToken
) public nonReentrant onlyStrategist {
    TokenInfo memory info = tokenInfo[baseToken];
    require(info.isValid, 'TOKEN_DOES_NOT_EXIST');

    info.reserve = 0;
    info.threshold = 0;
    info.lastResetTimestamp = 0;
    info.lpFeeRate = 0;
    info.R = 0;
    info.target = 0;
    info.isValid = false;
    info.chainlinkRefOracle = address(0);
    info.refPriceFixCoeff = 0;

    tokenInfo[baseToken] = info;
    emit ParametersUpdated(baseToken, 0, 0, 0);
    emit ChainlinkRefOracleUpdated(baseToken, address(0));
}

Description: WooPP.removeBaseToken is designed to remove stored baseToken. The struct value of baseToken is reset to zero here.
Recommendation: There are too many members inside the struct and some might be left out when resetting those values. delete is recommended to reset the struct values here. Replace the above code with the below format:

delete tokenInfo[baseToken];

Result: Resolved.

Disclaimer

Verilog Solutions 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 Solutions in no way claims any guarantee of security or functionality of the technology we agree to analyze.

In addition, Verilog Solutions 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 Solutions has the right to distribute the Report through other means, including via Verilog Solutions publications and other distributions. Verilog Solutions 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.

tags: Final Report