Final Report
Copyright © 2021 by Verilog Solutions. All rights reserved.
October 17, 2021
by Verilog Solutions
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).
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.
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 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.
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:
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.
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):
The Wooracle.sol
UML (after pre-audit & audit):
The WooGuardian.sol
UML (after pre-audit & audit):
There are two main privilege roles: owner
and strategist
.
owner
querySellBase
querySellQuote
sellBase
sellQuote
strategist
(including owner
)
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.
InformationalMinorMediumMajorCritical
Total | Acknowledged | Resolved | |
---|---|---|---|
Critical | 2 | 2 | 2 |
Major | 0 | 0 | 0 |
Medium | 1 | 1 | 0 |
Minor | 12 | 12 | 12 |
Informational | 4 | 4 | 4 |
<address>.transfer
is used for transferring eth. (WooRouter
: L122, L141, L259) Critical
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
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.
Check actual token amount received after transferFrom
(WooPP: L343, L373)Medium
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.
The quoteToken
is not immutable in WooPP
(WooPP
: L72)Minor
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
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
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
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
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
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
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.sender
to 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
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.
Result: Resolved.
public
functions can be changed to external
functions (WooPP
: L448,L478,L502)Minor
NOTE: Gas Saving Recommendation
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.
Result: Resolved.
No indexed
parameters in the events defined in WooPP
and WooRouter
(WooRouter
: L67, L77; WooPP
: L61)Informational
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
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:
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
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:
Result: Resolved.
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.