Beanstalk Part 2
Beanstalk
DeFiHardhat
35,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Precision loss inside the calculation of the deposited bean amounts inside the `addUnderlying()` function leads to mint less beans

matin

Summary

Low deposited bean amounts are calculated and minted due to the priority of division over multiplication leading to a precision loss.

Vulnerability Details

Solidity rounds down the result of an integer division, and because of that, it is always recommended to multiply before dividing to avoid that precision loss. In the case of a prior division over multiplication, the final result may face serious precision loss as the first answer would face truncated precision and then multiplied to another integer.
The problem arises in the addUnderlying() function inside adding a fertilizer that is used to unripe beans and LPs.

This function first calculates the percentToFill variable that would be used next in the calculation of the deposited bean and LP amounts.
If we look deeply at the function addUnderlying() we can see the bean calculation procedure is presented as:

function addUnderlying(uint256 tokenAmountIn, uint256 usdAmount, uint256 minAmountOut) internal {
AppStorage storage s = LibAppStorage.diamondStorage();
// Calculate how many new Deposited Beans will be minted
uint256 percentToFill = usdAmount.mul(C.precision()).div(
remainingRecapitalization()
);
​
uint256 newDepositedBeans;
if (C.unripeBean().totalSupply() > s.u[C.UNRIPE_BEAN].balanceOfUnderlying) {
newDepositedBeans = (C.unripeBean().totalSupply()).sub(
s.u[C.UNRIPE_BEAN].balanceOfUnderlying
);
newDepositedBeans = newDepositedBeans.mul(percentToFill).div(
C.precision()
);
}
​
// Calculate how many Beans to add as LP
uint256 newDepositedLPBeans = usdAmount.mul(C.exploitAddLPRatio()).div(
DECIMALS
);
...
}

we can see there is a hidden division before a multiplication that makes rounding down the whole expression.
The newDepositedBeans is calculated in the way that it multiplies the previous amount with the percentToFill that actually has a division inside its heart.

This is bad as the precision loss can be significant, which leads to the minting less newDepositedBeans than the actual.

At the Proof of Concept part, we can check this behavior precisely.
You can run this code to see the difference between the results:

function test_precissionLoss() public {
​
AppStorage storage s = LibAppStorage.diamondStorage();
uint totalSupply = C.unripeBean().totalSupply();
uint balanceOfUnderlying = s.u[C.UNRIPE_BEAN].balanceOfUnderlying;
​
uint256 percentToFill = usdAmount * (C.precision()) / (
remainingRecapitalization()
);
​
newDepositedBeans = (totalSupply) - (
balanceOfUnderlying
);
​
newDepositedBeans1 = newDepositedBeans * (percentToFill) / (
C.precision()
);
​
newDepositedBeans2 = newDepositedBeans * usdAmount * (C.precision()) / (
C.precision() * remainingRecapitalization()
);
​
console.log("Current Implementation ", newDepositedBeans1);
console.log("Precise Implementation ", newDepositedBeans2);
}

Thus, we can see that the actual implementation produces fewer deposited beans than the precise method.

Impact

Minting less deposited bean amounts than the actual amount due to precision loss

Tools Used

Manual Review

Recommendations

Consider modifying the deposited beans calculation to prevent such precision loss and prioritize the multiplication over division:

newDepositedBeans = newDepositedBeans * usdAmount * (C.precision()) / (
C.precision() * remainingRecapitalization()
);
Updates

Lead Judging Commences

giovannidisiena Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Other
Assigned finding tags:

Precision loss

Support

FAQs

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