Beanstalk: The Finale
Beanstalk
DeFiHardhatFoundry
250,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Misleading Return Value in `LibTokenSilo:removeDepositFromAccount` Function

0xbeastboy

Summary

The LibTokenSilo:removeDepositFromAccount is intended to return the Bean Derived Value (BDV) of the deposit being removed. However, the function's implementation causes a discrepancy in the return value when dealing with partial removals. Specifically, the function signature indicates that it returns crateBDV, but in the case of a partial removal, it returns removedBDV. This inconsistency can lead to confusion and potential errors in how the return value is used in the broader contract or by external callers.

Vulnerability Details

See the following code:

function removeDepositFromAccount(
address account,
address token,
int96 stem,
uint256 amount
) internal returns (uint256 crateBDV) {
AppStorage storage s = LibAppStorage.diamondStorage();
uint256 depositId = LibBytes.packAddressAndStem(token, stem);
​
uint256 crateAmount = s.accts[account].deposits[depositId].amount;
crateBDV = s.accts[account].deposits[depositId].bdv;
require(amount <= crateAmount, "Silo: Crate balance too low.");
​
// Partial remove
if (amount < crateAmount) {
// round up removal of BDV. (x - 1)/y + 1
// https://stackoverflow.com/questions/17944
uint256 removedBDV = amount.sub(1).mul(crateBDV).div(crateAmount).add(1);
uint256 updatedBDV = crateBDV.sub(removedBDV);
uint256 updatedAmount = crateAmount.sub(amount);
​
// SafeCast unnecessary b/c updatedAmount <= crateAmount and updatedBDV <= crateBDV,
// which are both <= type(uint128).max
s.accts[account].deposits[depositId].amount = uint128(updatedAmount);
s.accts[account].deposits[depositId].bdv = uint128(updatedBDV);
​
s.accts[account].mowStatuses[token].bdv = s.accts[account].mowStatuses[token].bdv.sub(
uint128(removedBDV)
);
​
return removedBDV;
}
// Full remove
if (crateAmount > 0) {
delete s.accts[account].deposits[depositId];
removeDepositIDfromAccountList(account, token, depositId);
}
​
// Will not overflow b/c crateBDV <= type(uint128).max
s.accts[account].mowStatuses[token].bdv = s.accts[account].mowStatuses[token].bdv.sub(
uint128(crateBDV)
);
}

Impact

The misleading return value can cause significant issues. Developers interacting with this function might incorrectly assume the function always returns the crateBDV, leading to incorrect logic in the calling functions. Any code relying on the return value for subsequent calculations or decisions might behave unexpectedly if it does not account for the actual return value being removedBDV in partial removals. Incorrect handling of return values could lead to vulnerabilities, especially in a financial context where accurate value tracking is crucial.

Tools Used

Manual Review

Recommendations

To resolve this issue, the function should be modified to always return uint256, whether the removal is partial or full. This ensures consistency and clarity in the function's behavior.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational/Gas

Invalid as per docs https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity

Support

FAQs

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