Links

PeckShield Audit Report

Access the full report here
Here is a breakdown of some key issues or misbehaviours identified by PeckShield and the fixes implemented wherever it was required.

Issue ID and Fixes:

PVE-001: Improper Funding Source in Locker

Auditor Severity Rating: Medium
Description: By design, the only way to obtain the governance veOLIVE tokens is by locking OLIVE tokens. A user’s veOLIVE balance decays linearly over time. And the rewards are distributed weekly and proportionally to the veOLIVE holder’s balance. While reviewing the current locking logic, we notice the key helper routine _deposit_for() needs to be revised.
To elaborate, we show below the implementation of this _deposit_for() helper routine. In fact, it is an internal function to perform deposit and lock OLIVE for a user. This routine has a number of arguments and the first one _addr is the address to receive the veOLIVE balance. It comes to our attention that the _addr address is also the one to actually provide the assets, oliveToken.transferFrom (_addr, address(this), _value)) (line 255). In fact, the msg.sender should be the one to provide the assets for locking! Otherwise, this function may be abused to lock veOLIVE tokens from users who have approved the locking contract before without their notice.
Status: Fixed
Alleviation: The issue has been fixed in the following commit: ef49048

PVE-002: Proper Pool/Manager Initialization in Pool/Manager

Auditor Severity Rating: Low
Description: The governance support also comes with core Pool and Manager contracts. These contracts inherit from a number of well-defined modules, such as ERC20, Ownable, and ReentrancyGuard. Note these parent contracts also come with their own specific initialization routines. While examining the initialization logic of these two contracts (Pool and Manager), we notice their current implementation needs to be improved.
To elaborate, we show below the implementation of the initialize() helper routine from the Pool contract. It has properly invoked the initialization routines from the inherited ERC20, Ownable, and Pausable. However, it comes to our attention the initialization routine from the inherited ReentrancyGuard is not invoked. The lack of the ReentrancyGuard initialization may cause issues in the needed reentrancy protection.
Status: Fixed
Alleviation: The issue has been fixed in the following commit: ef49048

PVE-003: Trust Issue of Admin Keys

Auditor Severity Rating: Low
Description: In the OliveDAO protocol, there are certain privileged accounts, i.e., admin. When examining the related contracts, we notice an inherent trust in these privileged accounts. For example, this admin account plays a critical role in governing and regulating the system-wide operations (e.g., configuring various settings). It also has the privilege to control or govern the flow of assets within the protocol contracts. In the following, we examine the privileged account and their related privileged accesses in current contracts.
We understand the need for privileged functions for proper contract operations, but at the same time, the extra power to the admin may also be a counter-party risk to the contract users. Therefore, we list this concern as an issue here from the audit perspective and highly recommend making these privileges explicit or raising necessary awareness among protocol users.
Status: Confirmed
Alleviation: This issue has been confirmed.

PVE-004: Accommodation of Non-ERC20-Compliant Tokens

Auditor Severity Rating: Low
Description: Though there is a standardized ERC-20 specification, many token contracts may not strictly follow the specification or have additional functionalities beyond the specification. In this section, we examine the approve() routine and analyze possible idiosyncrasies from current widely-used token contracts.
In particular, we use the popular stablecoin, i.e., USDT, as our example. We show the related code snippet below. On its entry of approve(), there is a requirement, i.e., require(!((_value != 0) && (allowed[msg.sender][_spender] != 0))). This specific requirement essentially indicates the need of reducing the allowance to 0 first (by calling approve(_spender, 0)) if it is not, and then calling a second one to set the proper allowance. This requirement is in place to mitigate the known approve()/ transferFrom() race condition github.com/ethereum/EIPs/issues/20#issuecomment-263524729.
Because of that, a normal call to approve() is suggested to use the safe version, i.e., safeApprove(). In essence, it is a wrapper around ERC20 operations that may either throw on failure or return false without reverts. Moreover, the safe version also supports tokens that return no value (and instead revert or throw on failure). Note that non-reverting calls are assumed to be successful. Similarly, there is a safe version of transfer() as well, i.e., safeTransfer().
In the current implementation, if we examine the Pool::approveManager() routine, it is designed to authorize the given account for the intended spending. To accommodate the specific idiosyncrasy, there is a need to use safeApprove(), instead of safeIncreaseAllowance(). Moreover, the safeApprove() call needs to be invoked twice: the first time resets the allowance to 0 and the second time sets the intended allowance amount.
Status: Fixed
Alleviation: The issue has been fixed in the following commit: ef49048.

Post-Audit Conclusion

In this audit, PeckSheild analyzed the design and implementation of the governance support in the OliveDAO protocol, which enables users to provide liquidity without bearing the risk of impermanence loss while providing web3 projects with an everlasting source of sustainable, low-cost liquidity for their project tokens. PeckSheild concluded that the current code base is well organized and all identified issues have been promptly confirmed and addressed.