Vesper Metronome Synth (+ Vesper Escrowed Met)

The AddressSet debtTokensOfAccount is used by multiple functions to iterate over all debt tokens with non-zero balance for a specific account (and avoid iterating over all tokens of the protocol). For instance:

Pool::debtOf()

function debtOf(
    address account_
) public view override returns (uint256 _debtInUsd) {
    IMasterOracle _masterOracle = masterOracle();
    uint256 _length = debtTokensOfAccount.length(account_);
    for (uint256 i; i < _length; ++i) {
        IDebtToken _debtToken =
            IDebtToken(debtTokensOfAccount.at(account_, i));
        _debtInUsd += _masterOracle.quoteTokenToUsd(
                          address(_debtToken.syntheticToken()),
                          _debtToken.balanceOf(account_)
                      );
    }
}

Apart from the gas savings, using debtTokensOfAccount considerably reduces the probability of an out-of-gas denial of service: even if hundreds of synthetic tokens are registered in the protocol, each account will only own a handful of them, hence these loops under usual operation will be bounded.

On the other hand, under adversarial use such loops can be exploited. Imagine a malicious user with a large underwater position risking being liquidated. Assuming that a large number of synthetic tokens are available in the system, this user can completely prevent being liquidated as follows:

• He repeatedly issues new synthetic tokens, with the minimum allowed amount for each one, hence increasing the size of debtTokensOfAccount.
• After multiple repetitions, DebtToken::issue (which calls debtPositionOf) will exceed the block gas limit and fail. At this point no more tokens can be issued for this account.
• As a consequence, Pool::liquidate will also necessarily fail, since its gas usage is even higher than that of DebtToken::issue. So the user is protected from liquidation.
• In the future, the malicious user can repay the “dummy” tokens, reducing the size of debtTokensOfAccount hence removing the DoS. Note that DebtToken::repay uses no loops so it is not subject to the DoS.

Note that, if debtFloorInUsd is small (in the deployed contract it is actually 0), then the cost of issuing a large number of tokens can be sufficiently small to make the attack profitable.

To prevent such attacks we recommend adding a maximum allowed size of debtTokensOfAccount (which can be large enough not to affect the regular use of the protocol).

While the protocol has split its logic into separate token contracts and instances for keeping distinct accountancy of each one, there is a single Treasury contract which receives the deposits of all underlying assets of all Deposit tokens.

Of course, the Treasury only allows access from the DepositTokens, which are considered trusted. However, separating the funds as much as possible is a good security practice that could become relevant under some adversarial scenarios.

Consider, for instance, a hypothetical scenario in which a single DepositTokenA is compromised, while all other deposit tokens are secure. This could happen, for instance, due to the upgradeable nature of the contracts; if a vulnerability is found in the future and some unused token is not updated to the patched version, the “old” vulnerable contracts could be compromised. Even if such a scenario is unlikely, one should still have the expectation that the compromised DepositTokenA should only be able to steal its own tokens, not all Treasury’s assets.

This is not true, however, due to the lack of separation. The Treasury holds all assets and blindly trusts the underlying() asset reported by the DepositToken itself. Hence a compromised token could call pull to recover any token from the Treasury.

Treasury::pull()

function pull(
    address to_,
    uint256 amount_
) external override nonReentrant onlyIfDepositToken {
    if (to_ == address(0)) revert RecipientIsNull();
    if (amount_ == 0) revert AmountIsZero();

    // Dedaub: The underlying asset is fetched from the DepositToken
    //         contract which if compromised can return any other asset
    //         of the Treasury
    IDepositToken(msg.sender).underlying().safeTransfer(to_, amount_);
}

Below we quote some possible solutions to address this issue:

• A separate Treasury for each DepositToken could be used to ensure that if any of them gets compromised it won’t be possible for the attacker to drain the funds of the other DepositToken’s underlying assets
• A simpler approach could be to fetch the corresponding underlying asset from the Pool instead of the token itself for ensuring that even if a DepositToken gets compromised the attacker wouldn’t be able to access any other underlying asset’s funds.

The Pool contract allows users to swap their Synthetic assets with other Synthetic assets, an operation which is subject to a protocol fee. By locking MET tokens, the user can get a discount on the fees. Since locking MET tokens has value for the protocol, it makes sense to provide such a discount. However, a user could still “lock” some MET, perform the swap, and immediately unlock them, getting a discount without effectively locking anything. An early unlock has a penalty, but it is possible for the discount to still outweigh the penalty, giving incentive to the user to perform such an action. A discount without really locking any MET has no value for the protocol so it should not be allowed.

The protocol defines a defaultSwapFee (=0.25%) and several fee tiers which determine whether a user is eligible of getting a fee discount (up to 100%) or not, based on the user’s balance of esMET tokens. The tiers are handled by the FeeProvider governor and they define a minimum amount of esMET tokens that a user should hold in order to be eligible for getting the tier’s discount. A user, in order to have balance of esMET tokens, he has to lock his MET tokens in the Escrowed Met contracts for a specific time period. Based on the lock period a user selects, he can boost his esMET balance up to 5x times from the initially locked amount. For example, if someone locks 1 MET token (=1 esMET) for the maximum locking period, then his balance will be boosted and will be equal to 5 esMET tokens.

The Escrowed Met contracts allow a user to unlock his tokens before the locking period ends with a penalty which goes up to 50% of his locked amount. It could be the case that the penalty of early unlocking the MET tokens does not exceed the profit one can get by granting a 100% fee discount upon swapping specific Synthetic tokens. Hence, one could buy MET tokens from the market, lock them in Escrowed Met contracts, grant the maximum discount rate, perform the swap and then unlock them losing half of them.

Furthermore, since the maximum boost one can get is able to make his balance 5x times bigger, the losses can be significantly reduced assuming that one may need only the 1/5 of the balance the maximum discount tier requires.

For example, if the 100% discount tier requires one to have a balance of 1000 esMET tokens, the user only needs to lock 200 MET tokens for the maximum locking period. This will make his esMET balance to be boosted to 1000 esMET tokens. Then, after performing the swap with a 100% fee discount he can unlock his tokens losing 50% of them which equals to 100 MET tokens.

For a user to get profit over this manipulation, the following condition has to be met:

0.25 * Y > 50 * X
Y > (50 / 0.25) * X
Y > 200 * X

X: The MET value
Y: The Synthetic Tokens value after swapping

As a result, if the value of the Synthetic Tokens he will get after performing the swap is 200x times the value of the MET tokens he used, he can profit by performing the manipulation described above.

To prevent this kind of abuse, we recommend to select the discount tiers in a way that makes such an action non-profitable (and/or possibly increasing the penalty of early exit when the account has obtained a fee discount).

Although the governor is clearly a trusted account, one should still try to limit its capabilities as much as possible and not allow unrestricted actions without a good reason. The Treasury::sweep function (inherited from TokenHolder) trivially allows the governor to extract all funds from the treasury. Since users have little risk of sending funds to the Treasury, and a large accidental transfer could anyway be recovered by upgrading the contract, the benefits of Treasury::sweep likely don’t justify the risk. A sweep function makes perfect sense for contracts not holding funds, but not so much for a treasury.

In the Pool contract, the liquidate() function can be used by any user in order to liquidate an unhealthy position by repaying a user’s dept and getting share of his collateral as penalty.

However, the function doesn’t make use of the onlyIfSyntheticTokenExists modifier for ensuring the correctness of the given address.

However, if the Synthetic token is not registered in the system the execution will fail when the function will try to call accrueInterest() over the address(0), but adding the modifier would make the code clearer and would be more consistent as well.

Pool::liquidate()

function liquidate(
    ISyntheticToken syntheticToken_,
    address account_,
    uint256 amountToRepay_,
    IDepositToken depositToken_
) external override whenNotShutdown nonReentrant
  onlyIfDepositTokenExists(depositToken_)
  returns (uint256 _totalSeized, uint256 _toLiquidator, uint256 _fee)
{
    if (amountToRepay_ == 0) revert AmountIsZero();
    if (msg.sender == account_) revert CanNotLiquidateOwnPosition();
    // Dedaub: Here the _debtToken will be equal to address(0) if the
    //         provided syntheticToken doesn't exist. Thus, the call to
    //         accrueInterest() later will fail
    IDebtToken _debtToken = debtTokenOf[syntheticToken_];
    _debtToken.accrueInterest();
    ...
}

In the DeptToken contract, the repay() and repayAll() functions are used to repay back part or the entire amount, respectively, of the dept a user has. However, while other core functions use the onlyIfSyntheticTokenExists and the onlyIfSyntheticTokenIsActive modifiers to check the state of the Synthetic token before minting (i.e. issue() and flashIssue()), these two mentioned functions do not. This behaviour, however, might be desired by the protocol, but we mention it here in case they were left without these checks accidentally.

The protocol makes use of several contracts coming from OpenZeppelin modules. However, these contracts are copied locally in the Vesper protocol codebase. As a result, some of them might be outdated or unpatched.

For example, the local ERC20.sol contract hasn’t been updated and doesn’t contain the fix of the transferFrom() function that OpenZeppelin has introduced to not allow decrementing caller’s allowance upon transferring tokens on behalf of someone who has granted the caller an infinite allowance.

ERC20::transferFrom() (local copy)

function transferFrom(
    address sender,
    address recipient,
    uint256 amount
) public virtual override returns (bool) {
    _transfer(sender, recipient, amount);

    uint256 currentAllowance = _allowances[sender][_msgSender()];

    // Dedaub: There is no check whether the sender has granted an
    //         infinite allowance to the caller, which can result in
    //         decrementing the caller's allowance
    require(currentAllowance >= amount,
        "ERC20: transfer amount exceeds allowance");
    unchecked {
        _approve(sender, _msgSender(), currentAllowance - amount);
    }

    return true;
}

ERC20::transferFrom() (@OpenZeppelin)

/**
 * @dev See {IERC20-transferFrom}.
 * ...
 * NOTE: Does not update the allowance if the current allowance
 * is the maximum `uint256`.
 * ...
 */
function transferFrom(
    address from,
    address to,
    uint256 amount
) public virtual override returns (bool) {
    address spender = _msgSender();
    _spendAllowance(from, spender, amount);
    _transfer(from, to, amount);
    return true;
}

ERC20::_spendAllowance() (@OpenZeppelin)

/**
 * @dev Updates `owner` s allowance for `spender` based on spent `amount`.
 *
 * Does not update the allowance amount in case of infinite allowance.
 * Revert if not enough allowance is available.
 *
 * Might emit an {Approval} event.
 */
function _spendAllowance(
    address owner,
    address spender,
    uint256 amount
) internal virtual {
    uint256 currentAllowance = allowance(owner, spender);
    if (currentAllowance != type(uint256).max) {
        require(currentAllowance >= amount,
            "ERC20: insufficient allowance");
        unchecked {
            _approve(owner, spender, currentAllowance - amount);
        }
    }
}

We highly recommend to keep those files updated to the latest versions of the original modules in order to ensure security and correctness.

Following the previous issue, the DepositToken and SyntheticToken contracts define their own transferFrom() functions. However, the allowance check could be moved before the call to the _trasnfer() function for saving gas in case the allowance is not sufficient and cause the function to revert.

DespotiToken::transferFrom()

SyntheticToken::transferFrom()

function transferFrom(
    address sender_,
    address recipient_,
    uint256 amount_
) external override (...) returns (bool) {
    // Dedaub: This call to _transfer() can be moved after the allowance
    //         check for saving gas
    _transfer(sender_, recipient_, amount_);

    uint256 _currentAllowance = allowance[sender_][msg.sender];
    if (_currentAllowance != type(uint256).max) {
        if (_currentAllowance < amount_)
            revert AmountExceedsAllowance();
        unchecked {
            _approve(sender_, msg.sender, _currentAllowance - amount_);
        }
    }
    return true;
}

This approach has also been adopted by the OpenZeppelin ERC20 latest contract which first spends the allowance by calling the spendAllowance() and performing the actual token trasnfer afterwards.

ERC20::transferFrom() (@OpenZeppelin)

function transferFrom(
    address from,
    address to,
    uint256 amount
) public virtual override returns (bool) {
    address spender = _msgSender();
    // Dedaub: Here the allowance is spent before actually transferring
    //         the tokens
    _spendAllowance(from, spender, amount);
    _transfer(from, to, amount);
    return true;
}

In the DepositToken contract, the _mint() function is declared as private and is only called by the deposit() function.

DepositToken::deposit()

function deposit(uint256 amount_, address onBehalfOf_)
    external
    override
    whenNotPaused
    nonReentrant
    onlyIfDepositTokenIsActive
    onlyIfDepositTokenExists
    returns (uint256 _deposited, uint256 _fee)
{
    if (amount_ == 0) revert AmountIsZero();
    if (onBehalfOf_ == address(0)) revert BeneficiaryIsNull();

    ...
    (_deposited, _fee) = quoteDepositOut(amount_);
    if (_fee > 0) {
        _mint(_pool.feeCollector(), _fee);
    }

    _mint(onBehalfOf_, _deposited);

    emit CollateralDeposited(
        msg.sender, onBehalfOf_, amount_, _deposited, _fee);
}

DepositToken::_mint()

function _mint(address account_, uint256 amount_)
    private
    onlyIfDepositTokenIsActive
    updateRewardsBeforeMintOrBurn(account_)
{ ... }

However, _mint() uses the onlyIfDepositTokenIsActive modifier even though it has already been used by its caller which is the deposit() function. Thus, it can be removed for saving some extra gas.

Some contracts of the codebase contain some redundant code that can be simplified by calling the declared modifiers. More specifically:

• In the DepositToken contract the onlyIfUnlocked modifier exists, but the withdraw() function checks whether the given amount_ has been unlocked for the caller or not. For simplicity and consistency, this check could be replaced by the use of the corresponding modifier.
  
  DepositToken::withdraw()

function withdraw(uint256 amount_, address to_)
    external
    override
    whenNotShutdown
    nonReentrant
    onlyIfDepositTokenExists
    returns (uint256 _withdrawn, uint256 _fee)
{
    if (to_ == address(0)) revert RecipientIsNull();
    // Dedaub: This check can be replaced by using the
    //         onlyIfUnlocked modifier
    if (amount_ == 0 || amount_ > unlockedBalanceOf(msg.sender))
        revert AmountIsInvalid();
    ...
}

• Similarly, in the Pool contract the functions addToDepositTokensOfAccount and removeFromDepositTokensOfAccount can use the onlyIfDepositTokenExists modifier instead of having code checking the same thing.
  
  Pool::addToDepositTokensOfAccount()

function addToDepositTokensOfAccount(address account_) external {
    // Dedaub: This check can be replaced by using the
    //         onlyIfDepositTokenExists modifier
    if (!depositTokens.contains(msg.sender))
        revert SenderIsNotDepositToken();
    if (!depositTokensOfAccount.add(account_, msg.sender))
        revert DepositTokenAlreadyExists();
}

Pool::addToDepositTokensOfAccount()

function addToDepositTokensOfAccount(address account_) external {
    // Dedaub: This check can be replaced by using the
    //         onlyIfDepositTokenExists modifier
    if (!depositTokens.contains(msg.sender))
        revert SenderIsNotDepositToken();
    if (!depositTokensOfAccount.add(account_, msg.sender))
        revert DepositTokenAlreadyExists();
}

The function Pool:swap, computes the amount of swapped tokens as the difference of the balance before and after the swap, instead of using the amount returned by swapper.swapExactInput. This method is often used to support tokens with non-standard behaviors, for instance, those with transfer fees.

function _swap(
    ISwapper swapper_,
    IERC20 tokenIn_,
    IERC20 tokenOut_,
    uint256 amountIn_,
    uint256 amountOutMin_
) private returns (uint256 _amountOut) {
    tokenIn_.safeApprove(address(swapper_), 0);
    tokenIn_.safeApprove(address(swapper_), amountIn_);
    uint256 _tokenOutBefore = tokenOut_.balanceOf(address(this));
    swapper_.swapExactInput(
        address(tokenIn_), address(tokenOut_),
        amountIn_, amountOutMin_, address(this));
    return tokenOut_.balanceOf(address(this)) - _tokenOutBefore;
}

However, without any checks this practice is potentially risky. swapper_.swapExactInput is an external call and as such, one cannot easily exclude the possibility of transferring execution to the adversary. In such a case, this introduces a potential attack vector of manipulating the balance of the contract to inflate the amount that appears to be transferred.

Although we couldn’t find a specific vulnerability in this particular instance, we have observed this pattern in many concrete vulnerabilities, and as a consequence, we recommend proactively adding checks to the computed amount. For instance, a simple check could be added to ensure that the actual difference is no larger than the amount returned by swapper_.swapExactInput. This would support tokens with transfer fees, but without the risk of potentially inflating the computed difference.

Several functions of several contracts (Pool, DepositToken, DebtToken, etc) are protected by reentrancy guards. These guards protect against reentering a function of the same contract, however, a large amount of the protocol’s functionality is shared between contracts, hence limiting the effectiveness of such guards.

For instance, during a call to Pool::leverage or Pool:flashRepay, an adversary cannot reenter any Pool function, however, he could still try to call functions of DepositToken, for instance, to transfer his deposits. Although we could not find any concrete vulnerability, this limitation should be kept in mind in future versions of the code, and a cross-contract guard should be added if necessary.

The code is compiled with Solidity 0.8.9. Version 0.8.9, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.