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

Insufficient token withdrawal handling in `_withdrawTokens` function that can lead to blocked withdrawals and DOS

rhaydden

Title

Insufficient token withdrawal handling in _withdrawTokens function that can lead to blocked withdrawals and DOS

Summary

The issue is that _withdrawTokens function does not properly handle cases where the total amount of tokens withdrawn from the specified deposits is less than the requested maxTokens amount. This can potentially lead to unexpected reverts and prevent legitimate token withdrawals.

Vulnerability Details

The issue lies in the _withdrawTokens function,

https://github.com/Cyfrin/2024-05-Beanstalk-3/blob/662d26f12ee219ee92dc485c06e01a4cb5ee8dfb/protocol/contracts/beanstalk/silo/ConvertFacet.sol#L125-L213

function _withdrawTokens(
address token,
int96[] memory stems,
uint256[] memory amounts,
uint256 maxTokens,
address account
) internal returns (uint256, uint256) {
require(
stems.length == amounts.length,
"Convert: stems, amounts are diff lengths."
);
LibSilo.AssetsRemoved memory a;
uint256 depositBDV;
uint256 i = 0;
​
// a bracket is included here to avoid the "stack too deep" error.
{
uint256[] memory bdvsRemoved = new uint256[](stems.length);
uint256[] memory depositIds = new uint256[](stems.length);
​
// get germinating stem and stemTip for the token
LibGerminate.GermStem memory germStem = LibGerminate.getGerminatingStem(token);
​
while ((i < stems.length) && (a.active.tokens < maxTokens)) {
// skip any stems that are germinating, due to the ability to
// circumvent the germination process.
if (germStem.germinatingStem <= stems[i]) {
i++;
continue;
}
​
if (a.active.tokens.add(amounts[i]) >= maxTokens) amounts[i] = maxTokens.sub(a.active.tokens);
depositBDV = LibTokenSilo.removeDepositFromAccount(
account,
token,
stems[i],
amounts[i]
);
bdvsRemoved[i] = depositBDV;
a.active.stalk = a.active.stalk.add(
LibSilo.stalkReward(
stems[i],
germStem.stemTip,
depositBDV.toUint128()
)
);
a.active.tokens = a.active.tokens.add(amounts[i]);
a.active.bdv = a.active.bdv.add(depositBDV);
depositIds[i] = uint256(LibBytes.packAddressAndStem(
token,
stems[i]
));
i++;
}
for (i; i < stems.length; ++i) amounts[i] = 0;
emit RemoveDeposits(
account,
token,
stems,
amounts,
a.active.tokens,
bdvsRemoved
);
​
emit LibSilo.TransferBatch(
account,
account,
address(0),
depositIds,
amounts
);
}
​
require(
a.active.tokens == maxTokens,
"Convert: Not enough tokens removed."
);
LibTokenSilo.decrementTotalDeposited(token, a.active.tokens, a.active.bdv);
​
// all deposits converted are not germinating.
LibSilo.burnActiveStalk(
account,
a.active.stalk.add(a.active.bdv.mul(s.ss[token].stalkIssuedPerBdv))
);
return (a.active.stalk, a.active.bdv);
}

specifically in this part:

https://github.com/Cyfrin/2024-05-Beanstalk-3/blob/662d26f12ee219ee92dc485c06e01a4cb5ee8dfb/protocol/contracts/beanstalk/silo/ConvertFacet.sol#L201-L203

require(
a.active.tokens == maxTokens,
"Convert: Not enough tokens removed."
);

This require statement checks if the total amount of tokens withdrawn i.e (a.active.tokens) is exactly equal to the requested maxTokens amount.
Albeit, it's possible that there can be scenarios where the specified deposits do not have enough non-germinating tokens to reach the maxTokens amount. In such cases, the function will revert with the error message "Convert: Not enough tokens removed", even though it has legitimately withdrawn all available tokens from the given deposits.

The issue is valid because there's no consideration of the possibility of having insufficient tokens in the deposits to fulfill the exact maxTokens amount. It assumes that the specified deposits will always have enough tokens to meet the requested withdrawal amount which may not always be so.

Impact

As a result of this,
Legitimate token withdrawals may be blocked if the specified deposits do not have enough non-germinating tokens to reach the maxTokens amount, even though all available tokens have been withdrawn and users will be unable to convert their deposits if the _withdrawTokens function reverts due to insufficient tokens, even when they have provided all available deposits.
Additionally, in can possibly disallow users from executing the convert function in certain scenarios.

Recommended Mitigation Steps

Modify the require statement in the _withdrawTokens function to handle cases where less than maxTokens can be legitimately withdrawn due to deposit constraints.
We tried to come up with modification that will work and we think this will do:

https://github.com/your-repo/ConvertFacet.sol#L201-L204

require(
- a.active.tokens == maxTokens,
+ a.active.tokens == maxTokens ||
+ (i == stems.length && a.active.tokens < maxTokens),
"Convert: Not enough tokens removed."
);

This modification introduces an additional condition to the require statement:

  • If a.active.tokens equals maxTokens, it means the exact requested amount has been withdrawn, and the function can proceed.

  • If i has reached stems.length (all deposits have been processed) and a.active.tokens is still less than maxTokens, it means there are not enough non-germinating deposits to fulfill the requested amount. In this case, the function should also proceed since it has withdrawn all available tokens.

Updates

Lead Judging Commences

giovannidisiena Lead Judge about 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.