Beanstalk Part 3
Beanstalk
DeFiHardhat
21,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

`newBDV` is wrongly set in some instances of converting tokens

bauchibred

Summary

The ConvertFacet features a convert function designed to facilitate the conversion of one deposit into another, incorporating a decreaseBDV flag to manage adjustments to the Bean Denominated Value (BDV). This flag, when activated, logically necessitates setting toBDV to the lesser value between newBdv and fromBdv, aligning with the intention to reduce BDVs. Documentation further clarifies that, absent the decreaseBDV flag, toBDV should reflect the higher of these two values. Consequently, enabling decreaseBDV implies adjusting toBDV to the lower value, a logical approach intended to conserve resources. However, the current implementation incorrectly sets toBDV to newBdv regardless of the decreaseBDV flag's status, undermining the intended functionality and potentially affecting the accuracy of deposit germination statuses through incorrect stem data processing.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-05-Beanstalk-3/blob/662d26f12ee219ee92dc485c06e01a4cb5ee8dfb/protocol/contracts/beanstalk/silo/ConvertFacet.sol#L69-L116

function convert(
bytes calldata convertData,
int96[] memory stems,
uint256[] memory amounts
)
external
payable
nonReentrant
returns (int96 toStem, uint256 fromAmount, uint256 toAmount, uint256 fromBdv, uint256 toBdv)
{
uint256 grownStalk;
LibConvert.convertParams memory cp = LibConvert.convert(convertData);
​
if (cp.decreaseBDV) {require(stems.length == 1 && amounts.length == 1, "Convert: DecreaseBDV only supports updating one deposit.");}
​
require(cp.fromAmount > 0, "Convert: From amount is 0.");
​
// Replace account with msg.sender if no account is specified.
if(cp.account == address(0)) cp.account = msg.sender;
​
LibSilo._mow(cp.account, cp.fromToken);
​
// If the fromToken and toToken are different, mow the toToken as well.
if (cp.fromToken != cp.toToken) LibSilo._mow(cp.account, cp.toToken);
​
// Withdraw the tokens from the deposit.
(grownStalk, fromBdv) = _withdrawTokens(
cp.fromToken,
stems,
amounts,
cp.fromAmount,
cp.account
);
​
// Calculate the bdv of the new deposit.
uint256 newBdv = LibTokenSilo.beanDenominatedValue(cp.toToken, cp.toAmount);
​
// If `decreaseBDV` flag is not enabled, set toBDV to the max of the two bdvs.
//@audit
toBdv = (newBdv > fromBdv || cp.decreaseBDV) ? newBdv : fromBdv;
​
//@audit
toStem = _depositTokensForConvert(cp.toToken, cp.toAmount, toBdv, grownStalk, cp.account);
​
// Retrieve the rest of return parameters from the convert struct.
toAmount = cp.toAmount;
fromAmount = cp.fromAmount;
​
emit Convert(cp.account, cp.fromToken, cp.toToken, cp.fromAmount, cp.toAmount);
}

This function allows a user to convert a deposit to another deposit, given that the conversion is supported by the ConvertFacet.

Now, protocol implements a decreaseBDV flag that could be set/unset, whenever this is set it's understandable that the toBDV needs to be set to the lower value of newBdv & fromBdv, this can be concluded from common logic since we are trying to decrease the BDVs and also the fact that the documentation clearly states that if the decreaseBDV flag is not enabled, then the toBDV needs to be set to the max of the two bdvs. So this means that if the decreaseBDV is indeed enabled, the opposite should be done and the toBDV should be set to the lower value of the two bdvs.

This however is not correctly implemented due to the way the check is coded. The code reads:
toBdv = (newBdv > fromBdv || cp.decreaseBDV) ? newBdv : fromBdv;
This would always chose the right value for toBdv whenever the decreaseBDV flag is not enabled since the cp.decreaseBDV check would always evaluate to false and the final value that gets set for toBDV depends on the newBdv > fromBdv , however, whenever the decreaseBDV flag is indeed enabled, then the cp.decreaseBDV check is always going to be true which means that regardless of the newBdv > fromBdv check, toBDV is always going to be set to newBdv which would flaw the execution, since the decrease flag is set and least of the two between newBdv & fromBdv needs to be set to toBDV.

Keep in mind that the toStem value calculated is directly affected by the toBDV passed, since the _depositTokensForConvert() function delegates the calculation to LibTokenSilo#calculateStalkFromStemAndBdv() which has a direct relation between the toStem & the toBDV value.

Now since the toStem value processed by the flow would be flawed, this would then lead to the wrong finalization of the germinating status of a deposit's stem, since the evaluation in LibGerminate._getGerminationState() would be checking with the wrong stem data.

NB: The work flow to LibGerminate._getGerminationState() is shown below:

ConvertFacet.convert() -> ConvertFacet._depositTokensForConvert() -> LibTokenSilo.calculateStemForTokenFromGrownStalk() -> LibGerminate.getGerminationState() -> LibGerminate._getGerminationState()

Impact

The ConvertFacet#convert() does not always work as expected when the decreaseBDV flag is enabled, this report shows how there is a disparity in how the conversions treat converts in relation to when the decreaseBDV flag is set or not.

Which leads to multiple issues including the wrong finalization of the stem status since the LibGerminate._getGerminationState() integrates with wrong data.

Tools Used

Manual review

Recommendations

Reimplement the way toBDV is set in ConvertFacet#convert() whenever the decreaseBDV flag is enabled, consider breaking the checks into two and then have the toBDV when the decreaseBDV flag is not enabled set as the max between newBdv & fromBdv and then when the decreaseBDV flag is enabled, have the toBDV set to the least between newBdv & fromBdv.
Below is a pseudo fix in how the new implementation of the function should look

function convert(
bytes calldata convertData,
int96[] memory stems,
uint256[] memory amounts
)
external
payable
nonReentrant
returns (int96 toStem, uint256 fromAmount, uint256 toAmount, uint256 fromBdv, uint256 toBdv)
{
uint256 grownStalk;
LibConvert.convertParams memory cp = LibConvert.convert(convertData);
​
if (cp.decreaseBDV) {require(stems.length == 1 && amounts.length == 1, "Convert: DecreaseBDV only supports updating one deposit.");}
​
require(cp.fromAmount > 0, "Convert: From amount is 0.");
​
// Replace account with msg.sender if no account is specified.
if(cp.account == address(0)) cp.account = msg.sender;
​
LibSilo._mow(cp.account, cp.fromToken);
​
// If the fromToken and toToken are different, mow the toToken as well.
if (cp.fromToken!= cp.toToken) LibSilo._mow(cp.account, cp.toToken);
​
// Withdraw the tokens from the deposit.
(grownStalk, fromBdv) = _withdrawTokens(
cp.fromToken,
stems,
amounts,
cp.fromAmount,
cp.account
);
​
// Calculate the bdv of the new deposit.
uint256 newBdv = LibTokenSilo.beanDenominatedValue(cp.toToken, cp.toAmount);
​
// Corrected logic for setting toBDV based on the decreaseBDV flag.
if (cp.decreaseBDV) {
toBdv = fromBdv < newBdv? fromBdv : newBdv;
} else {
toBdv = newBdv > fromBdv? newBdv : fromBdv;
}
​
toStem = _depositTokensForConvert(cp.toToken, cp.toAmount, toBdv, grownStalk, cp.account);
​
// Retrieve the rest of return parameters from the convert struct.
toAmount = cp.toAmount;
fromAmount = cp.fromAmount;
​
emit Convert(cp.account, cp.fromToken, cp.toToken, cp.fromAmount, cp.toAmount);
}
Updates

Lead Judging Commences

giovannidisiena Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.