Smart Contract Auditing for ZenPool

Management Summary

ZenPool team contacted Sayfer Security in order to perform a full security audit for all their contracts.

Before assessing these services we held a kickoff meeting with ZenPool’s technical team and received an overview of the system and the goals for this assessment.

The following audit took 20 man-days to complete, all on-chain contracts were reviewed line by line with at least 2 auditors per contract. Due to time constraints from the client-side, we reviewed off-chain components and took a best-effort approach to make sure they do not contain any critical vulnerabilities.

We found a total of 9 findings, 3 of which were classified as “high” risk vulnerabilities and could be exploitable by malicious attackers, who could empty the pool’s funds completely.

We documented our process and our suggestions for how each vulnerability should be fixed. ZenPool’s team implemented the fixes which were documented in every vulnerability section.

System Overview

ZenPool is an open-source, non-custodial token and lending market protocol.

Users can deposit their crypto assets to earn interest or borrow other tokens to pay interest in ZenPool’s market. ZenPool has its own token called ZEN.

ZEN is a free-floating currency backed by the stable coin BUSD treasury supply. ZEN tokens can only be minted and burned by the protocol, only in response to price does the protocol do so. Each ZEN is supported by at least one BUSD.

If the price of ZEN falls below 1 BUSD, the protocol buys and burns ZEN, pushing the price back up to 1 BUSD.

ZenPool also supports bonds which are another way to increase its treasury. The protocol sells bonds in exchange for various assets, and in exchange, the buyer receives ZEN at a significantly reduced price. This boosts the treasury and allows ZeenPool to provide incredible yields to its customers.

Finally, a portion of all ZenPool product fees will be used as additional backing, potentially providing ZenPool’s token with an infinite runway for staking rewards.

Smart Contract Auditing Findings

Users Can Withdraw Funds From Other Balances Until the Pool Is Empty

Description

The withdrawFunds function validates that the user can only withdraw the maximum amount in his balance, checked by its address and later compared within the token pool.

Later the withdrawn amount is deducted from the user’s current max borrow at the current price. If the total amount of borrowed funds by the user exceeds the new max borrow, the method fails because the user no longer has enough collateral to support their borrow position. This requirement, however, is only checked if the user is not already over-leveraged:

An attacker could use this functionality to exploit the withdrawFunds function to withdraw more than the max borrow amount.

He could do that because the getMaxBorrow will only be checked if the user borrows less than the maximum amount he is allowed, in a variety of scenarios the attacker could abuse the flow to skip this specific require, allowing him to call the withdrawFund multiple times until the pool will get emptied.

Mitigation

Change the require to be called before line 145 so it does not depend on the if statement. This way the require will always be executed.

Unsafe selfdestruct in Proxy Contract

Description

When the main ZenPoolManager is deployed, it also deploys multiple contracts as well.

One of them is the ZenTickets contract which is mostly secured, except for the destoryContract function which has no ACL mechanism in place like the rest of the functions:

By exploiting this functionality an unauthenticated attacker could call the destoryContract function and choose where the ETH of the contract will be transferred to. This is easy to exploit from the attacker’s perspective.

Mitigation

Use the onlyOwner modifier like it’s used in the reset of the functions.

Another more specific solution would be to use a custom ACL mechanism like openzeppelin’s roles ACL that will only allow specific roles to access the destoryContract functions

User’s Pools are Subjected to MEV Attacks

Description

The main ZenPoolUserManager is handling custom user pools that have been created in the system. The user can call different actions on these pools if he has the right permissions to do so, a user can also grant access via role mechanism based on OpenZeppelin’s access control contracts

The ZenPoolUserManager has a proper ACL mechanism, but at the same time, it has 2 functions that are subjected to front/back running or any MEV attacks.

While the business logic itself is right, and seems like it can not do much harm as it has a proper ACL, a user with the same role could exploit it via MEV attacks.

Mitigation

Use the onlyOwner modifier like it’s used in the reset of the functions.

Another more specific solution would be to use a custom ACL mechanism like openzeppelin’s roles ACL that will only allow specific roles to access the destoryContract functions.

Lack of Guardian Mitigation

Description

During our audit we performed Original Code Analysis, and compared the client’s current codebase to the

open-source projects that the client used to develop the code. If we found changes were made in the open-source code we took a deeper look at these to make sure they were done wisely.

Most clients consider 3rd party open-source projects secure because they come from major vendors (E.G Uniswap pool). This assumption is mostly true. However, major security bugs occur when clients perform minor changes in the code and assume these changes don’t affect the overall security of the product.

During our Original Code Analysis, we found that ZenPool uses code from STRIKE protocol in order to create governance token, the original code from STRIKE repo is:

While the code in ZenGovernorAlpha is:

The following statement was removed msg.sender == guardian, this means guardian can not cancel orders if a threshold is reached. Eliminating the power of guardians can cause very dangerous situations. For instance, if a compromised proposal that transfers all the contract money to a malicious actor was performed (by a private key hack for example) the threshold protection mechanism is redundant and can’t help, because there is no guardian that can stop the proposal from happening.

Mitigation

Add msg.sender == guardian to the require .

Lack of Reenetrancy Guard

Description

The following borrow function does not contain a reentrancy guard nor the checks-effects-interactions pattern, this is currently not exploitable as the pool only supports ERC20 tokens, but if in the future new ERC-777 will be added a reentrancy exploit can occur and cause full drawing of contract funds.

In addition, the code does not follow the checks-effects-interactions pattern. In this specific case, the require is checked after some business logic is already implemented, this could cause reentrancy bugs if side effects will be added before the require.

Mitigation

Add a reentrancy guard for same function reentrancy and also use the checks-effects-interactions pattern for complex cross-function reentrancy.

Disallow Zero Amount Transfers

Description

The ZenPoolFunds.sol contract allows zero amount transfers between users. While by itself it is not a security vulnerability, this is a code sample that can expand the attack vector of a malicious attacker.

The vulnerability exist because transfer on ZenPool emit events, by doing zero amount transfer an attacker could emit multiple events that can in some scenarios trigger off-chain business logic, without even holding any token in the pool.

Mitigation

A deeper understanding of the business is needed. If possible, remove this functionality.

If there are use cases where it makes sense to use zero amount transfers, implement another layer of checks to limit the use to the users who are part of this group.

Insufficient Logging for Privileged Functions

Description

The following privileged onlyOwner  function does not emit events.

Events are a common practice for privileged functions to announce to the public that a change has been made. Without events, changes are harder to detect and users get surprised by the contract behavior.

The owner can call the chainId by executing setChainId() and modifying the chaindId parameter without any event being emitted in the process.

Mitigation

Add code that emits events.

Redundant Condition Checking

Description

When creating a new governor token the _timelockDelay parameter is being validated twice. once in all checks when the function starts and then as part of another require, this is redundant and should be removed.

Duplicate code requires more gas to run and can lead to future bugs when the business logic changes.

Mitigation

Remove the second redundant require.

Naming Inconsistency

Description

There are multiple occurrences of different styles of capitalizations (lower camel case, snake case etc.).

Mitigation

Review the code for the code styling.

Add a code styling guide, and enforce it using a CI task.