WOOFi Swap Audit Report

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

• Pre-Audit Consulting Service

• Audit Service

1. 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.

2. Smart Contract Auditing Service

   In total, the audit service between WOOFi Swap and the Verilog Solutions engineering team lasted two 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.

• Code Assessment
  • We evaluate the overall quality of the code and comments as well as the architecture of the repository.
  • We help the project dev team improve the overall quality of the repository by providing suggestions on refactorization to follow the best practice of Web3 software engineering.

• Code Logic Analysis
  • We dive into the data structures and algorithms in the repository and provide suggestions to improve the data structures and algorithms for the lower time and space complexities.
  • We analyze the hierarchy among multiple modules and the relations among the source code files in the repository and provide suggestions to improve the code architecture with better readability, reusability, and extensibility.

• Business Logic Analysis
  • We study the technical whitepaper and other documents of the project and compare its specification with the functionality implemented in the code for any potential mismatch between them.
  • We analyze the risks and potential vulnerabilities in the business logic and make suggestions to improve the robustness of the project.

• Access Control Analysis
  • We perform a comprehensive assessment of the special roles of the project, including their authorities and privileges.
  • We provide suggestions regarding the best practice of privilege role management according to the standard operating procedures (SOP).

• Off-Chain Components Analysis
  • We analyze the off-chain modules that are interacting with the on-chain functionalities and provide suggestions according to the SOP.
  • We conduct a comprehensive investigation for potential risks and hacks that may happen on the off-chain components and provide suggestions for patches.

Our auditing for WOOFi Swap the repository:

• https://github.com/woonetwork/woofi_swap_smart_contracts:

  commit hash 57d7f2eb2230360a009461051235ad3ecf16f707

  contains the smart contracts source code WooRouter.sol, WooPP.sol, Wooracle.sol and WooGuardian.sol.

Wooracle.solis 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 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 has one;

• Base Token: can be added by the strategist from the WOO Network team and can be more than one.

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, etc.). There are two routines as indicated in the above picture:

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 third-party DEX.

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

WooPP.sol UML

Similar to Uniswap, the router contract will interact with the lower layer WooPP.solto execute the logic.

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

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

WooRouter.sol UML

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.

1. WooRouter.sol: <address>.transfer() function is used for transferring ETH.

   Severity	High
   Source	contracts/WooRouter.sol: #L122 contracts/WooRouter.sol: #L141 contracts/WooRouter.sol: #L259
   Status	Resolved.

   • Description

     In the lines specified, the transfer() function is used to send ETH. It is well known that using this function for transferring ETH has some limitations.

     transfer() forwards a gas amount that has a limit to the transaction (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 ETH can also fail because the execution of the recipient contract requires more than the allotted amount of gas.

     If the amount of gas that needs to be sent in order to complete the transaction is greater than the limit imposed by the transfer() function, the transaction will revert, therefore not performing the logic intended by the method.

   • Exploit Scenario

     Assuming an external contract calls the swap() or _generalTransfer() (both contain this vulnerability) and has some extra functionality since the amount of gas forwarded to the called smart contract is very low, the called smart contract can easily run out of gas. This would revert the transaction and make payment impossible.

     Even if the receiving contract is designed carefully not to exceed the gas limit or the call is made from an EOA, future changes in the gas costs for some opcodes can break this design and cause transactions to be reverted, therefore making interactions with these functions no longer plausible.

   • Recommendations

     Use call() with the reentrancy guard and check the return state. This is the preferred way to send Ether since there are no gas limitations in the transactions.

   • Results

     Resolved. The team adopted the recommendation.

2. WooRouter.sol: Avoid using selfdestruct().

   Severity	High
   Source	contracts/WooRouter.sol: #L284-L286
   Status	Resolved.

   • Description

     selfdestruct() in destroy() function should be avoided since there are better ways to disable contracts and it could lead to users losing money.

     WOOFi Swap was originally designed to use selfdestruct() function to disable the WooRouter.sol. 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. Besides, even if a contract’s code does not contain a call to selfdestruct(), it can still perform that operation using delegatecall() or callcode(). Therefore, using selfdestruct() is not the same as deleting data directly from a hard disk. If the owner calls destroy() function which inherits openzeppelin’s onlyOwner(), then the contract will be destroyed but it will still remain a part of the blockchain.

     This way, if a user doesn’t realize that the contract has been destroyed via selfdestruct() and transfers eth to that address, the amount of token will be completely lost since the transaction will not be reverted.

   • Exploit Scenario

     Access control for destruct() that contains selfdestruct() functionality does not change the implications of using it. Privileged users of the multi-sig wallet can seize funds and destroy the contract. Additionally, future transferred tokens from users to the address of the destroyed contract will be lost forever.

   • Recommendations

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

   • Results

     Resolved.

1. WooPP.sol: Check amount received after transferFrom().

   Severity	Medium
   Source	contracts/WooPP.sol: #L343 contracts/WooPP.sol: #L373
   Status	Acknowledged.

   • Description

     Double-checking that the amount of deposited tokens to a certain address • is correct is good practice and can avoid deducting some unexpected fees from the intended transfer transaction.

     All function logic related to deposit should have a secondary check after transferFrom() to make sure that the actual amount transferred is actually received. This happens two times while performing the logic in contract WooPP.sol, specifically when transferring basetoken and quotetoken.

     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.

   • Exploit Scenario

     The condition that needs to be fulfilled for this to happen is that some token that applies fees when using transferFrom() should be added as a baseToken or used as the quoteToken. If someone tries to swap() DGX or some other token that applies transfer fees when using transferFrom(), then the router contract will get a smaller amount than the specified in the call.

   • Recommendations

     Have a secondary check after transferFrom().

   • Results

     Acknowledged. Talked with the 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.sol.

1. WooPP.sol:The quoteToken variable is not immutable.

   Severity	Low
   Source	contracts/WooPP.sol #L72
   Status	Resolved.

   • Description

     immutable keyword is not used when defining quoteToken variable which is only modified once in the constructor().

     As a variable that does not change after initialization when the contract is deployed, the immutable keyword should be used to protect the variable from being altered after the deployment of the contract. Furthermore, if the immutable keyword is not used, then it would be placed in storage making it more vulnerable to attacks by playing with certain types of logic in the contract and under certain circumstances.

     Using the immutable keyword prevents altering the defined variable more than once. Moreover, it saves some gas as the variable will not take part in storage.

   • Exploit Scenario

     N/A.

   • Recommendation

     Use the immutable keyword.

   • Results

     Resolved.

2. WooRouter.sol: Adding require() statement for the input arguments fromToken, toToken, to, rebateTo in swap() function.

   Severity	Low
   Source	contracts/WooRouter.sol #L97-L103
   Status	Resolved in commit 82d25b10cd7c6358ab7cf22296c01b58840ecced.

   • Description

     Add require() statements to address arguments in swap() function. This will avoid incorrectly picking addresses or sending tokens to the null address.

     swap() contains logic to pick the two token addresses required to make the trade (fromToken and toToken) and then executes it to the addresses specified (to, rebateTo). Mistakenly setting one of these arguments could lead to users wasting tokens on gas or even sending them to the null address and therefore, losing them forever.

     Verifying that the addresses are correct allows to prevent incorrect transactions from happening and since transactions are reverted, it saves on gas and even tokens that could otherwise be sent to the null address.

   • Exploit Scenario

     N/A.

   • Recommendation

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

   • Results

     Resolved in commit 82d25b10cd7c6358ab7cf22296c01b58840ecced.

3. WooRouter.sol: Adding require() statement for the input arguments approveTarget, swapTarget, fromToken, toToken, to in externalSwap() function.

   Severity	Low
   Source	contracts/WooRouter.sol #L201-L207
   Status	Resolved in commit 82d25b10cd7c6358ab7cf22296c01b58840ecced.

   • Description

     Add require() statements to address arguments in externalSwap() function. This will avoid incorrectly picking addresses or sending tokens to the null address.

     externalSwap() contains logic to pick the two token addresses required to make the trade (fromToken and toToken) and then executes it to the addresses specified. Mistakenly setting one of these arguments could lead to users wasting tokens on gas or even sending them to the null address and therefore, losing them forever.

     Verifying that the addresses are correct allows to prevent incorrect transactions from happening and since transactions are reverted, it saves on gas and even tokens that could otherwise be sent to the null address.

   • Exploit Scenario

     N/A.

   • Recommendation

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

   • Results

     Resolved in commit. 82d25b10cd7c6358ab7cf22296c01b58840ecced.

4. WooRouter.sol: Adding require() statement for non-zero address to target in setWhiteListed() function.

   Severity	Low
   Source	contracts/WooRouter.sol #L274
   Status	Resolved in commit 82d25b10cd7c6358ab7cf22296c01b58840ecced.

   • Description

     Add require() statement to address argument in setWhiteListed() function. This will avoid incorrectly picking the null address and allowing to transfer to it.

     setWhiteListed() contains logic to add addresses to the isWhitelisted mapping. This mapping is used in other functions to approve and then transfer tokens to the addresses specified. Mistakenly adding the null address to this mapping could lead to being able to send tokens to it and therefore losing them forever.

     Verifying that the addresses are correct allows to prevent incorrect transactions from happening and since transactions are reverted, it saves on gas and even tokens that could otherwise be sent to the null address.

   • Exploit Scenario

     N/A.

   • Recommendation

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

   • Results

     Resolved in commit 82d25b10cd7c6358ab7cf22296c01b58840ecced.

5. WooPP.sol:Add non-zero address check for address arguments in other solidity files, including libraries.

   Severity	Low
   Source	contracts/WooPP.sol #L72, L143, L248-249, L287, L326, L329-331, L356, L359-361 , L385, L398, L411, L421, L437, L443, L498, L523, L529-530, L537
   Status	Resolved in commit 82d25b10cd7c6358ab7cf22296c01b58840ecced.

   • Description

     Add require() statements to address arguments in different functions across WooPP.sol. This will avoid incorrectly picking addresses or sending tokens to the null address.

     Most functions listed contain logic to pick token addresses and then execute transactions based on them. Mistakenly setting one of the arguments of these functions could result in users wasting tokens on gas or even sending them to the null address and therefore, losing them forever.

     Verifying that the addresses are correct allows to prevent incorrect transactions from happening and since transactions are reverted, it saves on gas and even tokens that could otherwise be sent to the null address.

   • Exploit Scenario

     N/A.

   • Recommendation

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

   • Results

     Resolved in commit. 82d25b10cd7c6358ab7cf22296c01b58840ecced.

6. WooPP.sol: Use of actual uint type instead of uint256 for arguments newThreshold, newLpFeeRate and R in tuneParameters().

   Severity	Low
   Source	contracts/WooPP.sol #L499-L501
   Status	Not resolved.

   • Description

     Variables newThreshold, newLpFeeRate and R are first defined as uint256 and then transformed to other types, casting under certain circumstances can be vulnerable, e.g. converting big numbers to small numbers.

     In tuneParameters() function, arguments newThreshold, newLpFeeRate and R are first defined as uint256. Then, in the implementation of the function, these values are transformed into their respective types (uint112, uint64, and uint64). This can be confusing because defining them in the argument of the function directly would seem more natural and readable.

     Converting the values to their respective sizes in the function implementation makes the code harder to read and confusing since variable types can be defined in the arguments of the function and therefore, avoid the conversion.

   • Exploit Scenario

     N/A.

   • Recommendation

     Use the actual uint type instead.

   • Results

     Not resolved.

7. WooPP_proxy.sol: WooPP_proxy.solis a flattened file.

   Severity	Low
   Source	contracts/WooPP_proxy.sol
   Status	Acknowledged.

   • Description

     Flattened files in solidity are usually not good practice, only when wanting to verify in platforms such as Etherscan.

     The proxy contract of WooPP.sol has all its functions and libraries in a single file. This is not a good practice since having different folders and files to store functionality allows one to more easily check for information.

     Having organized files and folders not only helps make the code clearer but actually prevents developers from potentially making mistakes because information can be checked in a simple way.

   • Exploit Scenario

     N/A.

   • Recommendation

     Refactor the WooPP_proxy.sol.

   • Results

     Acknowledged. Talked with the WOOFi Swap team, and decided no longer to use proxy contracts as an upgradeable method, so this issue has been resolved. Besides, all liquidity is provided by WOOFi Swap team themselves, so WooPP.sol can be replaced and changed to the new one without a need for an upgradable contract.

8. WooRouter.sol: Add require() for msg.sender at the receive() function.

   Severity	Low
   Source	contracts/WooRouter.sol #L79
   Status	Resolved.

   • Description

     There is no require() statement at receive() function which means anyone can send eth to the contract address and the eth cannot be rescued.

     The receive() function in WooRouter.sol is completely empty, this means that it will receive any tokens transferred to it. Validating the users that can actually have access to transferring tokens becomes crucial because it prevents users or other contract addresses from losing them.

     Adding require() for the msg.sender to only allow wETH contract and whitelisted contracts to send ETH prevents users from losing money if mistakenly transferring tokens to it.

   • Exploit Scenario

     N/A.

   • Recommendation

     Add require() for msg.sender at the receive() function.

   • Results

     Resolved.

9. WooRouter.sol: Require postBalance to be greater than preBalance at externalSwap().

   Severity	Low
   Source	contracts/WooRouter.sol #L201-L227
   Status	Resolved.

   • Description

     Add a require() statement to validate that postBalance is greater than preBalance in order to avoid reverted transactions.

     There is a subtraction postBalance.sub(preBalance) at the event emit of externalSwap(). Similarly, when calling _generalTransfer() above in the function, there should be a check to make sure that postBalance is indeed greater than preBalance.

     Not checking for overflows can lead to unexpected reverted transactions. Moreover, adding messages in require() helps users understand why transactions revert and developers debug.

   • Exploit Scenario

     N/A.

   • Recommendation

     Add a require() to check that postBalance > preBalanace.

   • Results

     Resolved.

10. WooPP.sol: internal functions can be changed to private functions.

    Severity	Low
    Source	contracts/WooPP.sol #L146, L172, L185, L198, L211, L224, L252, L291, L547
    Status	Resolved.

    • Description

      internal functions specified in the lines above can be turned into private since their functionality lies on the same contract.

      internal functions can be accessed within the contract and the contracts deriving from it. 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 only. For this reason, using the internal keyword in the function definition is more accurate.

      Besides functionality, using private limits the reach of the methods of the contract, therefore making it less vulnerable to possible attacks.

    • Exploit Scenario

      N/A.

    • Recommendation

      In WooPP.sol, change internal functions to private functions.

    • Results

      Resolved.

11. WooPP.sol: public functions can be changed to external functions.

    Severity	Low
    Source	contracts/WooPP.sol #L448, L478, L502
    Status	Resolved.

    • Description

      Public functions specified in the lines above can be turned into external ones since there is no need to have a getter function and it saves on gas.

      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. When there’s a need for a way to interact directly with the method, public can be used, however, this is not the case.

      Besides functionality, public and external differ in terms of gas usage. The former uses more than the latter when applied to large arrays of data.

    • Exploit Scenario

      N/A.

    • Recommendation

      Change the functions from public to external.

    • Results

      Resolved.

1. WooPP.sol: No indexed parameters in the events defined.

   WooRouter.sol: No indexed parameters in the events defined.

   Severity	Informational
   Source	contracts/WooRouter.sol #L67, L77 contracts/WooPP.sol #L61
   Status	Resolved.

   • Description

     Adding index parameters to the events defined in the lines specified of contracts WooPP.sol and WooRouter.sol.

     The indexed parameters can be used as filters or queried for the logged events. This can be useful when trying to identify certain transactions or trying to create more advanced functionalities.

     There are no indexed parameters in events in WooPP.sol and WooRouter.sol which makes it impossible to query for them.

   • Exploit Scenario

     N/A.

   • Recommendations

     Add the indexed keyword to parameters in the events.

   • Results

     Resolved.

2. WooPP.sol: Require message reformat.

   WooRouter.sol: Require message reformat.

   Severity	Informational
   Source	contracts/WooRouter.sol #L82, L83, L90, L111, L236, L238, L245, L249 contracts/WooPP.sol #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
   Status	Resolved in commit 82d25b10cd7c6358ab7cf22296c01b58840ecced.

   • Description

     Making clear messages is key when debugging and understanding how some logic works. If messages are not clear or do not specify where errors are happening, it becomes very difficult to develop accurately.

     WooRouter.sol and WooPP.sol require() messages that do not have the uniform format and can not be distinguished properly. The format below is our suggestion:

       WooRouter: ZERO ADDRESS
          ｜		|
       Filename : capitalized warning message
     

     This method allows engineers to clearly distinguish which module in the architecture is causing the problem.

   • Exploit Scenario

     N/A.

   • Recommendations

     Change WooRouter.sol and WooPP.sol require() message to the format below:

       WooRouter: ZERO ADDRESS
          ｜		|
       Filename : capitalized warning message

   • Results

     Resolved in commit 82d25b10cd7c6358ab7cf22296c01b58840ecced.

3. Create interface for WooPP.sol and WooRouter.sol

   Severity	Informational
   Status	Resolved.

   • Description

     Creating an interface for WooPP.sol and WooRouter.sol would provide a clear structure of the code and make it easy for others to know how to interact with the contract.

     Interfaces in solidity contain function signatures without the function definition implementation, but they are useful in scenarios where the application requires extensibility but there’s no need for added complexity, this is because they represent a minimalistic version of the actual implementation.

     Having an interface for both contracts would provide a guideline for users and a standard of what calls can be made. This is the reason why it is considered good practice for contracts that provide important functionality.

   • Exploit Scenario

     N/A.

   • Recommendations

     Creating interface for WooPP.sol and WooRouter.sol.

   • Results

     Resolved.

4. WooPP.sol: Use delete() to reset the struct values in removeBaseToken()

   Severity	Informational
   Source	contracts/WooPP.sol #L478
   Status	Resolved.

   • Description

     WooPP.removeBaseToken() removes the values of struct tokenInfo at baseToken address. However, the implementation does not use the recommended delete to perform it.

     removeBaseToken() function in WooPP.sol is designed to remove stored baseToken from struct tokenInfo. However, it intends to reset it by converting each value of the nine members that the struct has to zero. Updating the values one by one to the default value is not the best way to clear the struct since delete does exactly that.

     Using delete is the recommended way to reset a variable type struct, as is the case with removeBaseToken().

   • Exploit Scenario

     N/A.

   • Recommendations

     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. Replace with the format delete tokenInfo[baseToken];.

   • Results

     Resolved.

• Manage the ownership of WooRouter.sol, WooPP.sol, Wooracle.sol;

• Set new pool for WooRouter.sol;

• Add and remove whitelist address;

• Pause and unpause the following functions in WooPP.sol:
  • querySellBase(),
  • querySellQuote(),
  • sellBase(),
  • sellQuote();

• Withdraw ERC20 tokens from WooRouter.sol and WooPP.sol;

• Set all important variables in Wooracle.sol.

• Add and remove strategist;

• Set price oracles for WooPP.sol;

• Set rewardManager();

• Add and remove baseToken at WooPP.sol;

• 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 multi-sig 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.

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.

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

Wooracle UML

WooGuardian.sol is the smart contract that provides the tolerance threshold of token prices given by the oracle.

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

WooGuardian UML