cducrest-brainbot

medium

IchiVaultOracle vulnerable to IchiVault owner change of twap period

Summary

The IchiVaultOracle that uses Ichi vault to price the LP token does so using the vault twap and spot price. It checks that the twap period of the vault is not outside the range of constants defined by the protocol (1 hour - 2 days). This value is set by the owner of the Ichi vault and can be changed at anypoint. The owner of Ichi vault can make the oracle selectively revert.

Vulnerability Detail

IchiVaultOracle's oracle getPrice uses twapPrice0InToken1:

    function getPrice(address token) external view override returns (uint256) {
        ...
        uint256 spotPrice = spotPrice0InToken1(vault);
        uint256 twapPrice = twapPrice0InToken1(vault);
        ...

twapPrice0InToken1 calls the Ichi vault and reverts when twap period is outside of range:

    function twapPrice0InToken1(
        IICHIVault vault
    ) public view returns (uint256) {
        uint32 twapPeriod = vault.twapPeriod();
        if (twapPeriod > Constants.MAX_TIME_GAP)
            revert Errors.TOO_LONG_DELAY(twapPeriod);
        if (twapPeriod < Constants.MIN_TIME_GAP)
            revert Errors.TOO_LOW_MEAN(twapPeriod);

twapPeriod can be set by owner of IchiVault (twap period is currently at the minimum accepted time of 1 hour):

    function setTwapPeriod(uint32 newTwapPeriod) external onlyOwner {
        require(newTwapPeriod > 0, "IV.setTwapPeriod: missing period");
        twapPeriod = newTwapPeriod;
        emit SetTwapPeriod(msg.sender, newTwapPeriod);
    }

Owner of IchiVault is a multisig.

Impact

Owner of IchiVault can accidentaly break IchiVaultOracle by changing the twap period.

Owner of IchiVault can maliciously and selectively update twap period to decide when the oracle will fail. If a position uses ichi vault tokens, the owner can prevent its repayment, its liquidation, or any other action. It can lock positions until they are liquidatable and liquidate them themselves. It can prevent their own position to be liquidated.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/oracle/IchiVaultOracle.sol#L84-L103

Tool used

Manual Review

Recommendation

Make explicit your reliance on IchiVault's owner, the contest details only state reliance on Chainlink. If you rely on correct behaviour of the Ichi vault, remove the checks for twap period constraints.

moneyversed

high

Hard Vault has no validation for depositing tokens other than LP tokens

Summary

The deposit function of HardVault does not check if the user is trying to deposit a token that is not an LP token. This can lead to a loss of user funds if they accidentally deposit the wrong token.

Vulnerability Detail

The deposit function of HardVault allows users to deposit any ERC20 token, not just LP tokens. This is because the function only checks if the amount parameter is zero or not, but does not check if the token parameter is a valid LP token.

Impact

If a user accidentally deposits an ERC20 token that is not a valid LP token, the tokens will be locked in the HardVault contract and cannot be withdrawn. This can result in a loss of user funds.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/vault/HardVault.sol#L105

Tool used

Manual Review

Recommendation

The deposit function should be updated to check if the token parameter is a valid LP token. If the token is not a valid LP token, the function should revert with an appropriate error message.

cducrest-brainbot

medium

ChainlinkAdapterOracle can still give stale data

Summary

The implemented fix to issue sherlock-audit/2023-02-blueberry-judging#94 is not sufficient. The ChainlinkAdapterOracle can still give stale data.

Vulnerability Detail

The fix is the following:

    function getPrice(address token_) external view override returns (uint256) {
        // remap token if possible
        address token = remappedTokens[token_];
        if (token == address(0)) token = token_;

        uint256 maxDelayTime = timeGaps[token];
        if (maxDelayTime == 0) revert Errors.NO_MAX_DELAY(token_);

        // Get token-USD price
        uint256 decimals = registry.decimals(token, USD);
        (, int256 answer, , uint256 updatedAt, ) = registry.latestRoundData(
            token,
            USD
        );
        if (updatedAt < block.timestamp - maxDelayTime)
            revert Errors.PRICE_OUTDATED(token_);
+       if (answer <= 0) revert Errors.PRICE_NEGATIVE(token_); 

        return
            (answer.toUint256() * Constants.PRICE_PRECISION) / 10 ** decimals;
    }

There is no check regarding answeredInRound and roundId, so the price data could be carried over.

Impact

Correct price is detrimental to the protocol as a whole. It is required to guarantee proper borrow limits, liquidation conditions, etc ...

An incorrect price may result in loss of funds for users e.g. when they are liquidated due to an incorrect price while they should not have.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/oracle/ChainlinkAdapterOracle.sol#L77-L97

Tool used

Manual Review

Reference

code-423n4/2022-04-backd-findings#17
https://docs.chain.link/data-feeds/historical-data

Recommendation

Add require(answeredInRound >= roundID, "Stale price"); as an additional check:

    function getPrice(address token_) external view override returns (uint256) {
        // remap token if possible
        address token = remappedTokens[token_];
        if (token == address(0)) token = token_;

        uint256 maxDelayTime = timeGaps[token];
        if (maxDelayTime == 0) revert Errors.NO_MAX_DELAY(token_);

        // Get token-USD price
        uint256 decimals = registry.decimals(token, USD);
-        (, int256 answer, , uint256 updatedAt, ) = registry.latestRoundData(
+        (uint80 roundID, int256 answer, uint256 updatedAt, uint80 answeredInRound) = registry.latestRoundData(
            token,
            USD
        );
        if (updatedAt < block.timestamp - maxDelayTime)
            revert Errors.PRICE_OUTDATED(token_);
       if (answer <= 0) revert Errors.PRICE_NEGATIVE(token_); 
+      if(answeredInRound < roundID) revert Errors.PRICE_OUTDATED(token_);

        return
            (answer.toUint256() * Constants.PRICE_PRECISION) / 10 ** decimals;
    }

Duplicate of #118

n1punp

high

No slippage control when closing position in ConvexSpell

Summary

No slippage control when closing position in ConvexSpell

Vulnerability Detail

No slippage control when closing position in ConvexSpell, specifically remove_liquidity_one_coin can output almost 0 tokens, if the attacker sandwich attack so the desired is very very expensive (low liquidity in the underlying pool).

Impact

Attacker can sandwich attack users when they want to close position in ConvexSpell, leading to users getting nothing out (0 tokens).

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/ConvexSpell.sol#L147-L230

Tool used

Manual Review

Recommendation

• Add slippage control for all tokens that are involved with swapping (including reward token CVX).

Duplicate of #121

devScrooge

medium

Accrue function is not called before executing some functions

Summary

As the NatSpec comments and documentation indicate, the functions getDebtValue, getIsolatedCollateralValue, getPositionDebt, on the BlueBerryBank contract, the accrue function should be called first to get the current debt, but it is actually not being called.

Vulnerability Detail

The NatSpec lines 340, 420, 431 and also in the Blueberry docs indicates that: The function should be called after calling the accrue function to get the current debt.

But actually none of these function (getDebtValue, getIsolatedCollateralValue, getPositionDebt) are calling the accrue function before.

Impact

No calling the accrue function before executing the mentioned function means that the following operations and/or calculations are not done with the actual value of the current debt, thus a non-correct value is being used.

Inside the BlueBerryBank contract, all of the mentioned functions are called by functions that are called by other functions that implement the poke modifier, which in turn calls the accrue function. This means that the debt is going to be updated to the current one so the value will be correct but the getDebtValue, getIsolatedCollateralValue, getPositionDebt functions are public so future or external implemented contracts can call them and use a non update value for the current debt.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L340,
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L420,
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L431

Tool used

Manual Review

Recommendation

Add the poke modifier to the getDebtValue, getIsolatedCollateralValue, getPositionDebt functions so that if external contracts call to this functions a correct value of the current debt is going to be used correct.

BugHunter101

medium

HardVault.sol ,the function withdraw() does not check the token

Summary

HardVault.sol ,the function withdraw() does not check the token

Vulnerability Detail

The withdraw function in the HardVault contract seems to be missing some input validation. Specifically, it does not check whether the token address passed in is a valid ERC20 token address. This could potentially lead to a situation where a user is able to withdraw tokens that are not supported by the vault, or worse, tokens that do not exist.

Impact

Affect the normal use of functions

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/vault/HardVault.sol#L126

function withdraw(
address token,
uint256 shareAmount
) external override nonReentrant returns (uint256 withdrawAmount) {
if (shareAmount == 0) revert Errors.ZERO_AMOUNT();
IERC20Upgradeable uToken = IERC20Upgradeable(token);
_burn(msg.sender, _encodeTokenId(token), shareAmount);

    // Cut withdraw fee if it is in withdrawVaultFee Window (2 months)
    _ensureApprove(
        address(uToken),
        address(config.feeManager()),
        shareAmount
    );
    withdrawAmount = config.feeManager().doCutVaultWithdrawFee(
        address(uToken),
        shareAmount
    );
    uToken.safeTransfer(msg.sender, withdrawAmount);

    emit Withdrawn(msg.sender, withdrawAmount, shareAmount);
}

Tool used

Manual Review

Recommendation

we should ensure that only trusted token contract addresses are accepted as parameters, and use the whitelist.

darksnow

high

_disableInitializers() function missing in contracts AuraSpell, ConvexSpell, CurveSpell. Anyone can take ownership of the implementation contracts

Summary

_disableInitializers() function missing in contracts AuraSpell, ConvexSpell, CurveSpell.

Vulnerability Detail

_disableInitializers() is used in the constructor to prevent initialization of the implementation contract. This function is missing in contracts AuraSpell, ConvexSpell, CurveSpell.

Impact

Anyone can take ownership of the implementation contracts and can potentially use this as an attack vector.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/AuraSpell.sol#L27-L37

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/ConvexSpell.sol#L27-L37

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L27-L37

Tool used

Manual Review

Recommendation

Implement the constructor with oz _disableInitializers() function.

constructor() {
    _disableInitializers();
}

ravikiran.web3

medium

When CoreOracle is paused, it is better to expose the paused/unpaused status so contracts using CoreOracle can know early about non availability

Summary

When the CoreOracle is paused, it directly impacts many functions of BlueBerryBank.
Examples below are

1. isWrappedTokenSupported
2. getWrappedTokenValue
3. getTokenValue
4. and more

but, the whenNotPaused() is attached only to _getPrice() which is called at the very end in the flow. The failure comes at the very end in the logic cycle.

Vulnerability Detail

The error comes very late in the business logic cycle since the status of CoreOracle pause status is not being checked.

Impact

Business functins fail:
Whitelisting will get in fine, but errors will be thrown during computation of position.

The token while whitelist might get in as a valid token, but eventually, the price will not be available and business logic will get "NO_ORACLE_ROUTE" error creating conflicts in the business flows down the line.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/oracle/CoreOracle.sol#L86-L104

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L201-L217

Tool used

Manual Review

Recommendation

Two approaches:
a) attach whenNotPaused() modifier to all functions of CoreOracle contract
b) expose a public or external function on CoreOracle contract which can be checked by all clients before attempting any computation on valuation.

moneyversed

medium

Unchecked return value of external call in joinPool and exitPool

Summary

The AuraSpell and CurveSpell contracts contain an external call to a third-party contract without checking the return value of the call. This can lead to unexpected behavior in the contract and could allow an attacker to execute a re-entrancy attack.

Vulnerability Detail

The AuraSpell and CurveSpell contracts make an external call to the joinPool and exitPool functions of the Balancer contract and the add_liquidity function of the Curve contract, respectively. These calls are made without checking the return value of the call, which can lead to unexpected behavior in the contract.

An attacker can potentially exploit this vulnerability by executing a re-entrancy attack. By calling back into the contract before the previous call has completed, an attacker can manipulate the state of the contract to their advantage.

Impact

An attacker can potentially exploit this vulnerability to manipulate the state of the contract, which could lead to unexpected behavior and result in a loss of funds for the Blueberry protocol.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/AuraSpell.sol#L109

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/AuraSpell.sol#L188

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L98

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L106

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L114

Tool used

Manual Review

Recommendation

To mitigate this vulnerability, the contracts should check the return value of external calls to ensure that they complete successfully before proceeding with the next step. If the call fails, the contract should revert the transaction to prevent further execution. Additionally, the contracts should be designed to prevent re-entrancy attacks by limiting the number of external calls made within a single transaction.

moneyversed

medium

Inconsistent error message format in SoftVault

Summary

The error messages in the SoftVault contract have an inconsistent format, which could make it difficult for developers to quickly understand what went wrong in case an error occurs.

Vulnerability Detail

The error messages in the contracts use different formats. Some error messages are written as normal strings, while others are defined as constants using the string constant or bytes32 constant keywords. This inconsistency could make it harder for developers to quickly understand what went wrong when an error occurs.

Impact

Inconsistent error message formats could lead to confusion and errors in the code, as developers might have to spend additional time figuring out what went wrong. This could lead to longer development times and increased costs.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/vault/SoftVault.sol#L84

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/vault/SoftVault.sol#L110

Tool used

Manual Review

Recommendation

To ensure consistency in error messages, it is recommended to define error messages as constants using the string constant or bytes32 constant keywords. This will make it easier for developers to quickly understand what went wrong when an error occurs. It is also recommended to use the same format for all error messages in the contracts.

moneyversed

high

Lack of Access Control for CoreOracle.setRoutes

Summary

The setRoutes function in the CoreOracle contract allows the owner to set the oracle routes for tokens. However, there is no access control mechanism implemented in this function, which allows any user to set the oracle routes.

Vulnerability Detail

Currently, any user can call the setRoutes function in the CoreOracle contract to set the oracle routes for tokens. This can be exploited by malicious actors to set incorrect oracle routes or to change the oracle routes without proper authorization.

Impact

A malicious user could potentially manipulate the oracle routes to cause incorrect prices to be returned, leading to unexpected behavior in the system.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/oracle/CoreOracle.sol#L51

Tool used

Manual Review

Recommendation

Implement proper access control mechanisms for the setRoutes function, such as adding a modifier that restricts access to only authorized users. This would prevent unauthorized users from modifying the oracle routes, ensuring the security and integrity of the system.

darksnow

medium

setWithdrawVaultFeeWindow(...) function in ProtocolConfig.sol does not convert days input parameter in the corresponding uint value

Summary

setWithdrawVaultFeeWindow(...) function in ProtocolConfig.sol does not convert days input parameter in the corresponding uint value.

Vulnerability Detail

Your tests fail. It seems that you want to specify the number of days as parameter but the function did not convert that to uint256 corresponding value.

Test results:

Protocol Config
    ✔ owner should be able to start vault withdraw fee
    1) owner should be able to set vault withdraw fee window
    ...
    Constructor
        ✔ should revert initializing twice
        ✔ should revert when treasury address is invalid
        1) should set initial states on constructor

Impact

This may leed to unexpected behaviour if the user expect to set the parameter in days.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/ProtocolConfig.sol#L99-L107

Tool used

Manual Review

Recommendation

Consider this changes. In ProtocolConfig.sol:

/**
* @dev Owner priviledged function to set withdraw vault fee window 
*/
function setWithdrawVaultFeeWindow(uint256 withdrawVaultFeeWindow_) external onlyOwner {
    // Cap to 60 days
    if (withdrawVaultFeeWindow_ * 1 days > Constants.MAX_WITHDRAW_VAULT_FEE_WINDOW) {
        revert Errors.FEE_WINDOW_TOO_LONG(withdrawVaultFeeWindow_);
    }
    withdrawVaultFeeWindow = withdrawVaultFeeWindow_ * 1 days;
}

In config.test.ts:

it("owner should be able to set vault withdraw fee window", async () => {
    await expect(
        config.connect(alice).setWithdrawVaultFeeWindow(90)
    ).to.be.revertedWith("Ownable: caller is not the owner");
    await expect(
        config.setWithdrawVaultFeeWindow(120)
    ).to.be.revertedWith("FEE_WINDOW_TOO_LONG");

    await config.setWithdrawVaultFeeWindow(60); // changed from 90 to 60 because the max value is MAX_WITHDRAW_VAULT_FEE_WINDOW = 60 days;
    expect(await config.withdrawVaultFeeWindow()).to.be.equal(60 * 24 * 60 * 60);
})

it("should set initial states on constructor", async () => {
    expect(await config.depositFee()).to.be.equal(50);
    expect(await config.withdrawFee()).to.be.equal(50);
    expect(await config.treasuryFeeRate()).to.be.equal(3000);
    expect(await config.blbStablePoolFeeRate()).to.be.equal(3500);
    expect(await config.blbIchiVaultFeeRate()).to.be.equal(3500);
    expect(await config.withdrawVaultFee()).to.be.equal(100);
    expect(await config.withdrawVaultFeeWindow()).to.be.equal(60 * 24 * 60 * 60); // changed here
    expect(await config.withdrawVaultFeeWindowStartTime()).to.be.equal(0);
})

coffiasd

medium

mapping(address⇒bool) using bool for storage incurs overhead

Summary

use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), if you don't use boolean for storage you will avoid Gwarmaccess 100 gas. In addition, state changes of boolean from true
 to false can cost up to ~20000 gas rather than uint256(2) to uint256(1) that would cost significantly less

Vulnerability Detail

Impact

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L66#L69

Tool used

Manual Review

Recommendation

use uint256(1) and uint256(2) for true/false

coffiasd

medium

.length Should Not Be Looked Up In Every Loop Of A For-loop

Summary

.length Should Not Be Looked Up In Every Loop Of A For-loop

Vulnerability Detail

Impact

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L175
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L193
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L211
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L226
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L315
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L411

Tool used

Manual Review

Recommendation

assign .length to local variable

cducrest-brainbot

medium

Racing condition in between repaying and liquidating a debt when repay was not allowed

Summary

The selected fix to sherlock-audit/2023-02-blueberry-judging#290 was to prevent liquidation when repay is not allowed. This introduces a racing condition when repay is not allowed, positions become liquidateable, and repay is again allowed. Liquidators and position owners will race to repay / liquidate the position.

Vulnerability Detail

Both liquidation and repayment are not allowed when the admin of BlueBerryBank disables repayment:

    function liquidate(
        uint256 positionId,
        address debtToken,
        uint256 amountCall
    ) external override lock poke(debtToken) {
        if (!isRepayAllowed()) revert Errors.REPAY_NOT_ALLOWED();
        ...
    }

    function repay(
        address token,
        uint256 amountCall
    ) external override inExec poke(token) onlyWhitelistedToken(token) {
        if (!isRepayAllowed()) revert Errors.REPAY_NOT_ALLOWED();
        ...
    }

There is no means to allow repayment and prevent liquidation for a time.

Impact

While repayment is not allowed, positions can become liquidateable. When admin re-enable repayments, the owner of the position risks liquidation without having a chance to repay its position if liquidator are faster to the chain than repayer. This is highly likely as liquidator are probably automated tools while repayers are human users.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/BlueBerryBank.sol#L487-L492

https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/BlueBerryBank.sol#L718-L722

Tool used

Manual Review

Recommendation

Allow reactivation of repayments without activating liquidation to give time to users to repay their position and not be liquidated. Alternatively do not disable repayments.

Duplicate of #117

n1punp

high

No slippage control when closing position in CurveSpell

Summary

No slippage control when closing position in CurveSpell

Vulnerability Detail

No slippage control when closing position in CurveSpell, specifically remove_liquidity_one_coin can output almost 0 tokens, if the attacker sandwich attack so the desired is very very expensive (low liquidity in the underlying pool).

Impact

Attacker can sandwich attack users when they want to close position in CurveSpell, leading to users getting nothing out (0 tokens).

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L143-L223

Tool used

Manual Review

Recommendation

• Add slippage control for all tokens that are involved with swapping (including reward token CRV).

Duplicate of #124

BugHunter101

medium

HardVault.sol ,the function deposit() does not check the token

Summary

In HardVault.sol ,the function deposit() does not check the token.if token is a malicious token contract ,it could tranfer a large number token to HardVault.

Vulnerability Detail

Overall, this function appears to be secure, but it should be noted that if token is a malicious token contract, it could tranfer a large number token to HardVault. Therefore, we should ensure that only trusted token contract addresses are accepted as parameters.

Impact

Affect the normal use of functions

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/vault/HardVault.sol#L105

function deposit(
    address token,
    uint256 amount
) external override nonReentrant returns (uint256 shareAmount) {
    if (amount == 0) revert Errors.ZERO_AMOUNT();
    IERC20Upgradeable uToken = IERC20Upgradeable(token);
    uint256 uBalanceBefore = uToken.balanceOf(address(this));
    uToken.safeTransferFrom(msg.sender, address(this), amount);
    uint256 uBalanceAfter = uToken.balanceOf(address(this));

    shareAmount = uBalanceAfter - uBalanceBefore;
    _mint(msg.sender, uint256(uint160(token)), shareAmount, "");

    emit Deposited(msg.sender, amount, shareAmount);
}

Tool used

Manual Review

Recommendation

we should ensure that only trusted token contract addresses are accepted as parameters, and use the whitelist.

ravikiran.web3

medium

Spell accepts multiple instances of the same strategy

Summary

BasicSpell's addStrategy() does not validate if the vault was already listed as part of strategies under this spell.

As such, it is possible to violate the validation of MaxPositionSize since, strategyId could be different with different max position, while the balance of token for the spell contract will be sum of multiple instances of strategies.

Vulnerability Detail

Example:
Lets say strategyA = { vault:"0x0aaab...", maxPositionSize: 100}
strategyId1 = AuraSpell.addStrategy(strategyA.vault, strategyA.maxPositionSize);

now, strategyA2 = { vault:"0x0aaab...", maxPositionSize: 125 }
strategyId2 = AuraSpell.addStrategy(strategyA2.vault, strategyA2.maxPositionSize);

EOA1 use strategyId1 to add a position for 55;
spellBalance = strategyA.vault.balanceOf(address(this));
console.log(spellBalance) = 55;

EOA2 use strategyId2 to add a position for 95;
spellBalance = strategyA.vault.balanceOf(address(this));
console.log(spellBalance) = 55 + 95 = 150;

Impact

using the two different strategy ids, the spell balance went above maxPositionSize causing issues in almost every spell in openPositionFarm() function. New Open positions in the spell may start to fail.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/BasicSpell.sol#L36-L39

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/BasicSpell.sol#L198-L207

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/AuraSpell.sol#L127

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/ConvexSpell.sol#L125

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L121

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/IchiSpell.sol#L108

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/ShortLongSpell.sol#L105

Tool used

Manual Review

Recommendation

existingStrategy modifier should be based on vault address

or in the _addStrategy call, check if the vault address is already existing in strategies array before adding it to the strategy list. Add a new strategy only if the vault is not found in all the strategies added to the current spell contract.

n1punp

high

Calculation underflow/overflow in BalancerPairOracle, which will affect pools in Aura Finance

Summary

LP price calculation for Balancer Pair in BalancerPairOracle will produce calculation underflow/overflow (so Aura pools won't work too).

Vulnerability Detail

• The values r0, r1 can underflow, e.g. if resA < resB --> r0 = 0, so it'll go to the else case --> and so ratio will be 0 --> fairResA calculation will revert upon dividing by 0.
• There are also other math calculations there that will cause reverts, e.g. ratio ** wB will lead to overflow. What you'd need here is Balancer's implementation of bpow or similar.

Impact

LP price for Balancer-like collateral token will revert in most cases, if not all.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/oracle/BalancerPairOracle.sol#L53-L64

Tool used

Manual Review

Recommendation

• Change the calculation logic so it aligns with Alpha's original implementation (with precision control), e.g. https://github.com/AlphaFinanceLab/alpha-homora-v2-contract/blob/master/contracts/oracle/BalancerPairOracle.sol#L42-L53 (you can see there's BONE extra precision in each step)

nobody2018

medium

Liquidation will fail in certain scenario

Summary

When an external protocol adds a new reward token, oracle does not support getting the price of this token. This will cause  BlueBerryBank#getPositionValue to revert, causing all transactions that call this function to fail.

Vulnerability Detail

BlueBerryBank#getPositionValue is used to calculate the value of a certain position, including collateral token and reward tokens of external protocols. It uses the IERC20Wrapper#pendingRewards function to get the reward token array of the external protocol, and then get  the price of each reward token by CoreOracle#getTokenValue.

function getPositionValue(
        uint256 positionId
    ) public view override returns (uint256 positionValue) {
        ...
        } else {
            ...

            uint rewardsValue;
->          (address[] memory tokens, uint256[] memory rewards) = IERC20Wrapper(
                pos.collToken
            ).pendingRewards(pos.collId, pos.collateralSize);
            for (uint256 i; i < tokens.length; i++) {
->              rewardsValue += oracle.getTokenValue(tokens[i], rewards[i]);
            }

            return collValue + rewardsValue;
        }
    }

CoreOracle#getTokenValue internally calls the _getPrice function, which checks whether the value of routes[token] is non-zero.

function _getPrice(
        address token
    ) internal view whenNotPaused returns (uint256) {
        address route = routes[token];
->      if (route == address(0)) revert Errors.NO_ORACLE_ROUTE(token);
        uint256 px = IBaseOracle(route).getPrice(token);
        if (px == 0) revert Errors.PRICE_FAILED(token);
        return px;
    }

Obviously, a new reward token has no route. Eventually BlueBerryBank#getPositionValue will revert. Therefore, functions that call this function will revert. Here I give the flow of the two functions:

• BlueBerryBank#liquidate->isLiquidatable->getPositionRisk->getPositionValue->oracle.getTokenValue
• BlueBerryBank#execute->isLiquidatable->getPositionRisk->getPositionValue->oracle.getTokenValue

Impact

Both BlueBerryBank#liquid and BlueBerryBank#execute will be affected. Due to the importance of liquidation time, untimely liquidation can result in financial losses for both parties involved in the liquidation.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L408-L413

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/oracle/CoreOracle.sol#L74

Tool used

Manual Review

Recommendation

--- a/blueberry-core/contracts/BlueBerryBank.sol
+++ b/blueberry-core/contracts/BlueBerryBank.sol
@@ -409,7 +409,9 @@ contract BlueBerryBank is
                 pos.collToken
             ).pendingRewards(pos.collId, pos.collateralSize);
             for (uint256 i; i < tokens.length; i++) {
-                rewardsValue += oracle.getTokenValue(tokens[i], rewards[i]);
+                if (oracle.isTokenSupported(tokens[i])) {
+                    rewardsValue += oracle.getTokenValue(tokens[i], rewards[i]);
+                }
             }

Duplicate of #115

moneyversed

high

AuraSpell can be compromised by a reentrancy attack

Summary

AuraSpell uses the joinPool() function from BalancerVault, which can lead to a reentrancy vulnerability. The joinPool() function can be called by an attacker to recursively call into the AuraSpell contract and manipulate the state of the contract to the attacker's advantage.

Vulnerability Detail

The joinPool() function used in AuraSpell can execute arbitrary external contract code through the IERC20.approve() and IERC20.transferFrom() functions. This creates the possibility of reentrancy attack, which occurs when an external contract calls a vulnerable contract's function repeatedly before the first invocation is finished.

For instance, if an attacker calls the openPositionFarm() function while also calling an external contract's fallback function that calls joinPool(), the attacker can use the approved tokens to manipulate the state of the AuraSpell contract in its favor.

Impact

An attacker can use a reentrancy attack to drain funds from the contract or lock up assets indefinitely, causing a Denial of Service (DoS) attack, making the system unusable.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/AuraSpell.sol#L108

Tool used

Manual Review

Recommendation

The AuraSpell contract should be updated to prevent reentrancy attacks by employing the checks-effects-interactions pattern. The pattern can be used to separate checks from interaction with external contracts and prevent reentrancy attacks. An option to consider is the use of the OpenZeppelin ReentrancyGuard library, which can be used to mitigate reentrancy attacks. Alternatively, the contract can be updated to use the transferFrom() and safeTransferFrom() functions from the OpenZeppelin library instead of the approve() and transfer() functions.

Bauer

high

The protocol does not return all of the rewards to user

Summary

When user calls the openPositionFarm() function to add liquidity to Balancer pool, with staking to Aura, if user has a collateral and the protocol does not return all of the rewards to user upon withdrawal.

Vulnerability Detail

The AuraSpell.openPositionFarm() function allows users to open a new position on a specific farming pool using collateral and borrowed tokens. The function follows a series of steps to deposit collateral, borrow tokens, add liquidity to a Balancer pool, and validate the maximum loan-to-value (LTV) and position size. Inside the function,it takes out any existing collateral and burns the corresponding pool tokens.

        // 6. Take out existing collateral and burn
        IBank.Position memory pos = bank.getCurrentPositionInfo();
        if (pos.collateralSize > 0) {
            (uint256 pid, ) = wAuraPools.decodeId(pos.collId);
            if (param.farmingPoolId != pid)
                revert Errors.INCORRECT_PID(param.farmingPoolId);
            if (pos.collToken != address(wAuraPools))
                revert Errors.INCORRECT_COLTOKEN(pos.collToken);
            bank.takeCollateral(pos.collateralSize);
            wAuraPools.burn(pos.collId, pos.collateralSize);
            _doRefundRewards(AURA);
        }

For the WAuraPools.burn() function ,it retrieves any pending rewards for the burned tokens by calling the "pendingRewards" function and transfers the corresponding reward tokens and amounts back to the AuraSpell protocol using a loop that iterates over the reward tokens and calls the "safeTransfer" function.

function burn(
        uint256 id,
        uint256 amount
    )
        external
        nonReentrant
        returns (address[] memory rewardTokens, uint256[] memory rewards)
    {
        if (amount == type(uint256).max) {
            amount = balanceOf(msg.sender, id);
        }
        (uint256 pid, ) = decodeId(id);
        _burn(msg.sender, id, amount);

        (address lpToken, , , address balRewarder, , ) = getPoolInfoFromPoolId(
            pid
        );
        // Claim Rewards
        IAuraRewarder(balRewarder).withdraw(amount, true);
        // Withdraw LP
        auraPools.withdraw(pid, amount);

        // Transfer LP Tokens
        IERC20Upgradeable(lpToken).safeTransfer(msg.sender, amount);

        // Transfer Reward Tokens
        (rewardTokens, rewards) = pendingRewards(id, amount);

        for (uint i = 0; i < rewardTokens.length; i++) {
            IERC20Upgradeable(rewardTokens[i]).safeTransfer(
                msg.sender,
                rewards[i]
            );
        }
    }

However, the AuraSpell protocol only refunds any earned rewards in AURA tokens by calling the "_doRefundRewards" function.

Impact

The reward tokens were not returned to the user

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/AuraSpell.sol#L131-L140

Tool used

Manual Review

Recommendation

Swap rewards tokens to corresponding token and reward to user.

Duplicate of #101

moneyversed

medium

Incorrect Usage of SafeMath in BBMath Library

Summary

The BBMath library defines a divCeil function to perform division with the ceil rounding. However, the function uses an incorrect implementation of SafeMath that may not guarantee the correct result in some edge cases.

Vulnerability Detail

The divCeil function implementation in BBMath library can be simplified as:

function divCeil(uint256 a, uint256 b) internal pure returns (uint256) {
    return (a + b - 1) / b;
}

As the function uses SafeMath to prevent integer overflow, it should be implemented as:

function divCeil(uint256 a, uint256 b) internal pure returns (uint256) {
    require(b > 0, "BBMath: division by zero");
    return (a + b - 1) / b;
}

This is because the (a + b - 1) term can overflow when a is close to the maximum value and b is 1. When this happens, the result of the division would be incorrect, and it may cause unexpected behavior.

Impact

An attacker could manipulate the input values of divCeil function to force the incorrect calculation and result in unexpected behavior.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/libraries/BBMath.sol#L13

Tool used

Manual Review

Recommendation

Would be recommended modifying the divCeil implementation by using the SafeMath library to ensure that no integer overflow occurs during the operation.

Bauer

high

Dos attack to openPositionFarm()

Summary

A bad actor can transfer 1 wei worth of the corresponding token to the protocol before user calling the openPositionFarm() function, in order to increase the protocol's balance and build an LP position to call ICurvePool(pool).add_liquidity(), since the protocol only allows the curvel pool to spend the borrowed token, this will cause an error when Curve attempts to transfer other tokens out of the protocol.

Vulnerability Detail

The openPositionFarm() function is used to add liquidity to Curve pool with 2 underlying tokens, with staking to Curve gauge. When add liquidity on curve ,the protocol use the borrowed token and the collateral token, it checks the number of tokens in the pool and creates an array of the supplied token amounts to be passed to the add_liquidity function.If the pool contains three tokens, the process is repeated with an array of three elements, and if the pool contains four tokens, an array of four elements is created and used. Here is the problem,a bad actor may transfer 1 wei worth of the corresponding token to the protocol before user calling the openPositionFarm() function, in order to increase the protocol's balance and build an LP position to call ICurvePool(pool).add_liquidity(). However, since the protocol only allows the curvel pool to spend the borrowed token, this will cause an error when Curve attempts to transfer other tokens out of the protocol.

  uint256 borrowBalance = _doBorrow(
            param.borrowToken,
            param.borrowAmount
        );

        // 3. Add liquidity on curve
        _ensureApprove(param.borrowToken, pool, borrowBalance);
        if (tokens.length == 2) {
            uint256[2] memory suppliedAmts;
            for (uint256 i = 0; i < 2; i++) {
                suppliedAmts[i] = IERC20Upgradeable(tokens[i]).balanceOf(
                    address(this)
                );
            }
            ICurvePool(pool).add_liquidity(suppliedAmts, minLPMint);
        } else if (tokens.length == 3) {
            uint256[3] memory suppliedAmts;
            for (uint256 i = 0; i < 3; i++) {
                suppliedAmts[i] = IERC20Upgradeable(tokens[i]).balanceOf(
                    address(this)
                );
            }
            ICurvePool(pool).add_liquidity(suppliedAmts, minLPMint);
        } else if (tokens.length == 4) {
            uint256[4] memory suppliedAmts;
            for (uint256 i = 0; i < 4; i++) {
                suppliedAmts[i] = IERC20Upgradeable(tokens[i]).balanceOf(
                    address(this)
                );
            }
            ICurvePool(pool).add_liquidity(suppliedAmts, minLPMint);
        }

Impact

User will not able to call the openPositionFarm() function to add liquidity to Curve pool

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L84-L115

Tool used

Manual Review

Recommendation

tallo

medium

Chainlink oracle makes no check to see if the Arbitrum sequencer is down

Summary

When working with chainlink in L2's its important to remember the centralization risk of the sequencer. If the sequencer is down, which has has suffered outages of over 10 hours in the past, then users will be served falsely fresh but incorrect prices.

Vulnerability Detail

    function getPrice(address token_) external view override returns (uint256) {

        //..

        //@audit The 'answer' returned by latestRoundData will return the most recent value BEFORE the sequencer went down
        (, int256 answer, , uint256 updatedAt, ) = registry.latestRoundData(
            token,
            USD
        );

        if (updatedAt < block.timestamp - maxDelayTime)
            revert Errors.PRICE_OUTDATED(token_);
        if (answer <= 0) revert Errors.PRICE_NEGATIVE(token_);

        return
            (answer.toUint256() * Constants.PRICE_PRECISION) / 10 ** decimals;
    }

Impact

The price recorded by latestRoundData will be inaccurate since the true price won't be reported to the chain due to the sequence being down. This could lead to users being reported an outdated price.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/oracle/ChainlinkAdapterOracle.sol#L77

Tool used

Manual Review

Recommendation

Follow the chainlink docs for dealing with evm chains. The docs suggest to query the sequenceUptimeFeed to get an answer on whether the the sequencer is up or down
https://docs.chain.link/data-feeds/l2-sequencer-feeds#handling-outages-on-optimism-and-metis

Additional resources

https://thedefiant.io/arbitrum-outage-2
sherlock-audit/2023-02-bond-judging#1

Duplicate of #142

darksnow

medium

getPrice(...) function in ChainlinkAdapterOracle.sol can cause a DoS

Summary

getPrice(...) function in ChainlinkAdapterOracle.sol can cause a DoS caused by ChainLink oracle external call.

Vulnerability Detail

As mentioned here it is possible that Chainlink’s multisigs can immediately block access to price feeds at will.

Impact

In this case it will cause a DoS with current implementation.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/oracle/ChainlinkAdapterOracle.sol#L86-L90

Tool used

Manual Review

Recommendation

To prevent denial of service scenarios, it is recommended to query ChainLink price feeds using a defensive approach with a try/catch structure.

function getPrice(address token_) external view override returns (uint256) {
    // remap token if possible
    address token = remappedTokens[token_];
    if (token == address(0)) token = token_;

    uint256 maxDelayTime = timeGaps[token];
    if (maxDelayTime == 0) revert Errors.NO_MAX_DELAY(token_);

    // Get token-USD price
    uint256 decimals;
    int256 answer;
    try registry.decimals(token, USD) returns (uint256 _decimals) {
        decimals = _decimals;
    } catch Error(string memory error) { 
        // handle failure here
     };
    try registry.latestRoundData(
        token,
        USD
    ) returns (
        uint80 roundId,
        int256 _answer,
        uint256 startedAt,
        uint256 updatedAt,
        uint80 answeredInRound) {
        if (updatedAt < block.timestamp - maxDelayTime)
            revert Errors.PRICE_OUTDATED(token_);
        if (_answer <= 0) revert Errors.PRICE_NEGATIVE(token_);
        answer = _answer;
    } catch Error(string memory error) { 
        // handle failure here
     };

    return
        (answer.toUint256() * Constants.PRICE_PRECISION) / 10 ** decimals;
}

Duplicate of #77

Bauer

high

AuraSpell executes swaps without slippage protection

Summary

The AuraSpell protocol executes swaps without slippage protection. That will cause a loss of funds because of sandwich attacks.

Vulnerability Detail

In AuraSpell .closePositionFarm() swaps are executed through the swapRouter.The amountOutMin value has been set to 0. Without slippage protection, this is vulnerable to sandwich attacks

for (uint256 i = 0; i < rewardTokens.length; i++) {
            uint256 rewards = _doCutRewardsFee(rewardTokens[i]);
            _ensureApprove(rewardTokens[i], address(swapRouter), rewards);
            swapRouter.swapExactTokensForTokens(
                rewards,
                0,
                swapPath[i],
                address(this),
                type(uint256).max
            );
        }

Impact

Swaps will be sandwiched causing a loss of funds for users you withdraw their rewards.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/AuraSpell.sol#L196-L202

Tool used

Manual Review

Recommendation

Calculate the maximum output and set slippage protection

Duplicate of #121

ravikiran.web3

high

IchiVaultOracle is using a mockup instead of the correct library

Summary

IchiVaultOracle contract is referring to "UniV3WrappedLibMockup" instead of "UniV3WrappedLib".
The mock up does not implement any of the functions, they are dummy implementation.

Vulnerability Detail

Example: check spotPrice0InToken1() in the IchiVaultOracle contract which points to UniV3WrappedLibMockup.getQuoteAtTick().

If you check the implementation in the mock up contract, the below is the implementation.

function getQuoteAtTick(
int24 tick,
uint128 baseAmount,
address baseToken,
address quoteToken
) external pure returns (uint256 quoteAmount) {}

The contract is pointing to dummy implementation which will produce un expected results.

Impact

None of the functionality related to IchiVaultOracle is expected to work properly as there is no implemetation in the mockup and even if it was there, it was a static data and does not meet the real business scenarios. This will imply incorrect function of business logic and could lead to economic loss.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/oracle/IchiVaultOracle.sol#L60-L76

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/oracle/IchiVaultOracle.sol#L78-L103

Implementation for Mock up
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/libraries/UniV3/UniV3WrappedLibMockup.sol#L5-L48

Tool used

Manual Review

Recommendation

The logics in the IchiVaultOracle contract should point to "UniV3WrappedLib" instead of "UniV3WrappedLibMockup".
After pointing to the correct lib, proper testing needs to be done to make sure the business logic is working as expected.

cducrest-brainbot

high

IchiVaultOracle getPrice will fail during price crashes

Summary

The function to get the price of the IchiVault LP token will fail when TWAP and spot price differ too much. This will be the case when price of the LP token crashes.

The oracle will revert and positions using IchiVault LP token won't be able to be repaid / liquidated when price crashes, when this is needed the most.

Vulnerability Detail

The function getPrice() checks the spot and twap price of the LP token, it reverts when they differ too much:

    function getPrice(address token) external view override returns (uint256) {
        IICHIVault vault = IICHIVault(token);
        uint256 totalSupply = vault.totalSupply();
        if (totalSupply == 0) return 0;

        address token0 = vault.token0();
        address token1 = vault.token1();

        // Check price manipulations on Uni V3 pool by flashloan attack
        uint256 spotPrice = spotPrice0InToken1(vault);
        uint256 twapPrice = twapPrice0InToken1(vault);
        uint256 maxPriceDeviation = maxPriceDeviations[token0];
        if (!_isValidPrices(spotPrice, twapPrice, maxPriceDeviation))
            revert Errors.EXCEED_DEVIATION();

        // Total reserve / total supply
        (uint256 r0, uint256 r1) = vault.getTotalAmounts();
        uint256 px0 = base.getPrice(address(token0));
        uint256 px1 = base.getPrice(address(token1));
        uint256 t0Decimal = IERC20Metadata(token0).decimals();
        uint256 t1Decimal = IERC20Metadata(token1).decimals();

        uint256 totalReserve = (r0 * px0) /
            10 ** t0Decimal +
            (r1 * px1) /
            10 ** t1Decimal;

        return (totalReserve * 10 ** vault.decimals()) / totalSupply;
    }

Impact

The twap period can range from 1 hour to 2 days. If the twap period is long enough or the maxPriceDeviations small enough, the pricing oracle will revert when price crashes (or spikes) and prevent actions on positions using IchiVault LP tokens.

The liquidation / repayment will be impossible when most needed, during important changes of market prices.

As noted in the comment and understood from the code, the intent of this check is to prevent price manipulation of the LP token. If the maxPriceDeviations, the price oracle is vulnerable to price manipulation since it does not the fair LP token pricing method.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/oracle/IchiVaultOracle.sol#L110-L138

Tool used

Manual Review

Recommendation

Use the fair lp token pricing strategy instead of checking twap and spot price.

dacian

high

Borrower can't repay but can be liquidated as token whitelist can prevent existing positions from repaying

Summary

Borrower can't repay but can be liquidated as token whitelist can prevent existing positions from repaying.

Vulnerability Detail

BlueBerryBank.repay() has onlyWhitelistedToken() modifier, while BlueBerryBank.liquidate() does not; both end up calling _repay(). If Borrower has an existing position and then the token is removed from the whitelist, Borrower is unable to repay but can still be liquidated.

Impact

Borrower with existing position can't repay their loan but can be liquidated - this severely disadvantages the Borrower guaranteeing their liquidation with no possibility to repay.

Code Snippet

BlueBerryBank.liquidate() L487-491 vs BlueBerryBank.repay() L718-721

Tool used

Weaponized Autism (I read through every single c4/sherlock lending/borrowing contest and examined every single high/medium finding, since the beginning. Kinda crazy right?)

Recommendation

First please consider Repayments Paused While Liquidations Enabled from BlueBerry's first audit finding. BlueBerry addressed this issue by having liquidate() call isRepayAllowed() L492

However the same state can also be reached due to the inconsistent use of onlyWhitelistedToken() modifier between repay() and liquidate(). So one potential fix is to have liquidate() also use onlyWhitelistedToken() modifier, therefore at least if the Borrower can't repay, they also can't be liquidated.

Now secondly please consider Collateral Pause Stops Existing Repayment & Liquidation, a high finding from Sherlock's Isomorph Audit. In this audit it was judged that if governance disallowed a previously allowed token and if this causes existing positions to not be able to be repaid & liquidated, this was also a high finding, as governance disallowing a token should only apply to new positions, but existing positions should be allowed to continue to be repaid and liquidated, even if the token is no longer approved by governance.

So ideally neither repay() nor liquidate() would have onlyWhitelistedToken() - this is fair to all market participants and is the most consistent fix in line with the precedent set by the judging in Sherlock's Isomorph audit. I have submitted as High since that is what Sherlock's Isomorph audit classified the same bug. If my submission is downgraded to medium, kindly please explain why the same issue was High in Isomorph but is only medium here.

My submission actually combines 2 distinct issues which have been recognized separately in previous Sherlock competitions:

• Borrower can't repay but can be liquidated
• Governance token disallow prevents existing positions from repay (and in other contests from liquidation)

However because the primary goal of the audit is to benefit the sponsor, and because the ideal solution (remove onlyWhitelistedToken() from repay()) resolves both issues, I have combined them into this single issue to keep all discussion concentrated in the one place. I do hope that this won't disadvantage me in judging, and at the very least combining both issues into one submission should uphold this submission as a high finding.

nobody2018

high

spell#closePositionFarm executes swapExactTokensForTokens without slippage protection

Summary

AuraSpell/ConvexSpell/CurveSpell#closePositionFarm executes swaps without slippage protection. That will cause a loss of funds because of sandwich attacks.

Vulnerability Detail

As we all know, if the second parameter amountOutMin of the UniswapV2Router02#swapExactTokensForTokens function is set to 0, it is vulnerable to sandwich attacks. AuraSpell/ConvexSpell/CurveSpell#closePositionFarm will internally swap reward tokens into debt tokens for repayment.

//AuraSpell.sol
    // 4. Swap rewards tokens to debt token
        for (uint256 i = 0; i < rewardTokens.length; i++) {
            uint256 rewards = _doCutRewardsFee(rewardTokens[i]);
            _ensureApprove(rewardTokens[i], address(swapRouter), rewards);
            swapRouter.swapExactTokensForTokens(
                rewards,
->              0,		
                swapPath[i],
                address(this),
                type(uint256).max
            );
        }

Impact

swapExactTokensForTokens will be sandwiched causing a loss of funds for users.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/AuraSpell.sol#L196-L202

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/ConvexSpell.sol#L174-L180

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L167-L173

Tool used

Manual Review

Recommendation

Slippage parameters should be included in the tx's calldata and passed to swapExactTokensForTokens.

Duplicate of #121

BugHunter101

high

Function accrue() does not check if the caller of the function is authorized to trigger the interest accrual for the given bank

Summary

The accrue function in the provided code block has a potential security issue. Specifically, it does not check if the caller of the function is authorized to trigger the interest accrual for the given bank. This means that any external account can call this function and trigger the accrual of interest for any bank, which could potentially lead to unauthorized access to funds.

Vulnerability Detail

The accrue function in the provided code block has a potential security issue. Specifically, it does not check if the caller of the function is authorized to trigger the interest accrual for the given bank. This means that any external account can call this function and trigger the accrual of interest for any bank, which could potentially lead to unauthorized access to funds.

Impact

which could potentially lead to unauthorized access to funds.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L306

function accrue(address token) public override {
    Bank storage bank = banks[token];
    if (!bank.isListed) revert Errors.BANK_NOT_LISTED(token);
    ICErc20(bank.bToken).borrowBalanceCurrent(address(this));
}

Tool used

Manual Review

Recommendation

Here's an example implementation of the onlyBankOwner modifier:

modifier onlyBankOwner(address token) {
require(msg.sender == banks[token].owner, "Only bank owner can trigger interest accrual");
_;
}

You could then modify the accrue function to include this modifier:

function accrue(address token) public override onlyBankOwner(token) {
Bank storage bank = banks[token];
if (!bank.isListed) revert Errors.BANK_NOT_LISTED(token);
ICErc20(bank.bToken).borrowBalanceCurrent(address(this));
}

BugHunter101

high

takeCollateral() doesn't check whether the caller actually has a corresponding debt position in the bank

Summary

this function is that it does not check whether the caller actually has a corresponding debt position in the bank. This means that an attacker could potentially call this function with a large amount value to drain the bank's collateral without actually having any debt to repay.

Vulnerability Detail

this function is that it does not check whether the caller actually has a corresponding debt position in the bank. This means that an attacker could potentially call this function with a large amount value to drain the bank's collateral without actually having any debt to repay.

Impact

an attacker could potentially call this function with a large amount value to drain the bank's collateral without actually having any debt to repay.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L793

function takeCollateral(
uint256 amount
) external override inExec returns (uint256) {
Position storage pos = positions[POSITION_ID];
if (amount == type(uint256).max) {
amount = pos.collateralSize;
}
pos.collateralSize -= amount;
IERC1155Upgradeable(pos.collToken).safeTransferFrom(
address(this),
msg.sender,
pos.collId,
amount,
""
);
emit TakeCollateral(
POSITION_ID,
msg.sender,
pos.collToken,
pos.collId,
amount
);

    return amount;
}

Tool used

Manual Review

Recommendation

In this implementation, the function first checks whether the caller has a corresponding debt position by verifying that the debtShare value is non-zero. If it is zero, then the function reverts with an error message indicating that the caller does not have a debt position. If the debtShare value is non-zero, then the function proceeds with transferring the requested amount of collateral back to the caller.

example:

function takeCollateral(uint256 amount) external override inExec returns (uint256) {
Position storage pos = positions[POSITION_ID];
if (amount == type(uint256).max) {
amount = pos.collateralSize;
}
if (pos.debtShare == 0) {
revert Errors.NO_DEBT_POSITION();
}
pos.collateralSize -= amount;
IERC1155Upgradeable(pos.collToken).safeTransferFrom(
address(this),
msg.sender,
pos.collId,
amount,
""
);
emit TakeCollateral(
POSITION_ID,
msg.sender,
pos.collToken,
pos.collId,
amount
);

return amount;

}

moneyversed

high

Lack of input validation for the amountCall parameter in the _doERC20TransferIn and _doERC1155TransferIn functions

Summary

The _doERC20TransferIn and _doERC1155TransferIn functions do not validate the amountCall parameter, which may lead to unexpected behavior and potential vulnerabilities.

Vulnerability Detail

Both functions calculate the amount actually received by subtracting the balance before and after the transfer. However, there is no check for the amountCall being zero or excessively large, which could lead to incorrect calculations and unexpected contract behavior.

Impact

An attacker could potentially exploit this lack of input validation to manipulate contract state, leading to incorrect calculations or vulnerabilities in other functions.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L862

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L884

Tool used

Manual Review

Recommendation

Add input validation for the amountCall parameter in both functions to ensure it is within an acceptable range and not zero.

BugHunter101

high

The burn function in the WAuraPools contract appears to be vulnerable to a reentrancy attack.

Summary

The burn function in the WAuraPools contract appears to be vulnerable to a reentrancy attack.

Vulnerability Detail

The function first calls IAuraRewarder(balRewarder).withdraw(amount, true) to claim rewards, then auraPools.withdraw(pid, amount) to withdraw LP tokens, and finally transfers reward tokens and LP tokens to the caller. However, if the withdraw function in IAuraRewarder or the withdraw function in auraPools calls back into the WAuraPools contract, the burn function can be reentered before the transfer of tokens is complete, allowing an attacker to potentially drain the contract of tokens.

Impact

appears to be vulnerable to a reentrancy attack.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/wrapper/WAuraPools.sol#L228

function burn(
uint256 id,
uint256 amount
)
external
nonReentrant
returns (address[] memory rewardTokens, uint256[] memory rewards)
{
if (amount == type(uint256).max) {
amount = balanceOf(msg.sender, id);
}
(uint256 pid, ) = decodeId(id);
_burn(msg.sender, id, amount);

    (address lpToken, , , address balRewarder, , ) = getPoolInfoFromPoolId(
        pid
    );
    // Claim Rewards
    IAuraRewarder(balRewarder).withdraw(amount, true);
    // Withdraw LP
    auraPools.withdraw(pid, amount);

    // Transfer LP Tokens
    IERC20Upgradeable(lpToken).safeTransfer(msg.sender, amount);

    // Transfer Reward Tokens
    (rewardTokens, rewards) = pendingRewards(id, amount);

    for (uint i = 0; i < rewardTokens.length; i++) {
        IERC20Upgradeable(rewardTokens[i]).safeTransfer(
            msg.sender,
            rewards[i]
        );
    }
}

Tool used

Manual Review

Recommendation

To fix this vulnerability, the burn function should be modified to follow the checks-effects-interactions pattern. Specifically, the function should first transfer the LP tokens and reward tokens to a temporary contract, then call IAuraRewarder(balRewarder).withdraw(amount, true) and auraPools.withdraw(pid, amount) to claim rewards and withdraw LP tokens, respectively. Finally, the function should transfer the LP tokens and reward tokens from the temporary contract to the caller. This ensures that the transfer of tokens is completed before any external calls are made, preventing reentrancy attacks.

Example:

function burn(
uint256 id,
uint256 amount
)
external
nonReentrant
returns (address[] memory rewardTokens, uint256[] memory rewards)
{
if (amount == type(uint256).max) {
amount = balanceOf(msg.sender, id);
}
(uint256 pid, ) = decodeId(id);
_burn(msg.sender, id, amount);

(address lpToken, , , address balRewarder, , ) = getPoolInfoFromPoolId(pid);

// Transfer LP Tokens and Reward Tokens to temporary contract
IERC20Upgradeable(lpToken).safeTransferFrom(msg.sender, address(this), amount);
(rewardTokens, rewards) = pendingRewards(id, amount);
for (uint i = 0; i < rewardTokens.length; i++) {
    IERC20Upgradeable(rewardTokens[i]).safeTransferFrom(msg.sender, address(this), rewards[i]);
}

// Claim Rewards and Withdraw LP
IAuraRewarder(balRewarder).withdraw(amount, true);
auraPools.withdraw(pid, amount);

// Transfer LP Tokens and Reward Tokens from temporary contract to caller
IERC20Upgradeable(lpToken).safeTransfer(msg.sender, amount);
for (uint i = 0; i < rewardTokens.length; i++) {
    IERC20Upgradeable(rewardTokens[i]).safeTransfer(msg.sender, rewards[i]);
}

}

Bauer

high

The quotes from Curve may be subject to manipulation

Summary

The get_virtual_price() function in Curve has a reentrancy risk, which can affect the price if the protocol fetches quotes from pools integrated with ETH on Curve.

Vulnerability Detail

The CurveOracle protocol calls the function get_virtual_price_from_lp_token() to obtain a quote from Curve. However, all pools integrated with ETH pose a read-only reentrancy risk. Please refer below link for detail.
https://chainsecurity.com/heartbreaks-curve-lp-oracles/

Impact

The read-only reentrancy operation manipulates the price.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/oracle/CurveOracle.sol#L101

Tool used

Manual Review

Recommendation

Duplicate of #123

n1punp

high

Missing slippage control validation in opening position function in AuraSpell

Summary

Missing slippage control validation in opening position function in AuraSpell

Vulnerability Detail

Users who try to open position in Aura pool via Blueberry can get sandwiched attack, leading to the user potentially getting 0 lp amount from swaps, and the tx will still succeed.

Impact

All your farmers to the Aura pool will get sandwiched and wrecked.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/AuraSpell.sol#L63-L147

Tool used

Manual Review

Recommendation

• Add slippage control for minLPAmount and check with the obtained lpAmount.

Duplicate of #121

Bauer

medium

auraPools.deposit and auraPools.withdraw boolean return value not handled in WAuraPools.sol

Summary

auraPools.deposit() and auraPools.withdraw() boolean return value not handled in WAuraPools.sol

Vulnerability Detail

The WAuraPools.mint() function allows users to deposit "amount" of a specific pool token, identified by "pid". The deposited tokens are then transferred from the user's address to the contract's address. The function also ensures that the contract is approved to spend the deposited tokens by calling the "_ensureApprove" function with the specified amount.
The deposit() function of the auraPools contract is then called to deposit the tokens into the specified pool.
However, the protocol does not handle the AuraPool.withdrawAndUnwrap() boolean return value.

 function mint(
        uint256 pid,
        uint256 amount
    ) external nonReentrant returns (uint256 id) {
        (address lpToken, , , address crvRewarder, , ) = getPoolInfoFromPoolId(
            pid
        );
        IERC20Upgradeable(lpToken).safeTransferFrom(
            msg.sender,
            address(this),
            amount
        );

        _ensureApprove(lpToken, address(auraPools), amount);
        auraPools.deposit(pid, amount, true);

        uint256 crvRewardPerToken = IAuraRewarder(crvRewarder).rewardPerToken();
        id = encodeId(pid, crvRewardPerToken);
        _mint(msg.sender, id, amount, "");
        // Store extra rewards info
        uint extraRewardsCount = IAuraRewarder(crvRewarder)
            .extraRewardsLength();
        for (uint i = 0; i < extraRewardsCount; i++) {
            address extraRewarder = IAuraRewarder(crvRewarder).extraRewards(i);
            uint rewardPerToken = IAuraRewarder(extraRewarder).rewardPerToken();
            accExtPerShare[id].push(rewardPerToken);
        }
    }

In the AuraBooster implmenetation, a Boolean is indeed returned to acknowledge that deposit is completely successfully.

https://etherscan.io/address/0x7818A1DA7BD1E64c199029E86Ba244a9798eEE10#code#F34#L1

  /**
     * @notice  Deposits an "_amount" to a given gauge (specified by _pid), mints a `DepositToken`
     *          and subsequently stakes that on Convex BaseRewardPool
     */
    function deposit(uint256 _pid, uint256 _amount, bool _stake) public returns(bool){

The same issue for auraPools.withdraw()

Impact

If the boolean value is not handled, the transaction may fail silently.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/wrapper/WAuraPools.sol#L209
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/wrapper/WAuraPools.sol#L248

Tool used

Manual Review

Recommendation

Recommend checking for success return value

  bool depositSuccess =   auraPools.deposit(pid, amount, true);
 require(depositSuccess , 'deposit failed');

moneyversed

high

Reentrancy vulnerability in IBank.sol

Summary

The IBank contract is vulnerable to reentrancy attacks. Specifically, the liquidate function can be called by an attacker multiple times before the state changes are finalized, allowing the attacker to withdraw more than they should.

Vulnerability Detail

The liquidate function calls the withdrawLend function, which is not protected against reentrancy. This means that an attacker can call the liquidate function multiple times before the state changes are finalized, allowing them to withdraw more than they should.

Impact

An attacker can exploit this vulnerability to steal funds from the contract.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/interfaces/IBank.sol#L166

Tool used

Manual Review

Recommendation

To fix this vulnerability, the withdrawLend function should be updated to include a reentrancy guard. A simple fix could be to add the nonReentrant modifier to the withdrawLend function, similar to the liquidate function. Alternatively, the withdrawLend function could be modified to use the Checks-Effects-Interactions pattern to avoid reentrancy attacks.

volodya

medium

withdraw can be paused by the system

Summary

In the docs, there is a statement

(Withdraw and repay of any market cannot be paused)

Vulnerability Detail

There is a way to pause a CoreOracle now, that will disable repaying user position, because in the bank there is a transaction for a price, but Oracle is disabled so it will be reverted.

    function _getPrice(
        address token
    ) internal view whenNotPaused returns (uint256) {
        address route = routes[token];
        if (route == address(0)) revert Errors.NO_ORACLE_ROUTE(token);
        uint256 px = IBaseOracle(route).getPrice(token);
        if (px == 0) revert Errors.PRICE_FAILED(token);
        return px;
    }

core/contracts/oracle/CoreOracle.sol#L72

Impact

Code Snippet

Tool used

Manual Review

Recommendation

Remove the pause feature.

Bauer

high

The protocol will not be able to add liquidity on the curve with another token with a balance.

Summary

The CurveSpell protocol only ensure approve curve pool to spend its borrow token. Hence, it will not be able to add liquidity on the curve with another token with a balance.

Vulnerability Detail

The openPositionFarm() function enables user to open a leveraged position in a yield farming strategy by borrowing funds and using them to add liquidity to a Curve pool, while also taking into account certain risk management parameters such as maximum LTV and position size. When add liquidity on curve ,the protocol use the borrowed token and the collateral token, it checks the number of tokens in the pool and creates an array of the supplied token amounts to be passed to the add_liquidity function. Then the curve will transfer the tokens from the protocol and mint lp tokens to the protocol. However, the protocol only ensure approve curve pool to spend its borrow token. Hence, it will not be able to add liquidity on the curve with another token with a balance.

 // 3. Add liquidity on curve
        _ensureApprove(param.borrowToken, pool, borrowBalance);
        if (tokens.length == 2) {
            uint256[2] memory suppliedAmts;
            for (uint256 i = 0; i < 2; i++) {
                suppliedAmts[i] = IERC20Upgradeable(tokens[i]).balanceOf(
                    address(this)
                );
            }
            ICurvePool(pool).add_liquidity(suppliedAmts, minLPMint);
        } else if (tokens.length == 3) {
            uint256[3] memory suppliedAmts;
            for (uint256 i = 0; i < 3; i++) {
                suppliedAmts[i] = IERC20Upgradeable(tokens[i]).balanceOf(
                    address(this)
                );
            }
            ICurvePool(pool).add_liquidity(suppliedAmts, minLPMint);
        } else if (tokens.length == 4) {
            uint256[4] memory suppliedAmts;
            for (uint256 i = 0; i < 4; i++) {
                suppliedAmts[i] = IERC20Upgradeable(tokens[i]).balanceOf(
                    address(this)
                );
            }
            ICurvePool(pool).add_liquidity(suppliedAmts, minLPMint);
        }

Impact

The protocol will not be able to add liquidity on the curve with another token with a balance.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L90-L115

Tool used

Manual Review

Recommendation

Allow the curve pool to spend tokens that have a balance in the protocol to add liquidity

moneyversed

high

Missing access controls for internal functions

Summary

Several internal functions in the contract do not have access controls, potentially allowing unauthorized access to sensitive contract operations.

Vulnerability Detail

The _doBorrow, _doRepay, _doERC20TransferIn, and _doERC1155TransferIn functions do not have any access control mechanisms in place, potentially allowing unauthorized access to sensitive contract operations.

Impact

An attacker could potentially exploit these missing access controls to manipulate contract state, leading to incorrect calculations or vulnerabilities in other functions.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L825

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L846

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L862

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L884

Tool used

Manual Review

Recommendation

Implement access controls for these internal functions, such as using the onlyOwner or onlyAuthorized modifiers, to restrict access to only authorized parties. This will help prevent unauthorized manipulation of the contract's state and protect its users.

Bauer

medium

getPrice() doesn't check If Arbitrum sequencer is down in Chainlink feeds

Summary

Not checking if the sequencer is down may result in bd actors obtaining inconsistent and unfair prices.

Vulnerability Detail

When utilizing Chainlink in L2 chains like Arbitrum, it's important to ensure that the prices provided are not falsely perceived as fresh, even when the sequencer is down. This vulnerability could potentially be exploited by malicious actors to gain an unfair advantage.
Example:https://docs.chain.link/data-feeds/l2-sequencer-feeds#example-code
There is no check in the ChainlinkAdapterOracle.sol

function getPrice(address token_) external view override returns (uint256) {
        // remap token if possible
        address token = remappedTokens[token_];
        if (token == address(0)) token = token_;

        uint256 maxDelayTime = timeGaps[token];
        if (maxDelayTime == 0) revert Errors.NO_MAX_DELAY(token_);

        // Get token-USD price
        uint256 decimals = registry.decimals(token, USD);
        (, int256 answer, , uint256 updatedAt, ) = registry.latestRoundData(
            token,
            USD
        );
        if (updatedAt < block.timestamp - maxDelayTime)
            revert Errors.PRICE_OUTDATED(token_);
        if (answer <= 0) revert Errors.PRICE_NEGATIVE(token_);

        return
            (answer.toUint256() * Constants.PRICE_PRECISION) / 10 ** decimals;
    }

Impact

Could potentially be exploited by malicious actors to gain an unfair advantage.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/oracle/ChainlinkAdapterOracle.sol#L77-L97

Tool used

Manual Review

Recommendation

Check if sequencer is down

Duplicate of #142

n1punp

high

Transaction will revert when using USDT tokens (or other non-compliant ERC20 tokens)

Summary

Transaction will revert when using USDT tokens

Vulnerability Detail

USDT token has a non-standard approve function implementation, as it doesn't return a boolean. So, normal IERC20 interface will cause the EVM to expect a boolean as a return value but it won't get any when token is USDT, and so the tx will revert.

Impact

Any contract functionality that utilizes _ensureApprove will cause tx revert when the token is USDT, including lend, withdrawLend , and executions in all spells.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/utils/EnsureApprove.sol#L22-L23

Tool used

Manual Review

Recommendation

• use safeApprove from OpenZeppelin's standard SafeERC20.sol

moneyversed

high

Potential reentrancy vulnerability in withdrawLend function

Summary

The withdrawLend function may be vulnerable to a reentrancy attack, potentially allowing an attacker to exploit the function and steal funds.

Vulnerability Detail

The withdrawLend function transfers tokens to the message sender after reducing the user's vault share but before emitting the WithdrawLend event. If the token being withdrawn is malicious and has a callback to the withdrawLend function within its transfer function, it could lead to a reentrancy attack.

Impact

An attacker could potentially exploit this vulnerability to repeatedly withdraw tokens without updating the user's vault share, leading to a loss of funds for the contract and its users.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L672

Tool used

Manual Review

Recommendation

Implement a reentrancy guard to prevent reentrancy attacks. Alternatively, emit the WithdrawLend event before transferring tokens to the message sender.

cducrest-brainbot

medium

AuraSpell openPositionFarm does not join pool

Summary

The function to open a position for the AuraSpell does not join the pool due to wrong conditional check.

Vulnerability Detail

The function deposits collateral into the bank, borrow tokens, and attempts to join the pool:

    function openPositionFarm(
        OpenPosParam calldata param
    )
        external
        existingStrategy(param.strategyId)
        existingCollateral(param.strategyId, param.collToken)
    {
        ...
        // 1. Deposit isolated collaterals on Blueberry Money Market
        _doLend(param.collToken, param.collAmount);

        // 2. Borrow specific amounts
        uint256 borrowBalance = _doBorrow(
            param.borrowToken,
            param.borrowAmount
        );

        // 3. Add liquidity on Balancer, get BPT
        {
            IBalancerVault vault = wAuraPools.getVault(lpToken);
            _ensureApprove(param.borrowToken, address(vault), borrowBalance);

            (address[] memory tokens, uint256[] memory balances, ) = wAuraPools
                .getPoolTokens(lpToken);
            uint[] memory maxAmountsIn = new uint[](2);
            maxAmountsIn[0] = IERC20(tokens[0]).balanceOf(address(this));
            maxAmountsIn[1] = IERC20(tokens[1]).balanceOf(address(this));

            uint totalLPSupply = IBalancerPool(lpToken).totalSupply();
            // compute in reverse order of how Balancer's `joinPool` computes tokenAmountIn
            uint poolAmountFromA = (maxAmountsIn[0] * totalLPSupply) /
                balances[0];
            uint poolAmountFromB = (maxAmountsIn[1] * totalLPSupply) /
                balances[1];
            uint poolAmountOut = poolAmountFromA > poolAmountFromB
                ? poolAmountFromB
                : poolAmountFromA;

            bytes32 poolId = bytes32(param.farmingPoolId);
            if (poolAmountOut > 0) {
                vault.joinPool(
                    poolId,
                    address(this),
                    address(this),
                    IBalancerVault.JoinPoolRequest(
                        tokens,
                        maxAmountsIn,
                        "",
                        false
                    )
                );
            }
        }
        ...
    }

The function only borrowed one type of tokens from the bank so the contract only owns one type of token. As a result one of the maxAmountsIn value is 0. Either poolAmountFromA or poolAmountFromB is 0 as a result of computation. poolAmountOut is the minimal value of poolAmountFromA and poolAmountFromB, it is 0. The following check if (poolAmountOut > 0) will always fail and the pool will never be joined.

Impact

The rest of the function proceeds correctly without reverting. Users will think they joined the pool and are earning reward while they are not earning anything. This is a loss of funds to the user.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/spell/AuraSpell.sol#L63-L147

Tool used

Manual Review

Recommendation

It is hard to tell the intent of the developer from this check. Maybe the issue is simply that poolAmountOut should be the sum or the max value out of poolAmountFromA and poolAmountFromB instead of the min.

moneyversed

high

Reentrancy Attack vulnerability in SoftVault.sol

Summary

SoftVault.sol contract is vulnerable to reentrancy attacks on the withdraw() function, as it is calling a third-party contract's function before it has completed processing the user's request.

Vulnerability Detail

The function withdraw() in SoftVault.sol calls the external contract function doCutVaultWithdrawFee() from the IProtocolConfig interface. This external contract function's implementation can be delayed, and during this delay, the withdraw() function remains active, allowing for a reentrancy attack to occur.

Impact

An attacker can exploit this vulnerability to repeatedly execute the withdraw() function until the vault runs out of funds, which will cause the vault to fail. The attacker could also potentially manipulate the balance of the vault and/or the token balance of the contract, resulting in a loss of funds.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/vault/SoftVault.sol#L107

Tool used

Manual Review

Recommendation

It is recommended to avoid third-party function calls within a vulnerable function. For a safe approach, the external function should be called last to avoid the possibility of reentrancy attacks. A possible solution is to transfer the funds to the user first and then call the external function. Another solution is to use the Checks-Effects-Interactions pattern by making all external contract function calls after modifying the state. In this case, a possible solution is to change the function sequence and first transfer the funds to the user and then call the doCutVaultWithdrawFee() function to prevent any potential attack.

Another option is to use reentrancy guards that can be found in OpenZeppelin's ReentrancyGuardUpgradeable library. These guards prevent reentrancy attacks by using a modifier that blocks any recursive calls to the contract function while it is still executing.

moneyversed

medium

Potential flashloan attack in openPositionFarm

Summary

The AuraSpell and CurveSpell contracts allow the openPositionFarm function to execute the process of depositing funds, borrowing, and adding liquidity all in a single atomic transaction. This makes it possible for an attacker to perform a flash loan attack by borrowing a large amount of funds and manipulating the price of assets before the transaction is confirmed.

Vulnerability Detail

An attacker can use a flash loan to borrow a large amount of tokens from a flash loan provider, then manipulate the price of assets on the market before the transaction is confirmed. The attacker can then use the manipulated price to make a profit on their initial flash loan. This can be done by performing a re-entrancy attack, where the attacker is able to call back into the AuraSpell or CurveSpell contract multiple times within the same transaction.

Impact

A successful flash loan attack could result in a significant loss of funds from the Blueberry protocol. The attacker could borrow a large amount of funds, manipulate the price of assets, and profit from the manipulated price, leaving the Blueberry protocol with a large debt that it may not be able to repay.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/AuraSpell.sol#L63

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L67

Tool used

Manual Review

Recommendation

To mitigate flash loan attacks, the contracts should ensure that the amount of collateral deposited by the user is sufficient to cover the value of the loan. Additionally, the contracts should be designed to prevent re-entrancy attacks by limiting the number of external calls made within a single transaction. One possible solution is to separate the lending, borrowing, and liquidity provision steps into separate transactions, rather than allowing them to be executed in a single atomic transaction.

n1punp

high

Missing slippage control when closing position in AuraSpell

Summary

Missing slippage control when closing position in AuraSpell

Vulnerability Detail

There's no slippage control for minimum out tokens obtained.

Impact

Farmers who close positions in AuraSpell can potentially get sandwich-attacked and get $0 value out. (the attacker can manipulate the pool so the swap gets very bad rate). Right now all swaps get 0 min token amount out for slippage control and there are no other checks.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/AuraSpell.sol#L149-L224

Tool used

Manual Review

Recommendation

• Add slippage control to all tokens in the closePositionFarm function

Duplicate of #121

ravikiran.web3

medium

setTokenRemappings() of ChainlinkAdapterOracle contract should check for address(0) for remappedTokens_

Summary

SetTokenRemapping is updating the remapping of tokens for the chainlink oracle. The function is checking for mapped token to be a non-zero address, but should also check for target mapping values also to be non zero address.

Vulnerability Detail

Potentially mapping a valid token to zero address as a pair. The isue is marked medium as the ability to do this update is available only to the owner of this contract.

Impact

Since this pair like BTC -> ZERO is not of interest to any one, it is not high for normal use case point of view. Might bring some vulnerability and hence should be fixed. The code for most part checks for address(0) very deligently, so, this might be a miss.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/oracle/ChainlinkAdapterOracle.sol#L55-L70

Tool used

Manual Review

Recommendation

Check for remappedTokens_ element to be not equal to zero address as below.

function setTokenRemappings(
address[] calldata tokens_,
address[] calldata remappedTokens_
) external onlyOwner {
if (remappedTokens_.length != tokens_.length)
revert Errors.INPUT_ARRAY_MISMATCH();
for (uint256 idx = 0; idx < tokens_.length; idx++) {
if (tokens_[idx] == address(0)) revert Errors.ZERO_ADDRESS();
if (remappedTokens_[idx] == address(0)) revert Errors.ZERO_ADDRESS();

        remappedTokens[tokens_[idx]] = remappedTokens_[idx];
        emit SetTokenRemapping(tokens_[idx], remappedTokens_[idx]);
    }
}