Security Review Report

[christos-eth]

Yield VR Security Review

A security review of the Yield Variable Rate smart contract protocol was done by Christos Pap.
This security review report includes all the vulnerabilities, issues and code improvements found during the security review.

Disclaimer

"Audits are a time, resource and expertise bound effort where trained experts evaluate smart
contracts using a combination of automated and manual techniques to find as many vulnerabilities
as possible. Audits can show the presence of vulnerabilities but not their absence."

- Secureum

About Christos

Christos Pap is an independent security researcher who specializes in Ethereum smart contract security. He currently works as a Junior Security Researcher at Spearbit and is an alumnus of the yAcademy fellowship program. Additionally, he is undergraduate student of Electrical and Computer Engineering and he has extensive experience in offensive security, having worked as a Penetration Tester and holding the OSCP certification. You can connect with him on Twitter at @christos_eth.

Risk classification

Severity	Impact: High	Impact: Medium	Impact: Low
Likelihood: High	Critical	High	Medium
Likelihood: Medium	High	Medium	Low
Likelihood: Low	Medium	Low	Low

Impact

• High - leads to a significant material loss of assets in the protocol or significantly harms a group of users.
• Medium - only a small amount of funds can be lost (such as leakage of value) or a core functionality of the protocol is affected.
• Low - can lead to any kind of unexpected behaviour with some of the protocol's functionalities that's not so critical.

Likelihood

• High - attack path is possible with reasonable assumptions that mimic on-chain conditions and the cost of the attack is relatively low to the amount of funds that can be stolen or lost.
• Medium - only conditionally incentivized attack vector, but still relatively likely.
• Low - has too many or too unlikely assumptions or requires a huge stake by the attacker with little or no incentive.

Actions required by severity level

• Critical - client must fix the issue.
• High - client must fix the issue.
• Medium - client should fix the issue.
• Low - client could fix the issue.

Executive summary

Overview

Project Name	Yield Protocol
Repository	https://github.com/yieldprotocol/vault-v2
Commit hash	1d1602a06fda352f463b6f126c8a90e05e221541
Documentation	https://docs.yieldprotocol.com/
Methods	Manual review

Issues found

Severity	Count
Critical risk	0
High risk	2
Medium risk	1
Low risk	3
Informational	5

Scope

File	nSLOC
Contracts (6)
src/variable/VRCauldron.sol	297
src/variable/VRLadle.sol	210
src/variable/VRRouter.sol	18
src/variable/VRWitch.sol	57
src/variable/VYToken.sol	154
src/oracles/VariableInterestRateOracle.sol	149
Interfaces (2)
src/variable/interfaces/IVRCauldron.sol	22
src/variable/interfaces/IVRWitch.sol	78
Total (8)	985

Findings

ID	Title	Severity	Resolution
[H-01]	Precision loss in the VariableInterestRateOracle:get function affects the interest rate of the Yield Variable Rate Protocol	High	Fixed
[H-02]	Inaccurate refund logic causes incorrect accounting in underlying Join contract	High	Fixed
[M-01]	Name, Symbol and Decimals will have the default values in the VyToken	Medium	Fixed
[L-01]	If the spot oracle goes down, most of the Yield Variable Rate Protocol, including liquidations, can be frozen	Low	Aknowledged
[L-02]	Deviation from EIP3156 standard may affect composability	Low	-
[L-03]	An attacker can force the Redeemed event to be emitted with the wrong holder argument	Low	Aknowledged
[I-01]	No allowance front-running mitigation	Informational	Aknowledged
[I-02]	TransferHelper library does not verify token code size before executing transfers	Informational	Aknowledged
[I-03]	Lack of input validation in function parameters	Informational	Aknowledged
[I-04]	Typographical errors and missing data in comments & NatSpec docs	Informational	Fixed
[I-05]	Function ordering does not follow the Solidity style guide	Informational	Aknowledged

High severity

[H-01] Precision loss in the VariableInterestRateOracle:get function affects the interest rate of the Yield Variable Rate Protocol

Context: VariableInterestRateOracle.sol#L200-L204, VariableInterestRateOracle.sol#L206-L212, VariableInterestRateOracle.sol#L194-L196

Description: The interest rate calculation formula used in the code is based on the formula used by AAVE.

If utilizationRate <= rateParameters.optimalUsageRate, the interest rate is calculated as:

interestRate = rateParameters.baseVariableBorrowRate +
 utilizationRate.wmul(rateParameters.slope1).wdiv(rateParameters.optimalUsageRate

However, since wmul and wdiv function are used together, the calculation is performed as follows:
interestRate = rateParameters + utilizationRate * rateParameters.slope1 / 1e18 * 1e18 / rateParameters.optimalUsageRate.

This leads to precision loss, since a division (/ 1e18) is performed before the multiplication (* 1e18).

A similar issue occurs also when utilizationRate > rateParameters.optimalUsageRate.

Recommended Mitigation Steps: To avoid precision loss in the get function, it is recommended to adjust the calculations as follows:
interestRate = rateParameters.baseVariableBorrowRate + utilizationRate * rateParameters.slope1 / rateParameters.optimalUsageRate;

Yield Team: We have now removed the usage of the wmul and wdiv functions.

Christos Pap: Verified.

[H-02] Inaccurate refund logic causes incorrect accounting in underlying Join contract

Context: VRLadle.sol#L381, Join.sol#L44

Description: The repay function in the VRLadle contract can be used by a user to repay all the debt in a vault. According to the function comments, the surplus base will be returned to msg.sender.

However, the refund logic in the repay function is flawed. The user is refunded while the storedBalance from the underlying Join is reduced, leading to broken accounting in the Join contract because the storedBalance will get smaller when the surplus funds are refunded.

An attacker could potentially exploit this issue by sending a large amount to the Join contract, then calling the repay function, causing the storedBalance to decrease significantly.

The exit function of the Join contract:

    /// @dev Transfer `amount` `asset` to `user`
    function exit(address user, uint128 amount) external virtual override auth returns (uint128) {
        return _exit(user, amount);
    }

    /// @dev Transfer `amount` `asset` to `user`
    function _exit(address user, uint128 amount) internal virtual returns (uint128) {
        IERC20 token = IERC20(asset);
        storedBalance -= amount;
        token.safeTransfer(user, amount);
        return amount;
    }

Recommended Mitigation Steps: To address this issue, it is recommended to add a new function to the Join contract that retrieves the unaccounted amount. This function should be called instead of the exit function to ensure that the correct accounting is maintained.

Yield Team: Good catch. I think the Join needs a skim function that allows to take the difference between the actual balance and the stored balance, similar to this.
We fixed it by introducing a skim function in the Join smart contract.

Christos Pap: Verified.

Medium severity

[M-01] Name, Symbol and Decimals will have the default values in the VyToken

Context: VYToken.sol#L41

Description: The yield-utils-v2 ERC20 implementation does not declare the decimals, string, and symbol as immutables. As the VYToken contract is intended to be upgradeable, the constructor code of the ERC20 implementation will not be executed during initialization, resulting in default values. Consequently, VYToken has 0 decimals, which differs from the underlying token.

Due to a requirement of the proxy-based upgradeability system, no constructors can be used in upgradeable contracts. Immutable will work, since they aren't stored in the storage, and the compiler will place them into the bytecode in the deployment.

If we ran the following snippet we can see that the VYToken has 0 decimals, which is different than the intended behaviour.

  function testDecimals() public {
        console.log("underlying is:", vyToken.underlying());
        console.log(vyToken.decimals());
        console.log("Name is", vyToken.name());
 }

Recommended Mitigation Steps: It is recommended to also mark the decimals, string, and symbol as immutables in the yield-utils-v2 ERC20 token. Alternatively, the initialize function could be used to set those values.

Yield Team: Fixed by using the initialize() function to set the decimals, string, and symbol variables.

Christos Pap: Verified.

Low severity

[L-01] If the spot oracle goes down, most of the Yield Variable Rate Protocol, including liquidations, can be frozen

Severity: Low

Context: VRCauldron.sol#L139, ChainlinkMultiOracle.sol#L16

Description: The setSpotOracle function in the VRCauldron contract sets an instance of the ChainlinkMultiOracle contract contract as the IOracle. However, the ChainlinkMultiOracle contract uses a single oracle to obtain the latest price feed.

According to OpenZeppelin's Smart Contract Security Guidelines #3: The Dangers of Price Oracles,

While currently there’s no whitelisting mechanism to allow or disallow contracts from reading prices, powerful multisigs can tighten these access controls. In other words, the multisigs can immediately block access to price feeds at will. Therefore, to prevent denial of service scenarios, it is recommended to query ChainLink price feeds using a defensive approach with Solidity’s try/catch structure. In this way, if the call to the price feed fails, the caller contract is still in control and can handle any errors safely and explicitly.

Chainlink has taken oracles offline in extreme cases, such as during the UST collapse when it paused the UST/ETH price oracle to prevent protocols from receiving inaccurate data.

Recommended Mitigation Steps: To protect against the possibility of access to Chainlink feeds being denied, it is recommended to implement a safeguard such as a fallback oracle or an alternative approach that can act when needed.

Yield Team: In extreme cases a proposal could be executed to change the spotOracle. Since it is a risk in an extreme case we would stick with the current mitigation methodology.

Christos Pap: Aknowledged.

[L-02] Deviation from EIP3156 standard may affect composability

Context: VYToken.sol#L248, VYToken.sol#L181-L195

Description: VYToken isn't fully compatible with the EIP3156 standard.

According to the Lender Specification, it's noted that:

After the callback, the flashLoan function MUST take the amount + fee token from the receiver, or revert if this is not successful.

However, due to the custom _burn function in the VYToken contract, the amount is taken from the contract instead, if there are some funds inside it.

function _burn(address holder, uint256 principalAmount) internal override returns (bool) {
        // First use any tokens locked in this contract
        uint256 available = _balanceOf[address(this)];
        if (available >= principalAmount) {
            return super._burn(address(this), principalAmount);
        } else {
            if (available > 0) super._burn(address(this), available);
            unchecked {
                _decreaseAllowance(holder, principalAmount - available);
            }
            unchecked {
                return super._burn(holder, principalAmount - available);
            }
        }
    }

Recommended Mitigation Steps: To make the VYToken contract fully compatible with the EIP3156 standard, it is recommended to remove the custom _burn function and to modify the VYToken accordingly.

Yield Team: Hi, you are right, this is a deviation from the standard as written. However, when I wrote it I intended that this would be allowed (same as is in 4626).

Meaning that if the borrower approves the repayment, that should always work. However, if the lender also has an alternative repayment method and the borrower decides to use it, there should be no impediment.

Thanks for raising it, I'll revise the wording in 3156 instead of changing the code here.

Christos Pap: Verified.

[L-03] An attacker can force the Redeemed event to be emitted with the wrong holder argument

Context: VYToken.sol#L111, VYToken.sol#L143

Description: The custom _burn function allows a user to transfer vyToken to the contract to enable a burn, potentially saving the cost of approve or permit. This is achieved by using any tokens that are present in the VyToken contract.

However, this functionality can be exploited by an attacker because the withdraw and redeem functions in the contract allow a user to input a holder address as an argument. An attacker can send tokens directly to the contract and then pass any address as the holder argument in the withdraw /redeem function, which forces that address to be emitted at the Redeemed event.

Recommended Mitigation Steps: It is difficult to mitigate this issue with the current setup. A possible solution could be to check the approval in the withdaw/redeem function.

Yield Team: Our users are instructed to strictly use the frontend. So, we are not going to change this.

Christos Pap: Acknowledged.

Informational

[i-01] No allowance front-running mitigation

Context: VYToken.sol#L16

Description: The VYToken contract is inherited from ERC20Permit, which, in turn, is inherited from the ERC20 contract. These contracts are part of the yield-utils-v2 GitHub repository.

None of these contracts provide protection against the allowance front-running attack. This attack can occur when a token owner authorizes another account to transfer a specific amount of tokens on their behalf. If the token owner decides to change the allowance amount, the spender can spend both allowances by front-running the allowance-changing transaction.

Recommendation To mitigate this issue, you could consider using the OpenZeppelin ERC20 implementation. This implementation includes the increaseAllowance and decreaseAllowance functions, which protect against the allowance front-running attack. Although an attacker may not have any incentive to perform this attack, adding these functions can be seen as an added layer of protection.

[i-02] TransferHelper library does not verify token code size before executing transfers

Context: TransferHelper.sol

Description: The TransferHelper library fails to verify whether a token has code before executing a token transfer. Consequently, if the token address is empty, the transfer will succeed without crediting the contract with any tokens. While the tokens added to the VRLadle are whitelisted, this can still disrupt internal accounting in the following ways:

• If a token is self-destructed, .transferFrom and .transfer will succeed, breaking the internal accounting.
• If the Yield Variable Rate is deployed on multiple chains, such as on Mainnet and on Arbitrum, and an admin mistakenly adds a token to Arbitrum that does not exist only exists on Mainnet, it could create an opportunity for a honeypot attack similar to the Qubit Finance hack.

Recommended Mitigation Steps: To mitigate this issue, it is recommended to check if the contract has code before executing .transfer and .transferFrom functions. OpenZeppelin's SafeERC20 library verifies this condition here and can be used as a reference.

Yield Team:

If a token is self-destructed, .transferFrom and .transfer will succeed, breaking the internal accounting.

The same and worse effects can happen if one of the tokens is upgraded, which is the same kind of attack vector but is much more likely.

If the Yield Variable Rate is deployed on multiple chains, such as on Mainnet and on Arbitrum, and an admin mistakenly adds a token to Arbitrum that does not exist only exists on Mainnet, it could create an opportunity for a honeypot attack similar to the Qubit Finance hack.

With regards to all governance errors, we prefer to use a robust governance review process as it offers a more comprehensive protection.

Christos Pap: Aknowledged.

[i-03] Lack of input validation in function parameters

Context: VRCauldron.sol#L114, VRCauldron.sol#L149, VYToken.sol#L66

Description: Some functions of the Variable Rate Yield project lack threshold checks, which could lead to unexpected behaviour in certain situations.

Recommended Mitigation Steps: To address these issues, it is recommended to apply the following mitigations:

• In the setDebtLimits you could add a require statement that min > max or min >= max.
• In the setSpotOracle function could add a require statement that ratio <= 1000000.
• In the setFlashFeeFactor function a threshold could be added as an upper bound for the fee value.

Yield Team: We rely on our mature governance process to prevent issues for the above.

Christos Pap: Aknowledged.

[i-04] Typographical errors and missing data in comments & NatSpec docs

Context: VRCauldron.sol#L110, VRLadle.sol#L220, VRLadle.sol#L111, VariableInterestRateOracle.sol#L12, VYToken.sol#L20, VYToken.sol#L21, VRLadle.sol#L366

Description: During the audit, several typographical errors and missing data in comments and NatSpec documentation have been identified. Please refer to the Recommended Mitigation Steps section for the detailed list.

Recommended Mitigation Steps:

• The address -> address casting is unnecessary in VRCauldron.sol#L110.
• Typo in VRLadle.sol#L220: implemnting should be implementing.
• Since the addToken can also be used to remove a token you could consider renaming the TokenAdded event to TokenStatusChanged.
• The VariableInterestRateOracle.sol contract is missing NatSpec documentation.
• The Point event isn't used in the VYToken. You could consider removing it.
• You could consider also emitting also the old fee value in the FlashFeeFactorSet event.
• The comment in the repay function is wrong. Consider replacing it with: The surplus base will be returned to the refundTo address, if refundTo is different than address(0).

Yield Team: Fixed.

Christos Pap: Verified.

[i-05] Function ordering does not follow the Solidity style guide

Context: VRWitch.sol#L15, VRCauldron.sol#L13, VRLadle.sol#L21, VYToken.sol#L16

Description: The recommended order of functions in Solidity, as outlined in the Solidity style guide, is as follows: constructor(), receive(), fallback(), external, public, internal and private. However, this ordering isn't enforced through the Variable Rate codebase.

Recommended Mitigation Steps: It is recommended to follow the recommended order of functions in Solidity, as outlined in the Solidity style guide.

Yield Team: We are going to stick to our current style.

Christos Pap: Aknowledged.

Additional Notes

• During the security review, it was discovered that some sections of the code did not follow the Check Effects Interaction (CEI) pattern. These functions do not have the protection of the nonReentrant() modifier. If any ERC777 tokens are supported, a re-entrancy (either a same-function or a cross-function) could be introduced.
  The protocol team responded:

Tokens are supported in a case-by-case basis. We haven't supported any erc777 tokens so far, and have no immediate plans to do so. If we would, we would investigate the consequences at that time.

[christos-eth]

• Do you consider this an issue? Pr variable rate 2 #3 (comment) I could include it in my report as a low level finding as well.
  Given the fact that the _convertToUnderlying function rounds down, If a user calls the mint (https://github.com/yieldprotocol/vault-v2/blob/1d1602a06fda352f463b6f126c8a90e05e221541/src/variable/VYToken.sol#L147) function , the user may pay some tokens due to the safeTransferFrom in the Join (https://github.com/yieldprotocol/vault-v2/blob/1d1602a06fda352f463b6f126c8a90e05e221541/src/Join.sol#L38) but he may end up with 0 vyTokens. I think that it will be a similar check like : https://github.com/Uniswap/v2-core/blob/ee547b17853e71ed4e0101ccfd52e70d5acded58/contracts/UniswapV2Pair.sol#LL125C1-L125C1
• I think that we have to also consider this one: Pr variable rate 2 #3 (comment) I was thinking something like this:

1. The user triggers a call to the VariableInterestRateOracle variable to update the price. The VariableInterestRateOracle is only updated once per block, so it won't be updated again for the current block.
2. The user takes out a large flashloan from the VYToken.
3. The user converts their vyTokens back to principal using the outdated price that was in effect before they took out the flashloan. The VariableInterestRateOracle price isn't updated.
4. The user creates a vault and deposits some collateral.
5. The user invokes the level function to move collateral from one vault to another.
6. The user repays the vault using either the repay or the pour function.
7. The user swaps the underlying asset for the underlying asset they used in step 1.
8. The user mints vyTokens using the vrLadle via the router.
9. The user repays the flashloan and takes the profit.

Regarding the attack that can be performed by manipulating the outdated price, the level function will attempt to convert the debt to base using the outdated price, which can result in an inaccurate calculation. However, if the user is prevented from repaying a vault in the same transaction in which the vault was created/funded, this type of attack can be prevented.
The issue I see is the fact that the VariableInterestRateOracle  is only updated once  per block + that a user can take a flashloan, deposit the collateral and then repay the vault 

Perhaps we could ask the other auditors as well if they consider it as an issue. If an issue cannot be identified I could add it in the Additional Notes section.

[iamsahu]

• Do you consider this an issue? Pr variable rate 2 #3 (comment) I could include it in my report as a low level finding as well
• I think that we have to also consider this one: Pr variable rate 2 #3 (comment) I was thinking something like this:

1. The user triggers a call to the VariableInterestRateOracle variable to update the price. The VariableInterestRateOracle is only updated once per block, so it won't be updated again for the current block.
2. The user takes out a large flashloan from the VYToken.
3. The user converts their vyTokens back to principal using the outdated price that was in effect before they took out the flashloan. The VariableInterestRateOracle price isn't updated.
4. The user creates a vault and deposits some collateral.
5. The user invokes the level function to move collateral from one vault to another.
6. The user repays the vault using either the repay or the pour function.
7. The user swaps the underlying asset for the underlying asset they used in step 1.
8. The user mints vyTokens using the vrLadle via the router.
9. The user repays the flashloan and takes the profit.

Regarding the attack that can be performed by manipulating the outdated price, the level function will attempt to convert the debt to base using the outdated price, which can result in an inaccurate calculation. However, if the user is prevented from repaying a vault in the same transaction in which the vault was created/funded, this type of attack can be prevented.
The issue I see is the fact that the VariableInterestRateOracle  is only updated once  per block + that a user can take a flashloan, deposit the collateral and then repay the vault 

Perhaps we could ask the other auditors as well if they consider it as an issue. If an issue cannot be identified I could add it in the Additional Notes section.

You can add these in additional notes section. We have no plan of enabling flashloans on vyToken so not testing/fixing this for now.

[iamsahu]

@DecorativePineapple : Point 2 has been addressed in #21

[alcueca]

Regarding the rounding attack on mint or deposit, the attack involves front-running the deposit and inflating the supply of the vyToken so that the contract assets increase by the amount that the victim lost to rounding. This increase is profit for other depositors.

If the victim's deposit is very large with regards to the contract assets, this would be significant.

A zero check is quite limited in its protection, as the attacker could then modify the attack so that the user gets exactly 1 share.

We have implemented a system for general transaction checks that offers better protection. We will include vyToken transactions in a batch with a call at the end that verifies that the user received a number of tokens within slippage settings.

https://github.com/yieldprotocol/yield-utils-v2/blob/main/src/utils/Assert.sol

[alcueca]

Regarding the issue with the interest rate oracle not being updated more than once in a block, we will fix that at the root, by removing the maximum update per block constraint.