Vesper Pools Delta, ESVSP, VSP Minter

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.

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
     );
  }
}

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

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.

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.

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.

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.

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.

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