Skip to main content
Dedaub

Vesper Pools Delta, ESVSP, VSP Minter

Smart Contract Security Assessment

May 12, 2022

Vesper

SUMMARY


ABSTRACT

Dedaub was commissioned to perform a security audit of several smart contract modules of four independent Vesper protocols.

This report includes findings:

  1. on the recent changes (on multiple files) in the Vesper Pools V3 protocol, in the repository https://github.com/bloqpriv/vesper-pools-v3, up to commit b3719d7bb8b9c995b82ed2c21a689197bfccba75 (from commit 0a2a497a0974a21ddb2dce51d6e6afaf7586fa87). We have audited the project previously, up to the listed earlier commit.
  2. On the Escrowed VSP project in the repository https://github.com/bloqpriv/escrowed-vsp.
  3. On the VSP Minter project in the repository https://github.com/bloqpriv/vsp-minter.

SETTING AND CAVEATS

The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than regular use of the protocol. Functional correctness (i.e., issues in "regular use") is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e., full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling, quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.

Pools

Our earlier audits describe the setting and caveats for Vesper Pools V3. As a general warning, we note that an audit of small changes in a large protocol is necessarily out-of-context. We made a best-effort attempt to understand the changed lines of code and assess whether these changes are reasonable and do not introduce vulnerabilities. The audit, however, was restricted to the modified lines, and their interaction with the rest of the protocol is not always easy to assess.

Escrowed VSP

The Escrowed VSP (ESVSP) system is designed to allow VSP token holders to lock their tokens for a period between the set minimum and maximum locking periods. In exchange for locking their funds, users receive regular or “boosted” (by a set factor) rewards that are proportional to their participation in the total locked funds in the system. Locked positions are represented by newly minted ERC721 tokens. The system allows users to unlock their tokens before their set locking period finishes by paying a fee.

VSP Minter

The VSP Minter system is designed to allow users to buy newly minted VSP tokens which are then locked using the ESVSP system, using collateral tokens approved/wanted by the Vesper Governance. The value of the collateral tokens is estimated using a number of oracles (Chainlink, TWAPS), and an amount of VSP of equal value is minted and locked for the user for a period specified by them, using the ESVSP system. In addition an equal amount of VSP is minted and sent to the Vesper Treasury, thus, in total, minting VSP with a value equal to twice the value of collateral accepted. We do not understand the basis for the financial soundness of this minting and are currently awaiting explanations.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues that affect the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

Issue resolution includes “dismissed”, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

[No critical severity issues]


HIGH SEVERITY

[No high severity issues]


MEDIUM SEVERITY

M1

Possible DoS due to unbounded loop

M1MEDIUM

Possible DoS due to unbounded loop
closed

ESVSP::_kickAllExpiredOf performs a loop over all NFTs owned by the account, in order to kick the expired ones.

uint256 _len = esVSP721.balanceOf(account_);
uint256[] memory _toKick = new uint256[](_len);

for (uint256 i; i < _len; ++i) {
uint256 _tokenId = esVSP721.tokenOfOwnerByIndex(account_, i);
if (block.timestamp > positions[_tokenId].unlockTime) {
_toKick[i] = _tokenId;
}
}

The number of such NFTs is unbounded and can be controlled by an adversary. If the adversary transfers a large number of positions to a victim (which can be cheap, since the positions’ amounts can be arbitrarily small) then ESVSP::_kickAllExpiredOf might reach the block gas limit, making it impossible to execute.

Moreover, ESVSP::_kickAllExpiredOf is called by various other operations (lock, lockFor, unlock, Rewards::claimReward), so the DoS extends to those operations as well. Note that the victim will be unable to recover from the DoS since burning these positions will be impossible (using ESVSP::kick could be a solution, since it does not call _kickAllExpiredOf, but only after the positions expire).

To prevent such DoS, we recommend to ensure that the size of all loops in the code cannot be controlled by an adversary. Some possible ways to achieve this:

  • Set a hard limit to the number of positions owned by a single account.
  • Alternative, allow an arbitrary number of positions, but kick only the first N of them in ESVSP::_kickAllExpiredOf. If more positions exist, they will be kicked by the next call of _kickAllExpiredOf, or manually by ESVSP::kick.

M2

BenqiXYStrategy::_getRewardsAsCollateral()

can return a wrong value under some conditions

M2MEDIUM

BenqiXYStrategy::_getRewardsAsCollateral() can return a wrong value under some conditions
closed

The getRewardsAsCollateral() method of the BenqiXYStrategy contract distinguishes the case where address(collateralToken) == rewardToken. However for this case the _rewardsAsCollateral variable doesn’t get a value assigned to it, although it should be equal to _rewardsAccrued.

function _getRewardsAsCollateral(uint8 rewardType_, address rewardToken_)
internal view returns (uint256 _rewardsAsCollateral)
{
uint256 _rewardsAccrued = IRewardDistributor(rewardDistributor).rewardAccrued(rewardType_, address(this));

if (address(collateralToken) != rewardToken_ && _rewardsAccrued > 0) {
(, _rewardsAsCollateral, ) = swapManager.bestOutputFixedInput(
rewardToken_,
address(collateralToken),
_rewardsAccrued
);
}
}

M3

BenqiXYStrategy doesn’t implement the _getUnderlyingToken() method

M3MEDIUM

BenqiXYStrategy doesn’t implement the _getUnderlyingToken() method
closed

The _getUnderlyingToken() method of the BenqiXYStrategy contract is not implemented. It is instead inherited from the CompoundXYStrategy contract which has special handling for cEth. Due to this, in the current state of the codebase, qiAVAX is not handled as it should have been handled (in a way analogous to cEth in CompoundXYStrategy).



LOW SEVERITY

L1

No reentrancy protection in ESVSP

L1LOW

No reentrancy protection in ESVSP
closed

In ESVSP::lock (and possibly other ESVSP functions), the possibility of code reentrancy would be easily exploitable by an adversary. However, in the current version, there are no parts of the code that transfer the execution to adversary-controlled code; as a consequence, no reentrancy guards have been used.

We believe that this practice has the risk that, in the future, a reentrancy possibility could be introduced without realizing its consequences for this specific contract, leading to critical vulnerabilities. Consider for instance the following code from ESVSP:🔒

uint256 balanceBefore_ = VSP.balanceOf(address(this));
VSP.safeTransferFrom(_msgSender(), address(this), amount_);
uint256 _lockedAmount = VSP.balanceOf(address(this)) - balanceBefore_;

Now assume that VSP::safeTransferFrom calls some code controller by the sender. The adversary could reenter in ESVSP::lock and create another position, which would further increase the contract’s VSP balance, leading to _lockedAmount being larger that it should, hence stealing funds from the contract.

Currently, VSP does not seem to call user code, but it is not impossible in the future to add ERC777 hooks to it, hence allowing such an attack via the tokensToSend hook. (Similarly, one might want to use ESVSP in the future with some other token that does provide hooks).

We recommend to either add reentrancy guards to all functions that are susceptible to reentrancy attacks, or at least add clear warnings in the documentation that these functions are vulnerable to reentrancy and all future modifications should take this fact into account.

L2

No-op call in VesperIronBankXYStrategy::_claimRewardsAndConvertTo()

L2LOW

No-op call in VesperIronBankXYStrategy::_claimRewardsAndConvertTo()
closed

Inside the _claimRewardsAndConvertTo() method of the VesperIronBankXYStrategy contract a call to super._claimRewardsAndConvertTo() is made. This however calls the Strategy::_claimRewardsAndConvertTo() method which does not contain any code.

L3

Unclear semantics of the relationship of different component contracts of the ESVSP system

L3LOW

Unclear semantics of the relationship of different component contracts of the ESVSP system
closed

The 3 main contracts of the ESVSP system: ESVSP, ESVSP721, and Rewards all have synchronized state that is used to collectively manage the positions of users and compute their rewards.

However the methods ESVSP::setRewards() and ESVSP721::setESVSP() allow the Vesper governance to update the respective storage fields for ESVSP and ESVSP721. Doing so when the system is up and running would have unintended side effects, as the state of the newly set contracts would be out-of-sync from the rest of the system. Due to this we believe these methods need to only be callable at an initial state of the protocol.

In addition, the esVSP and esVSP721 fields of the Minter contract are both immutable, indicating that the ESVSP721::setESVSP() should never be called after the ESVSP system has started being in a usable state.

L4

Dead code in AaveLeverageAvalancheStrategy

L4LOW

Dead code in AaveLeverageAvalancheStrategy
closed

As also indicated by the comments in the code, dydx is not deployed on the Avalanche blockchain:

function updateFlashLoanStatus(bool, bool _aaveStatus) external virtual onlyGovernor {
// No DYDX support on AVA chain
// _updateDyDxStatus(_dydxStatus, address(collateralToken));
_updateAaveStatus(_aaveStatus);
}

However the rest of the code of this contract still contains code that handles dydx flash loans:

function _doFlashLoan(uint256 _flashAmount, bool _shouldRepay) internal returns (uint256) {
uint256 _totalFlashAmount;
if (isDyDxActive && _flashAmount > 0) {
_totalFlashAmount = _doDyDxFlashLoan(
address(collateralToken),
_flashAmount,
abi.encode(_flashAmount, _shouldRepay)
);
_flashAmount -= _totalFlashAmount;
}
if (isAaveActive && _shouldRepay && _flashAmount > 0) {
_totalFlashAmount += _doAaveFlashLoan(
address(collateralToken),
_flashAmount,
abi.encode(_flashAmount, _shouldRepay)
);
}
return _totalFlashAmount;
}

In particular the _doFlashLoan() method may only work when _shouldRepay == true. This does not break any required functionality of the contract as both kinds of flash loans are only optionally enabled. However we believe that dead code should be removed from the contract and the limited use of the _doFlashLoan() method should be acknowledged.

L5

Unclaimed rewards in CrvA3PoolStrategy

L5LOW

Unclaimed rewards in CrvA3PoolStrategy
closed

In the CrvA3PoolStrategy contract a _claimAave() internal method is implemented, to claim AAVE token rewards. However after the restructuring performed in commit 7e98fd6eeee90252e2bdfd961ba1f56bbd7b81ac this method is never called, leaving some rewards of the strategy unclaimable.



OTHER/ ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.

A1

Compiler bugs

A1ADVISORY

Compiler bugs
info

Vesper Pools contracts were compiled with the Solidity compiler v0.8.9 which, at the time of writing, has no known issues.



DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.


ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

Dedaub