Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/97
0x52
The current methodology for valuing Stable BPT is incorrect and can lead to significant over valuation of the stable BPT.
uint256 minPrice = base.getPrice(tokens[0]);
for(uint256 i = 1; i != length; ++i) {
uint256 price = base.getPrice(tokens[i]);
minPrice = (price < minPrice) ? price : minPrice;
}
return minPrice.mulWadDown(pool.getRate());
The above block is used to calculate the price. Finding the min price of all assets in the pool then multiplying by the current rate of the pool. This is nearly identical to how stable curve LP is priced. Balancer pools are a bit different and this methodology is incorrect for them. Lets look at a current mainnet pool to see the problem. Take the wstETH/aETHc pool. Currently getRate() = 1.006. The lowest price is aETHc at 2,073.23. This values the LP at 2,085.66. The issue is that the LPs actual value is 1,870.67 (nearly 12% overvalued) which can be checked here.
Overvaluing the LP as such can cause protocol insolvency as the borrower can overborrow against the LP, leaving the protocol with bad debt.
Protocol insolvency due to overborrowing
Manual Review
Stable BPT oracles need to use a new pricing methodology
sherlock-admin2
2 comment(s) were left on this issue during the judging contest.
0xyPhilic commented:
invalid because there is no sufficient data/explanations to support the explained issue
Kral01 commented:
only an issue if the protocol uses this LP pair
IAm0x52
Escalate
This is not a dupe of #100. Though it focuses on a similar area of the code, the underlying issue is completely different. StableBPT is value highly incorrectly for some pools and it will cause significant damage to the protocol.
sherlock-admin2
Escalate
This is not a dupe of #100. Though it focuses on a similar area of the code, the underlying issue is completely different. StableBPT is value highly incorrectly for some pools and it will cause significant damage to the protocol.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Shogoki
ing the LP as such can cause protocol insolvency as the borrower can overborrow against the LP, leaving the protocol with bad debt.
Yes, not a duplicate of #100
@Gornutz can you take a look at this?
Gornutz
Confirm this is not a duplicate of #100
hrishibhat
Result:
High
Unique
Considering this a valid high issue as the wrong price is calculated and returned
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/98
0x52, Vagner
CurveTricryptoOracle assumes that WETH is always the last token in the pool (tokens[2]). This is incorrect for a majority of tricrypto pools and will lead to LP being highly overvalued.
CurveTricryptoOracle.sol#L53-L63
if (tokens.length == 3) {
/// tokens[2] is WETH
uint256 ethPrice = base.getPrice(tokens[2]);
return
(lpPrice(
virtualPrice,
base.getPrice(tokens[1]),
ethPrice,
base.getPrice(tokens[0])
) * 1e18) / ethPrice;
}
When calculating LP prices, CurveTricryptoOracle#getPrice always assumes that WETH is the second token in the pool. This isn't the case which will cause the LP to be massively overvalued.
There are 6 tricrypto pools currently deployed on mainnet. Half of these pools have an asset other than WETH as token[2]:
0x4ebdf703948ddcea3b11f675b4d1fba9d2414a14 - CRV
0x5426178799ee0a0181a89b4f57efddfab49941ec - INV
0x2889302a794da87fbf1d6db415c1492194663d13 - wstETH
LP will be massively overvalued leading to overborrowing and protocol insolvency
CurveTricryptoOracle.sol#L48-L65
Manual Review
There is no need to assume that WETH is the last token. Simply pull the price for each asset and input it into lpPrice.
IAm0x52
Escalate
This is not a dupe of #105. This will cause a large number of tricrypto pools to be overvalued which presents a serious risk to the protocol.
sherlock-admin2
Escalate
This is not a dupe of #105. This will cause a large number of tricrypto pools to be overvalued which presents a serious risk to the protocol.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Shogoki
Agree.
This is not a duplicate of #105
This can become its own main report and #20 is a duplicate of it.
There were some issues with (de)duplication. I would resolve like this.
#98 is the duplicate with #20
#105 is duplicate with #42
hrishibhat
Result:
High
Has duplicates
This is a valid high issue based on the description
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/100
0x52, Kow
CurveTricryptoOracle#getPrice incorrectly divides the LP price by the price of ETH which causes it to return the price of LP in terms of ETH instead of USD
CurveTricryptoOracle.sol#L57-L62
(lpPrice(
virtualPrice,
base.getPrice(tokens[1]),
ethPrice,
base.getPrice(tokens[0])
) * 1e18) / ethPrice;
After the LP price has been calculated in USD it is mistakenly divided by the price of ETH causing the contract to return the LP price in terms of ETH rather than USD. This leads to LP that is massively undervalued causing positions which are actually heavily over collateralized to be liquidated.
Healthy positions are liquidated due to incorrect LP pricing
CurveTricryptoOracle.sol#L48-L65
Manual Review
Don't divide the price by the price of ETH
sherlock-admin2
2 comment(s) were left on this issue during the judging contest.
0xyPhilic commented:
invalid because lpPrice is considered in ETH so dividing by ETH/USD price returns the final result in USD
Kral01 commented:
there is a precision value
Gornutz
Judges accurately state why the division by ETH/USD is required to return the proper USD value.
Shogoki
Closing in regards to other judges and sponsors comments.
IAm0x52
This is valid. The code being used here was borrowed from the Sentiment CurveTriCryptoOracle, which is specifically meant to return the price in terms of ETH. This oracle is meant to return the valuation in USD which means the division by the price of ETH needs to be dropped.
Shogoki
This is valid. The code being used here was borrowed from the Sentiment CurveTriCryptoOracle, which is specifically meant to return the price in terms of ETH. This oracle is meant to return the valuation in USD which means the division by the price of ETH needs to be dropped.
Maybe was a bit quick in closing. Will reopen it and we will take a deeper look at it.
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/109
0x52
When calculating the amount of pending AURA owed to a user _getAuraPendingReward uses the current values for supply. This leads to incorrect calculation across cliffs which leads to loss of rewards for users.
if (cliff < totalCliffs) {
/// e.g. (new) reduction = (500 - 100) * 2.5 + 700 = 1700;
/// e.g. (new) reduction = (500 - 250) * 2.5 + 700 = 1325;
/// e.g. (new) reduction = (500 - 400) * 2.5 + 700 = 950;
uint256 reduction = ((totalCliffs - cliff) * 5) / 2 + 700;
/// e.g. (new) amount = 1e19 * 1700 / 500 = 34e18;
/// e.g. (new) amount = 1e19 * 1325 / 500 = 26.5e18;
/// e.g. (new) amount = 1e19 * 950 / 500 = 19e17;
mintAmount = (mintRequestAmount * reduction) / totalCliffs;
/// e.g. amtTillMax = 5e25 - 1e25 = 4e25
uint256 amtTillMax = emissionMaxSupply - emissionsMinted;
if (mintAmount > amtTillMax) {
mintAmount = amtTillMax;
}
}
The above code is used to calculate the amount of AURA owed to the user. This calculation is perfectly accurate if the AURA hasn't been minted yet. The problem is that each time a user withdraws, AURA is claimed for ALL vault participants. This means that the rewards will be realized for a majority of users before they themselves withdraw. Since the emissions decrease with each cliff, there will be loss of funds at the end of each cliff.
Example:
Assume for simplicity there are only 2 cliffs. User A deposits LP to WAuraPools. After some time User B deposits as well. Before the end of the first cliff User A withdraw. This claims all tokens owed to both users A and B which is now sitting in the contract. Assume both users are owed 10 tokens. Now User B waits for the second cliff to end before withdrawing. When calculating his rewards it will give him no rewards since all cliffs have ended. The issue is that the 10 tokens they are owed is already sitting in the contract waiting to be claimed.
All users will lose rewards at the end of each cliff due to miscalculation
Manual Review
I would recommend a hybrid approach. When rewards are claimed upon withdrawal, the reward per token should be cached to prevent loss of tokens that have already been received by the contract. Only unminted AURA should be handled this way.
IAm0x52
Escalate
This was wrongly excluded and causes a significant problem. Whenever a position is burned it calls the following line:
IAuraRewarder(auraRewarder).withdraw(amount, true);
Which withdraws the LP and claims rewards because of the true input. Digging into the rewarder contract:
https://etherscan.io/address/0x1204f5060be8b716f5a62b4df4ce32acd01a69f5#code#F18#L229
function withdraw(uint256 amount, bool claim)
public
updateReward(msg.sender)
returns(bool)
{
require(amount > 0, 'RewardPool : Cannot withdraw 0');
//also withdraw from linked rewards
for(uint i=0; i < extraRewards.length; i++){
IRewards(extraRewards[i]).withdraw(msg.sender, amount);
}
_totalSupply = _totalSupply.sub(amount);
_balances[msg.sender] = _balances[msg.sender].sub(amount);
stakingToken.safeTransfer(msg.sender, amount);
emit Withdrawn(msg.sender, amount);
if(claim){
getReward(msg.sender,true);
}
emit Transfer(msg.sender, address(0), amount);
return true;
}
Here we see that if claim == true then we call getReward
https://etherscan.io/address/0x1204f5060be8b716f5a62b4df4ce32acd01a69f5#code#F18#L296
function getReward(address _account, bool _claimExtras) public updateReward(_account) returns(bool){
uint256 reward = earned(_account);
if (reward > 0) {
rewards[_account] = 0;
rewardToken.safeTransfer(_account, reward);
IDeposit(operator).rewardClaimed(pid, _account, reward);
emit RewardPaid(_account, reward);
}
//also get rewards from linked rewards
if(_claimExtras){
for(uint i=0; i < extraRewards.length; i++){
IRewards(extraRewards[i]).getReward(_account);
}
}
return true;
}
Here we see that the ENTIRE reward balance is claimed for the vault. This presents the problem as outlined in my issue above. Let's assume that the reward for the current round is 1:1 (i.e. that each claimed BAL gives 1 AURA). Assume we have 2 users, each with half the pool. Now 100 tokens are claimed in this round, which means each user is owed 50 AURA. After some amount of rounds, the reward is now 1:0.5 (i.e. that each claimed BAL gives 1 AURA). Now when user A withdraws instead of being paid 100 AURA for that round they will instead only be paid 50 AURA and they will lose the other 50 since it won't be claimable. This is because it always uses the most recent exchange rate instead of the rate at which it was claimed.
sherlock-admin2
Escalate
This was wrongly excluded and causes a significant problem. Whenever a position is burned it calls the following line:
IAuraRewarder(auraRewarder).withdraw(amount, true);Which withdraws the LP and claims rewards because of the
trueinput. Digging into the rewarder contract:https://etherscan.io/address/0x1204f5060be8b716f5a62b4df4ce32acd01a69f5#code#F18#L229
function withdraw(uint256 amount, bool claim) public updateReward(msg.sender) returns(bool) { require(amount > 0, 'RewardPool : Cannot withdraw 0'); //also withdraw from linked rewards for(uint i=0; i < extraRewards.length; i++){ IRewards(extraRewards[i]).withdraw(msg.sender, amount); } _totalSupply = _totalSupply.sub(amount); _balances[msg.sender] = _balances[msg.sender].sub(amount); stakingToken.safeTransfer(msg.sender, amount); emit Withdrawn(msg.sender, amount); if(claim){ getReward(msg.sender,true); } emit Transfer(msg.sender, address(0), amount); return true; }Here we see that if claim == true then we call
getRewardhttps://etherscan.io/address/0x1204f5060be8b716f5a62b4df4ce32acd01a69f5#code#F18#L296
function getReward(address _account, bool _claimExtras) public updateReward(_account) returns(bool){ uint256 reward = earned(_account); if (reward > 0) { rewards[_account] = 0; rewardToken.safeTransfer(_account, reward); IDeposit(operator).rewardClaimed(pid, _account, reward); emit RewardPaid(_account, reward); } //also get rewards from linked rewards if(_claimExtras){ for(uint i=0; i < extraRewards.length; i++){ IRewards(extraRewards[i]).getReward(_account); } } return true; }Here we see that the ENTIRE reward balance is claimed for the vault. This presents the problem as outlined in my issue above. Let's assume that the reward for the current round is 1:1 (i.e. that each claimed BAL gives 1 AURA). Assume we have 2 users, each with half the pool. Now 100 tokens are claimed in this round, which means each user is owed 50 AURA. After some amount of rounds, the reward is now 1:0.5 (i.e. that each claimed BAL gives 1 AURA). Now when user A withdraws instead of being paid 100 AURA for that round they will instead only be paid 50 AURA and they will lose the other 50 since it won't be claimable. This is because it always uses the most recent exchange rate instead of the rate at which it was claimed.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Shogoki
@Gornutz What do you think about it?
I think it is a valid Medium.
Gornutz
Seems directionally correct and valid from the parts of the POC provided.
IAm0x52
The current AURA APR for pools are high double digit to triple digit returns. Additionally AURA is distributed over ~8 years and there are 500 cliffs over that period. This makes the average cliff ~6 days. Unless every single user withdraws and redeposits each 6 day period (which is prohibitively expensive due to gas costs) this loss cannot be avoided. Given the almost certainty of the loss and the substantial amount these users stand to lose due to the high APRs and leveraged nature of the system, I believe this should be high risk rather than medium.
hrishibhat
Result:
High
Unique
After further review considering this to be a high based on the comments above
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/117
mert_eren
BlueBerryBank.sol calculate position's risk value with getPositionRisk() . This function used for is position can be liquadatable or not.
function getPositionRisk(uint256 positionId) public returns (uint256 risk) { uint256 pv = getPositionValue(positionId); uint256 ov = getDebtValue(positionId); uint256 cv = getIsolatedCollateralValue(positionId); if ( (cv == 0 && pv == 0 && ov == 0) || pv >= ov /// Closed position or Overcollateralized position ) { risk = 0; } else if (cv == 0) { /// Sth bad happened to isolated underlying token risk = Constants.DENOMINATOR; } else { risk = ((ov - pv) * Constants.DENOMINATOR) / cv; } }
getIsolatedColleteralValue() calculate lended value in position.
function getIsolatedCollateralValue( uint256 positionId ) public override returns (uint256 icollValue) { Position memory pos = positions[positionId]; /// NOTE: exchangeRateStored has 18 decimals. uint256 underlyingAmount; if (_isSoftVault(pos.underlyingToken)) { underlyingAmount = (ICErc20(banks[pos.debtToken].bToken).exchangeRateStored() * pos.underlyingVaultShare) / Constants.PRICE_PRECISION; } else { underlyingAmount = pos.underlyingVaultShare; } icollValue = oracle.getTokenValue( pos.underlyingToken, underlyingAmount ); }
For calculate total value it should calculate first underlyingAmount. Due to calculate this, it should first multiply pos.underlyingVaultShare with exchangeRateStored() of this token.Because when stored underlyingVaultShare in lend function, it not stored directly lended amount but the amount minted in softVault after this tokens lended to it.
function lend( address token, uint256 amount ) external override inExec poke(token) onlyWhitelistedToken(token) { ... if (_isSoftVault(token)) { _ensureApprove(token, bank.softVault, amount); pos.underlyingVaultShare += ISoftVault(bank.softVault).deposit( amount ); } else { _ensureApprove(token, bank.hardVault, amount); pos.underlyingVaultShare += IHardVault(bank.hardVault).deposit( token, amount ); }
SoftVault's deposit function call mint function of blueBerry bToken (which work same as compound cToken).And return minted amount of bToken. bToken mintAmount is amount/exchangeRateStored . So when calculate underlyingAmount , pos.UnderlyinVaultShare must be multiplied with btoken of underlyingToken's exchangeRateStored. However as seen above of the getIsolatedColleteralValue() code snippet it multiply with debtToken's bToken not underlyingToken's. Due to all bTokens' has different exchangeRate, underlyingAmount will be calculate wrong.
misCalculation of getIsolatedColleteralValue() so cv in getPositionRisk.This will lead some protocols can be
Manual Review
use underlying token's bToken exchangeRate in getIsolatedColleteralValue() not debtToken's.
merteren1234
Escalate
I think this issue is not duplicate of #16. Because this issue indicate differeant root cause than #16. This issue's root cause is , wrong bToken's exchangeRate used in getIsolatedColleteral().Not about old exchangeRate used or not. Moreover without fixing this issue , #16 is not valid because pos.debtToken is updated with poke modifier in liquidate function and its exchangeRate used in getIsolatedColleteralValue() .
This issue is about getIsolatedColleteralValue() use debtToken's exchangeRate which is wrong because this functions should use underlyingToken's exchangeRate. So even though #16 is fixed (there will be no impact without fix this issue.) this issue still remain and changing ICErc20(banks[pos.debtToken].bToken).exchangeRateStored() with ICErc20(banks[pos.underlyingToken].bToken).exchangeRateStored() is mandatory to take correct getIsolatedColleteralValue().
sherlock-admin2
Escalate
I think this issue is not duplicate of #16. Because this issue indicate differeant root cause than #16. This issue's root cause is , wrong bToken's exchangeRate used in getIsolatedColleteral().Not about old exchangeRate used or not. Moreover without fixing this issue , #16 is not valid because pos.debtToken is updated with poke modifier in liquidate function and its exchangeRate used in getIsolatedColleteralValue() .
This issue is about getIsolatedColleteralValue() use debtToken's exchangeRate which is wrong because this functions should use underlyingToken's exchangeRate. So even though #16 is fixed (there will be no impact without fix this issue.) this issue still remain and changingICErc20(banks[pos.debtToken].bToken).exchangeRateStored()withICErc20(banks[pos.underlyingToken].bToken).exchangeRateStored()is mandatory to take correct getIsolatedColleteralValue().
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Shogoki
Not a duplicate of #16
hrishibhat
@Shogoki is this a valid issue on its own?
nevillehuang
@Shogoki is this a valid issue on its own?
@Gornutz
hrishibhat
Result:
High
Unique
After further review and discussions considering this a valid high issue as the exchange rate can be affected significantly by the tokens decimal and this is not a duplicate of #16
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/15
Arz
If the Curve pool that the ConvexSpell.sol uses becomes killed, users will be unable to close their position because remove_liquidity_one_coin() will revert. Users will be unable to repay their debt so their assets will be liquidated.
ConvexSpell.sol::closePositionFarm() is used to close an existing liquidity position. After the collateral is taken and the rewards are swapped, _removeLiquidity() is called which removes liquidity from a Curve pool by calling remove_liquidity_one_coin().
The problem arises when the Curve pool is killed(paused) so if self.is_killed in the curve pool contract is true, calling remove_liquidity_one_coin() function will always revert and closePositionFarm() function will be DOS'ed
Note: This issue was submitted in the previous contest however only the CurveSpell got fixed but not the ConvexSpell.
When user's position is about to be liquidated, if the closePositionFarm() function is DOS'ed,users will be unable to repay their debt, resulting in the users losing their funds
260: /// Removes liquidity from the Curve pool for the specified token. 261: ICurvePool(pool).remove_liquidity_one_coin( 262: amountPosRemove, 263: int128(tokenIndex), 264: param.amountOutMin 265: );
As you can see, remove_liquidity_one_coin() is called when the user calls closePositionFarm() in the ConvexSpell
670: def remove_liquidity_one_coin(_token_amount: uint256, i: int128, min_amount: uint256): 671: """ 672: Remove _amount of liquidity all in a form of coin i 673: """ 674: assert not self.is_killed # dev: is killed
The problem is that remove_liquidity_one_coin() checks if self.is_killed is true so if the Curve pool is killed then this will revert and the user wont be able to close his position.
Manual Review
When killed, it is only possible for existing LPs to remove liquidity via remove_liquidity so there should be the same isKilled check in the ConvexSpell like there is in the CurveSpell
sherlock-admin2
2 comment(s) were left on this issue during the judging contest.
0xyPhilic commented:
invalid because the case where a Curve pool is killed is handled by withdrawing liquidity using remove_liquidity function
darkart commented:
If its already been accepted its up to the DEV to decide if he wants to fix it or no
getPrice in WeightedBPTOracle.sol uses totalSupply for price calculations which can lead to wrong resultsSource: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/18
Vagner
getPrice is used to calculate the price in USD of a given balancer LP, and it respects the recommendations of Balancer docs by calculating the invariant and using it to protect from manipulations, but it uses totalSupply for every LP calculated which can lead to wrong results and assumptions.
In the Balancer docs it is specified that There are three potential functions to query when determining the BPT supply depending on pool type. https://docs.balancer.fi/concepts/advanced/valuing-bpt/valuing-bpt.html#getting-bpt-supply
getActualSupply : which is used by the most recent Weighted and Stable Pools and it accounts for pre-minted BPT as well as due protocol fees:getVirtualSupply : which is used by Linear Pools and "legacy" Stable Phantom Pools and it accounts just for pre-minted BPTtotalSupply : which makes sense to be called only by older legacy pools since those doesn't have pre-minted BPTgetPrice uses every time totalSupplyImpact is a medium one, since it can lead to wrong price assumptions
Manual Review
Consider implementing the more recent getActualSupply even if older pools doesn't have this functions , because it can lead to wrong assumptions and calculations.
sherlock-admin2
1 comment(s) were left on this issue during the judging contest.
YakuzaKiawe commented:
invalid as this could be a factor to exploit the pool in future
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/60
nobody2018
Before calling ICurvePool#add_liquidity to deposit tokens, each token needs to be approved to the pool. If the approved amount is smaller than the parameters passed into ICurvePool#add_liquidity, it will cause erc20.transferFrom revert inside the function. In this way, openPositionFarm will also revert.
In previous contest, [#47](https://github.com/sherlock-audit/2023-04-blueberry-judging/issues/47) was not fixed correctly. This issue is still exist.
The fix for this issue from this contest is as following:
File: blueberry-core\contracts\spell\CurveSpell.sol 098: // 2. Borrow specific amounts 099: uint256 borrowBalance = _doBorrow( 100: param.borrowToken, 101: param.borrowAmount 102: ); 103: 104: // 3. Add liquidity on curve 105: address borrowToken = param.borrowToken; 106: _ensureApprove(param.borrowToken, pool, borrowBalance); 107: if (tokens.length == 2) { 108: uint256[2] memory suppliedAmts; 109: for (uint256 i = 0; i < 2; i++) { //this 'if' check is the fix from the previous contest 110:-> if (tokens[i] == borrowToken) { 111: suppliedAmts[i] = IERC20Upgradeable(tokens[i]).balanceOf( 112: address(this) 113: ); 114: break; 115: } 116: } 117: ICurvePool(pool).add_liquidity(suppliedAmts, minLPMint); 118: } else if (tokens.length == 3) {
The key to this issue is that borrowBalance may be smaller than IERC20Upgradeable(borrowToken).balanceOf(address(this)). For simplicity, assume that CurveSpell supports an lptoken which contains two tokens : A and B.
Bob transferred 1wei of A and B to the CurveSpell contract. Alice opens a position by calling BlueBerryBank#execute, and the flow is as follows:
CurveSpell#openPositionFarm._doLend to deposit isolated collaterals._doBorrow to borrow 100e18 A token. borrowBalance = 100e18.A.approve(pool, 100e18).suppliedAmts[0] = A.balance(address(this)) = 100e18+1wei, suppliedAmts[1] = 0.ICurvePool(pool).add_liquidity(suppliedAmts, minLPMint), then revert because the approved amount is not enough.Therefore, no one can successfully open a position.
Of course, bob can also transfer 1wei of borrowToken to contract by front-running openPositionFarm for a specific user or all users.
In ConvexSpell, the issue lies in [the same code](https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/spell/ConvexSpell.sol#L110-L144) as in previous games.
ConvexSpell/CurveSpell.openPositionFarm will revert due to this issue.
Manual Review
The following fix is for CurveSpell, but please don't forget ConvexSpell.
Two ways for fix it:
--- a/blueberry-core/contracts/spell/CurveSpell.sol +++ b/blueberry-core/contracts/spell/CurveSpell.sol @@ -108,9 +108,7 @@ contract CurveSpell is BasicSpell { uint256[2] memory suppliedAmts; for (uint256 i = 0; i < 2; i++) { if (tokens[i] == borrowToken) { - suppliedAmts[i] = IERC20Upgradeable(tokens[i]).balanceOf( - address(this) - ); + suppliedAmts[i] = borrowBalance; break; } } @@ -119,9 +117,7 @@ contract CurveSpell is BasicSpell { uint256[3] memory suppliedAmts; for (uint256 i = 0; i < 3; i++) { if (tokens[i] == borrowToken) { - suppliedAmts[i] = IERC20Upgradeable(tokens[i]).balanceOf( - address(this) - ); + suppliedAmts[i] = borrowBalance; break; } } @@ -130,9 +126,7 @@ contract CurveSpell is BasicSpell { uint256[4] memory suppliedAmts; for (uint256 i = 0; i < 4; i++) { if (tokens[i] == borrowToken) { - suppliedAmts[i] = IERC20Upgradeable(tokens[i]).balanceOf( - address(this) - ); + suppliedAmts[i] = borrowBalance; break; } }
--- a/blueberry-core/contracts/spell/CurveSpell.sol +++ b/blueberry-core/contracts/spell/CurveSpell.sol @@ -103,7 +103,8 @@ contract CurveSpell is BasicSpell { // 3. Add liquidity on curve address borrowToken = param.borrowToken; - _ensureApprove(param.borrowToken, pool, borrowBalance); + require(borrowBalance <= IERC20Upgradeable(borrowToken).balanceOf(address(this)), "impossible"); + _ensureApprove(param.borrowToken, pool, IERC20Upgradeable(borrowToken).balanceOf(address(this))); if (tokens.length == 2) { uint256[2] memory suppliedAmts; for (uint256 i = 0; i < 2; i++) {
sherlock-admin2
2 comment(s) were left on this issue during the judging contest.
0xyPhilic commented:
invalid because the borrowAmount can't change mid function execution
darkart commented:
If already approven its up to Developrs to chose if they will implement it
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/96
0x52
Mainnet oracles are incompatible with wstETH causing many popular yields strategies to be broken. Chainlink and Band do not have wstETH oracles and using Uniswap LP pairs would be very dangerous given their low liquidity.
ChainlinkAdapterOracle.sol#L111-L125
uint256 decimals = registry.decimals(token, USD);
(
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;
ChainlinkAdapterOracle only supports single asset price data. This makes it completely incompatible with wstETH because chainlink doesn't have a wstETH oracle on mainnet. Additionally Band protocol doesn't offer a wstETH oracle either. This only leaves Uniswap oracles which are highly dangerous given their low liquidity.
Mainnet oracles are incompatible with wstETH causing many popular yields strategies to be broken
ChainlinkAdapterOracle.sol#L102-L126
Manual Review
Create a special bypass specifically for wstETH utilizing the stETH oracle and it's current exchange rate.
sherlock-admin2
2 comment(s) were left on this issue during the judging contest.
0xyPhilic commented:
invalid because this issue can be considered informational or at best low - tokens used are whitelisted
Kral01 commented:
low severity
IAm0x52
Escalate
This was wrongly excluded. Protocol is meant to be compatible with these pools but can't work with them. I believe this is a valid medium because the protocol is nonfunctional in this area.
sherlock-admin2
Escalate
This was wrongly excluded. Protocol is meant to be compatible with these pools but can't work with them. I believe this is a valid medium because the protocol is nonfunctional in this area.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Shogoki
Escalate
This was wrongly excluded. Protocol is meant to be compatible with these pools but can't work with them. I believe this is a valid medium because the protocol is nonfunctional in this area.
Not sure were it says that the protcol is meant to be compatible with WSTETH pools on mainnet.
If that is the case it can be a valid issue, i guess.
However if it is not, i think the Whitelisting of tokens would count for invalidating it.
IAm0x52
Protocol is meant to be compatible with Aura/Convex. wstETH is a component of many highly attractive pools. Not being able to support wstETH as an underlying asset will break support for these.
hrishibhat
@Gornutz
Gornutz
Confirm this is valid.
hrishibhat
Result:
Medium
Unique
Considering this issue a valid medium
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/102
0x52, Breeje, Oxhunter526, Vagner, bitsurfer, nobody2018
When exiting the balancer pool, vault#exitPool is called with an empty array for minAmountsOut causing the position to be exited with no slippage protection. Typically it is not an issue to exit off axis but since it is exiting to a single token this can cause massive loss.
(
uint256[] memory minAmountsOut,
address[] memory tokens,
uint256 borrowTokenIndex
) = _getExitPoolParams(param.borrowToken, lpToken);
wAuraPools.getVault(lpToken).exitPool(
IBalancerPool(lpToken).getPoolId(),
address(this),
address(this),
IBalancerVault.ExitPoolRequest(
tokens,
minAmountsOut,
abi.encode(0, amountPosRemove, borrowTokenIndex),
false
)
When exiting a the balancer vault, closePositionFarm makes a subcall to _getExitPoolParams which is used to set minAmountsOut.
(address[] memory tokens, , ) = wAuraPools.getPoolTokens(lpToken);
uint256 length = tokens.length;
uint256[] memory minAmountsOut = new uint256[](length);
Inside _getExitPoolParams we see that minAmountsOut are always an empty array. This means that the user has no slippage protection and can be sandwich attacked, suffering massive losses.
Exits can be sandwich attacked causing massive loss to the user
Manual Review
Allow user to specify min amount received from exit
securitygrid
Escalate:
Historically, lacking slippage protection is H.
sherlock-admin2
Escalate:
Historically, lacking slippage protection is H.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Shogoki
Escalate: Historically, lacking slippage protection is H.
Can be a high issue
IAm0x52
Why would lack of slippage be considered high in this scenario? Even with zero slippage protection sandwich attack profitability is always contingent on a few external factors such as: liquidity of underlying pool, the fee of the underlying pool, the amount being swapped and the current gas prices. The higher the liquidity and fee of the pool the higher the cost to push the pool price then pull it back. The lower the amount being swapped, the less the attacker can steal. If this cost exceeds the gain from the attack then the attack isn't profitable and won't happen.
VagnerAndrei26
I think no slippage is considered high most of the time cause sandwich attacks are easily doable in the space, especially for those experienced in doing it, and even if there are factors to consider even one successful can occur loss of funds for the protocol in a pretty easy manner. So considering that and also past contests I think it should be considered also a high.
Nabeel-javaid
as far as I know 98% of the times slippage issues are considered as Medium severity
hrishibhat
Result:
Medium
Has duplicates
This is a valid medium issue. Agree with the Lead Watson's comments here:
https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/102#issuecomment-1699649100
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/103
0x52
AuraSpell#AuraSpell#closePositionFarm will take reward fees on underlying tokens when borrow token is also a reward. This is because the BLP is burned before taking the reward cut.
wAuraPools.getVault(lpToken).exitPool(
IBalancerPool(lpToken).getPoolId(),
address(this),
address(this),
IBalancerVault.ExitPoolRequest(
tokens,
minAmountsOut,
abi.encode(0, amountPosRemove, borrowTokenIndex),
false
)
);
}
}
/// 4. Swap each reward token for the debt token
uint256 rewardTokensLength = rewardTokens.length;
for (uint256 i; i != rewardTokensLength; ) {
address sellToken = rewardTokens[i];
if (sellToken == STASH_AURA) sellToken = AURA;
_doCutRewardsFee(sellToken);
We can see above that closePositionFarm redeems the BLP before it takes the reward cut. This can cause serious issues. If there is any overlap between the reward tokens and the borrow token then _doCutRewardsFee will take a cut of the underlying liquidity. This causes loss to the user as too many fees are taken from them.
User will lose funds due to incorrect fees
Manual Review
Use the same order as ConvexSpell and sell rewards BEFORE burning BLP
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/104
0x52
PSwapLib.swap can be manipulated to leave hanging allowances via the expectedRewards input. These can then be abused to swap reward tokens out of order allowing them to bypass fees.
_doCutRewardsFee(sellToken);
if (
expectedRewards[i] != 0 &&
!PSwapLib.swap(
augustusSwapper,
tokenTransferProxy,
sellToken,
expectedRewards[i],
swapDatas[i]
)
) revert Errors.SWAP_FAILED(sellToken);
AuraSpell#closePositionFarm allows the user to specify any expectedRewards they wish. This allows the user to approve any amount, even if the amount is much larger than they would otherwise use. The can abuse these hanging approvals to swap tokens out of order and avoid paying reward fees.
Example:
Assume there are two rewards, token A and token B. Over time a user's position accumulates 100 rewards for each token. Normally the user would have to pay fees on those rewards. However they can bypass it by first creating hanging approvals. The user would start by redeeming a very small amount of LP and setting expectedRewards to uint256.max. They wouldn't sell the small amount leaving a very large approval left for both tokens. Now the user withdraws the rest of their position. This time they specify the swap data to swap token B first. The user still has to pay fees on token A but now they have traded token B before any fees can be taken on it.
User can bypass reward fees
Manual Review
After the swap reset allowances to 0
sherlock-admin2
2 comment(s) were left on this issue during the judging contest.
0xyPhilic commented:
invalid because fees are cut before the supplied check in _doCutFees function
Kral01 commented:
Not how approval works
IAm0x52
Escalate
This was wrongly excluded. The judging comments above are incorrect. The code allows the user to make any swap in whatever order they want. This is what allows the user to bypass the fees. The user is meant to swap in the order that the tokens are listed but by swapping in a different order they can bypass the cut. Assume we have two tokens to claim. Normally the flow would be:
RewardCut A > Swap A > RewardCut B > Swap B
Using the methodology I've shown above the user can instead:
RewardCut A > Swap B > RewardCutB > Swap A
Here we see that B can be swapped before the reward cut allowing the fee to be bypassed.
sherlock-admin2
Escalate
This was wrongly excluded. The judging comments above are incorrect. The code allows the user to make any swap in whatever order they want. This is what allows the user to bypass the fees. The user is meant to swap in the order that the tokens are listed but by swapping in a different order they can bypass the cut. Assume we have two tokens to claim. Normally the flow would be:
RewardCut A > Swap A > RewardCut B > Swap B
Using the methodology I've shown above the user can instead:
RewardCut A > Swap B > RewardCutB > Swap A
Here we see that B can be swapped before the reward cut allowing the fee to be bypassed.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Shogoki
Escalate
This was wrongly excluded. The judging comments above are incorrect. The code allows the user to make any swap in whatever order they want. This is what allows the user to bypass the fees. The user is meant to swap in the order that the tokens are listed but by swapping in a different order they can bypass the cut. Assume we have two tokens to claim. Normally the flow would be:
RewardCut A > Swap A > RewardCut B > Swap B
Using the methodology I've shown above the user can instead:
RewardCut A > Swap B > RewardCutB > Swap A
Here we see that B can be swapped before the reward cut allowing the fee to be bypassed.
Agree with escalation. As the user can specify any expectedRewards and any callData in any order, this is a valid attack vector.
hrishibhat
@Gornutz
Gornutz
The paraswap lib is already resetting the token approval to zero prior to allocating the allowance for each swap - https://github.com/sherlock-audit/2023-07-blueberry/blob/7c7e1c4a8f3012d1afd2e598b656010bb9127836/blueberry-core/contracts/libraries/Paraswap/PSwapLib.sol#L15
Shogoki
The paraswap lib is already resetting the token approval to zero prior to allocating the allowance for each swap - https://github.com/sherlock-audit/2023-07-blueberry/blob/7c7e1c4a8f3012d1afd2e598b656010bb9127836/blueberry-core/contracts/libraries/Paraswap/PSwapLib.sol#L15
I do not think this helps preventing this issue. As the approval is resetted before the swap, there still can be a hanging approval.
Issue is that the approval is set/reset for the specified sellToken, but the user can swap any other token by specifying different calldatas. Therefore, i think the stated attack path is still possible
Gornutz
Gotcha, then yes seems valid
hrishibhat
Result:
Medium
Unique
This is a valid medium issue
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/105
0x52, 0xjoseph
When a Curve pool utilizes native ETH it uses the address 0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee. This is problematic because it will try to call balanceOf on this address which will always revert.
if (tokens.length == 2) {
uint256[2] memory suppliedAmts;
for (uint256 i; i != 2; ++i) {
suppliedAmts[i] = IERC20Upgradeable(tokens[i]).balanceOf(
address(this)
);
}
ICurvePool(pool).add_liquidity(suppliedAmts, minLPMint);
ConvexSpell#openPositionFarm attempts to call balanceOf on each component of the LP. Since native ETH uses the 0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee this call will always revert. This breaks compatibility with EVERY curve pool that uses native ETH which make most of the highest volume pools on the platfrom.
ConvexSpell is completely incompatible with a majority of Curve pools
Manual Review
I would recommend conversion between native ETH and wETH to prevent this issue.
sherlock-admin2
1 comment(s) were left on this issue during the judging contest.
0xyPhilic commented:
invalid because the design of the protocol does not show desire for ETH compatibility so the issue can be classified as informational
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/106
0x52
Issue 47 from update contest 1 was only fixed for CurveSpell but wasn't fixed for ConvexSpell.
See issue 47
See issue 47
Manual Review
See issue 47
sherlock-admin2
1 comment(s) were left on this issue during the judging contest.
darkart commented:
Its up to Devs if they wanna fix it
IAm0x52
Escalate
This is a completely different issue than #42. This was the same issue from the previous contest. They dev simply missed the occurrence in convex spell. This is a valid med and needs to be fixed.
sherlock-admin2
Escalate
This is a completely different issue than #42. This was the same issue from the previous contest. They dev simply missed the occurrence in convex spell. This is a valid med and needs to be fixed.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Shogoki
Escalate
This is a completely different issue than #42. This was the same issue from the previous contest. They dev simply missed the occurrence in convex spell. This is a valid med and needs to be fixed.
Agree, not a duplicate #42. This is an issue from the previous contest that was fixed, but is here found at a different place. Can be a valid finding on its own.
IAm0x52
Missed that this should be a dupe of #60
hrishibhat
Result:
Medium
Unique
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
hrishibhat
Considering this issue a valid issue on its own. #60 talks of the fix that was implemented in the previous contest, while this issue points to the original underlying issue that still existed in one of the contracts.
This is a unique situation, hence both the issues are treated separately
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/108
0x52
Some Aura pools have two sources of AURA. First from the booster but also as a secondary reward. This secondary reward is stash AURA that doesn't behave like regular AURA. Although properly accounted for in AuraSpell, it is not properly accounted for in WAuraPools, resulting in all deposits being unrecoverable.
uint256 rewardTokensLength = rewardTokens.length;
for (uint256 i; i != rewardTokensLength; ) {
IERC20Upgradeable(rewardTokens[i]).safeTransfer(
msg.sender,
rewards[i]
);
When burning the wrapped LP token, it attempts to transfer each token to msg.sender. The problem is that stash AURA cannot be transferred like an regular ERC20 token and any transfers will revert. Since this will be called on every attempted withdraw, all deposits will be permanently unrecoverable.
All deposits will be permanently unrecoverable
Manual Review
Check if reward is stash AURA and send regular AURA instead similar to what is done in AuraSpell.
sherlock-admin2
1 comment(s) were left on this issue during the judging contest.
Kral01 commented:
Needs PoC
IAm0x52
Escalate
This was incorrectly excluded.
for (uint256 i; i != rewardTokensLength; ) {
address sellToken = rewardTokens[i];
if (sellToken == STASH_AURA) sellToken = AURA;
_doCutRewardsFee(sellToken);
if (
expectedRewards[i] != 0 &&
!PSwapLib.swap(
augustusSwapper,
tokenTransferProxy,
sellToken,
expectedRewards[i],
swapDatas[i]
)
) revert Errors.SWAP_FAILED(sellToken);
/// Refund rest (dust) amount to owner
_doRefund(sellToken);
unchecked {
++i;
}
AuraSpell requires the above code to prevent this. WAuraPools uses the exact same reward list and needs the same protections. The result is that funds will be permanently trapped, because the transfer will always fail.
sherlock-admin2
Escalate
This was incorrectly excluded.
for (uint256 i; i != rewardTokensLength; ) { address sellToken = rewardTokens[i]; if (sellToken == STASH_AURA) sellToken = AURA; _doCutRewardsFee(sellToken); if ( expectedRewards[i] != 0 && !PSwapLib.swap( augustusSwapper, tokenTransferProxy, sellToken, expectedRewards[i], swapDatas[i] ) ) revert Errors.SWAP_FAILED(sellToken); /// Refund rest (dust) amount to owner _doRefund(sellToken); unchecked { ++i; }AuraSpell requires the above code to prevent this. WAuraPools uses the exact same reward list and needs the same protections. The result is that funds will be permanently trapped, because the transfer will always fail.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Shogoki
Seems valid
STASH AURA seems to be a valid extraRewardToken but transfer can only be called from the pool, which will actually transfer AURA instead.
So if the WAURAPool tries to call transfer it will revert.
hrishibhat
Result:
Medium
Unique
Considering this issue a valid medium
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/119
Kow
WConvexPool (and consequently ConvexSpell) will be completely broken on Arbitrum due to not accounting for implementation differences in Convex Arbitrum contracts.
WConvexPool.sol is the contract expected to be deployed to both mainnet and Arbitrum (sponsor confirmed). While it does integrate with mainnet Convex contracts, it does not account for implementation differences in Arbitrum Convex contracts (briefly mentioned in their docs with the Arbitrum version of the Booster contract that Blueberry integrates with found here). The primary issue is the change in arguments returned from cvxPools.poolInfo() when calling getPoolInfoFromPoolId. Since the return signature expects 6 values (through destructuring) while the Arbitrum Booster contract's getter only returns 5 (due to a slight change in the PoolInfo struct - mainnet struct here), all attempts to call it will revert. This is called in pendingRewards, mint, and burn, 3 of the main functions in WConvexPool, and will consequently disable any interaction with it (this includes ConvexSpell which also uses this function and also attempts to call mint/burn for position management).
Other issues include the use of cvxPools.deposit(...) and cvxPools.withdraw(...) which do not match any function signatures in the Arbitrum Booster contract (withdraw has been changed to withdrawTo with different arguments and deposit no longer includes the 3rd argument).
Convex integration will be completely broken on Arbitrum due to unaccounted for implementation differences.
https://github.com/sherlock-audit/2023-07-blueberry/blob/7c7e1c4a8f3012d1afd2e598b656010bb9127836/blueberry-core/contracts/wrapper/WConvexPools.sol#L103-L126
https://github.com/sherlock-audit/2023-07-blueberry/blob/7c7e1c4a8f3012d1afd2e598b656010bb9127836/blueberry-core/contracts/wrapper/WConvexPools.sol#L179-L191
https://github.com/sherlock-audit/2023-07-blueberry/blob/7c7e1c4a8f3012d1afd2e598b656010bb9127836/blueberry-core/contracts/wrapper/WConvexPools.sol#L235-L241
https://github.com/sherlock-audit/2023-07-blueberry/blob/7c7e1c4a8f3012d1afd2e598b656010bb9127836/blueberry-core/contracts/wrapper/WConvexPools.sol#L276-L292
Manual Review
Consider creating a separate contract for WConvexPools for Arbitrum that correctly accounts for the Convex Booster implementation changes.
openPositionFarm will revert when the tokens contains lpTokenSource: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/125
bitsurfer, nobody2018, twcctop
AuraSpell openPositionFarm will revert when the tokens contains lpToken due to array length mismatch
In AuraSpell, the openPositionFarm will call joinPool in Balancer's vault. But when analyzing the JoinPoolRequest struct, we see issue on maxAmountsIn and amountsIn which can be in different length, thus this will be reverted since in Balancer's vault, this two array should be in the same length.
File: AuraSpell.sol 088: function openPositionFarm( 089: OpenPosParam calldata param, 090: uint256 minimumBPT 091: ) ... 095: { ... 110: /// 3. Add liquidity to the Balancer pool and receive BPT in return. 111: { ... 128: if (poolAmountOut != 0) { 129: vault.joinPool( 130: wAuraPools.getBPTPoolId(lpToken), 131: address(this), 132: address(this), 133: IBalancerVault.JoinPoolRequest({ 134: assets: tokens, 135: maxAmountsIn: maxAmountsIn, 136: userData: abi.encode(1, amountsIn, _minimumBPT), 137: fromInternalBalance: false 138: }) 139: ); 140: } 141: } ... 178: } ... 296: function _getJoinPoolParamsAndApprove( 297: address vault, 298: address[] memory tokens, 299: uint256[] memory balances, 300: address lpToken 301: ) internal returns (uint256[] memory, uint256[] memory, uint256) { ... 304: uint256 length = tokens.length; 305: uint256[] memory maxAmountsIn = new uint256[](length); 306: uint256[] memory amountsIn = new uint256[](length); 307: bool isLPIncluded; 308: 309: for (i; i != length; ) { 310: if (tokens[i] != lpToken) { 311: amountsIn[j] = IERC20(tokens[i]).balanceOf(address(this)); 312: if (amountsIn[j] > 0) { 313: _ensureApprove(tokens[i], vault, amountsIn[j]); 314: } 315: ++j; 316: } else isLPIncluded = true; 317: 318: maxAmountsIn[i] = IERC20(tokens[i]).balanceOf(address(this)); 319: 320: unchecked { 321: ++i; 322: } 323: } 324: 325: if (isLPIncluded) { 326: assembly { 327: mstore(amountsIn, sub(mload(amountsIn), 1)) 328: } 329: } ... 345: return (maxAmountsIn, amountsIn, poolAmountOut); 346: }
these maxAmountsIn and amountsIn are coming from _getJoinPoolParamsAndApprove. And by seeing the function, we can see that there is possible issue when the tokens[i] == lpToken.
When tokens[i] == lpToken, the flag isLPIncluded will be true. And will enter this block,
325: if (isLPIncluded) { 326: assembly { 327: mstore(amountsIn, sub(mload(amountsIn), 1)) 328: } 329: }
this will decrease the amountsIn length. Thus, amountsIn and maxAmountsIn will be in different length.
In Balancer's JoinPoolRequest struct, the maxAmountsIn, and userData second decoded bytes (amountsIn) should be the same array length, because it will be checked in Balancer.
133: IBalancerVault.JoinPoolRequest({ 134: assets: tokens, 135: maxAmountsIn: maxAmountsIn, 136: userData: abi.encode(1, amountsIn, _minimumBPT), 137: fromInternalBalance: false 138: })
Therefore, in this situation, it will be reverted.
User can't open position on AuraSpell when tokens contains lpToken
Manual Review
Remove the assembly code where it will decrease the amountsIn length when isLPIncluded is true to make sure the array length are same.
Source: https://github.com/sherlock-audit/2023-07-blueberry-judging/issues/135
n1punp, trauki
medium
Open Zeppelin's safeTransfer() is used throughout the blueberry contracts, this ensures that calls to contracts without return values don't fail, however, the EnsureApprove contract uses a normal IERC20 interface and a normal approve() function call. Since USDT doesn't return a boolean as expected by the interface, this would leave the contract unusable.
If a SoftVault is deployed using USDT as the uToken, the contract's main functions won't work as intended since the ensureApprove function call will fail.
USDT approve signature: function approve(address spender, uint value) public;
OpenZeppelin ERC20 approve signature: function approve(address spender, uint256 value) external returns (bool);
This will cause loss of funds because the cost to deploy the contract will essentially have been wasted. The developers explicitly stated that they intend to use USDT as an underlying token in the SoftVault's NatSpec:
Manual Review
Add support for USDT by importing another interface with ERC20 functions that don't return values.
sherlock-admin2
1 comment(s) were left on this issue during the judging contest.
0xyPhilic commented:
invalid because the IERC20 interface would fit also USDT as it has an approve function - no function signature is used anywhere
Gornutz
Item discussed in the previous competition full context is given here - https://github.com/sherlock-audit/2023-04-blueberry-judging/issues/15#issuecomment-1542727941
Shogoki
Item discussed in the previous competition full context is given here - sherlock-audit/2023-04-blueberry-judging#15 (comment)
Actually i do not understand why the issue was closed in the last contest with the comment:
USDT only can call approve function when the allowance is zero or to set the allowance to zero first.
This is not related to the stated issue, that the approval will revert because the generic IERC20 interface is used, which is expecting a bool return data.
nevillehuang
Escalate:
Referring to @Gornutz comment, this was marked as No-Fix and Non-Reward in previous contest, and should be invalid according to sherlock's rules here:
In an update contest, issues from the previous contest with wont fix labels are not considered valid.
In addition, EnsureApprove.sol uses _ensureApprove() directly to approve tokens (for e.g. USDT) for contracts inheriting it, so the issue above of needing an interface is not a problem.
sherlock-admin2
Escalate:
Referring to @Gornutz comment, this was marked as No-Fix and Non-Reward in previous contest, and should be invalid according to sherlock's rules here:
In an update contest, issues from the previous contest with wont fix labels are not considered valid.
In addition,
EnsureApprove.soluses_ensureApprove()directly to approve tokens (for e.g. USDT) for contracts inheriting it, so the issue above of needing an interface is not a problem.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Shogoki
Escalate:
Referring to @Gornutz comment, this was marked as No-Fix and Non-Reward in previous contest, and should be invalid according to sherlock's rules here:
In an update contest, issues from the previous contest with wont fix labels are not considered valid.
Hmm, yep that is kind of an interesting case. I agree, that it is probably fair in terms of rewards to consider this invalid.
However as stated in my comment above, i do not understand the reasoning for closing this in the initial contest.
securitygrid
The function signature only needs the function name and parameters, and does not need a return value.
Shogoki
The function signature only needs the function name and parameters, and does not need a return value.
While this is true for the call, it is still a problem as the transaction will revert because Solidity checks for the expected return size and compares it to the actual returned data size.
nevillehuang
The function signature only needs the function name and parameters, and does not need a return value.
While this is true for the call, it is still a problem as the transaction will revert because Solidity checks for the expected return size and compares it to the actual returned data size.
As @securitygrid said, there will be no reverts, given allowance is approved to zero first. The interface only requires the function selector to match and call the function in the USDT contract check this link here
When a contract is called, the EVM (Ethereum Virtual Machine) reads the first four bytes of the provided data to determine the function selector. The EVM uses this selector to match it with the correct function within the contract. If a match is found, the function is executed. If no match is found, the function call fails.
For example, function signature of approve() here would be bytes4(keccak256("approve(address,uint)"));
Unless there are two approve functions with different number of arguments, this submission seems like only a valid QA/low finding since there is only 1 approve function exposed in USDT contract
securitygrid
@nevillehuang
This problem should be considered from the compiled code:
The compiled code of IERC20(token).approve(spender, 0) is similar to the following:
(bool ret, bytes data) = token.call(abi.encodeWithSignature("approve(address,uint256)", spender, 0); if (ret) { if (data.length != 1) // since usdt.approve has no return value, so data.length = 0 { revert; } return abi.decode(data, (bool)); } else { revert; }
Shogoki
@nevillehuang This problem should be considered from the compiled code: The compiled code of
IERC20(token).approve(spender, 0)is similar to the following:(bool ret, bytes data) = token.call(abi.encodeWithSignature("approve(address,uint256)", spender, 0); if (ret) { if (data.length != 1) // since usdt.approve has no return vault, so data.length = 0 { revert; } return abi.decode(data, (bool)); } else { revert; }
That is correct.
To demonstrate the issue we can use the following litle PoC.
EnsureApproveTest.sol// SPDX-License-Identifier: MIT pragma solidity 0.8.16; import {EnsureApprove} from "./utils/EnsureApprove.sol" ; contract EnsureApproveTest is EnsureApprove { function approveMe(address token, uint256 amount) external { _ensureApprove(token, msg.sender, amount); } }
ensureApprove.tsimport { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; import { ethers, upgrades } from "hardhat"; import chai, { expect } from "chai"; import { EnsureApproveTest, IERC20 } from "../typechain-types"; import { roughlyNear } from "./assertions/roughlyNear"; import { near } from "./assertions/near"; import { Contract } from "ethers"; chai.use(roughlyNear); chai.use(near); const USDT_ADDRESS = "0xdac17f958d2ee523a2206206994597c13d831ec7"; const USDC_ADDRESS = "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48"; describe("Ensure Approve", () => { let admin: SignerWithAddress; let alice: SignerWithAddress; let testContract: EnsureApproveTest; let usdt: Contract; let usdc: Contract; before(async () => { [admin, alice] = await ethers.getSigners(); }); beforeEach(async () => { const EnsureApproveTest = await ethers.getContractFactory("EnsureApproveTest"); testContract = await EnsureApproveTest.deploy(); usdt = await ethers.getContractAt("@openzeppelin/contracts/token/ERC20/IERC20.sol:IERC20", USDT_ADDRESS); usdc = await ethers.getContractAt("@openzeppelin/contracts/token/ERC20/IERC20.sol:IERC20", USDC_ADDRESS); }); describe("IERC20 Interface", () => { it("TEST USDC APPROVAL", async () => { const approvalBefore = await usdc.allowance(testContract.address, alice.address); console.log("Allowance Before", approvalBefore); await testContract.connect(alice).approveMe(usdc.address, 1000); const approvalAfter = await usdc.allowance(testContract.address, alice.address); console.log("Allowance After", approvalAfter); expect(approvalAfter).to.equal(1000); }); it("TEST USDT APPROVAL", async () => { const approvalBefore = await usdt.allowance(testContract.address, alice.address); console.log("Allowance Before", approvalBefore); await testContract.connect(alice).approveMe(usdt.address, 1000); const approvalAfter = await usdt.allowance(testContract.address, alice.address); console.log("Allowance After", approvalAfter); expect(approvalAfter).to.equal(1000); }); }); });
Running this will result in the 2nd test to fail, because of the mismatching return data length
1 failing
1) Ensure Approve
IERC20 Interface
TEST USDT APPROVAL:
Error: Transaction reverted: function returned an unexpected amount of data
hrishibhat
Result:
Medium
Has duplicates
Considering this a value medium issue based on the discussions above, this was a valid issue that was not acknowledged in the previous contests, but now with additional information is considered a valid medium.
I agree the Wont fix rule might need to be more clear of the conditions to which they are applied
sherlock-admin2
Escalations have been resolved successfully!
Escalation status: