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

`Sun.stepSun()` incorrectly calculates Beans minted to Field in case of `activeField` update

T1MOH

Summary

Beanstalk 3 introduces multiple Fields - previously there was just 1 Field. Also it introduces new way to distribute newly minted Beans via ShipmentPlanner.

Issue arises on the edge of these 2 new mechanics. In summary problem is:

  1. ShipmentRoutes are stored in storage. ShipmentRoute for Field contains fieldId as data. I.e. fieldId to add new Beans is strictly stored.

  2. Sun.stepSun() uses difference between harvestable indexes before distribution and after distribution to calculate amount of Beans minted to Field. Note that it uses active fieldId to calculate it.

And here's a problem: active fieldId can differ from fieldId configured in ShipmentRoute. I assume development team is not aware of discrepancy which will arise from updating activeFieldId.

This issue doesn't require admin mistake because admin is not aware of the discrepancy in codebase which requires to update ShipmentRoutes AND activeFieldId in the same time to not cause issues.

Vulnerability Details

Here you can see that it "ships" newly minted Beans and then uses difference in active field to calculate Soil:
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/df2dd129a878d16d4adc75049179ac0029d9a96b/protocol/contracts/beanstalk/sun/SeasonFacet/Sun.sol#L41-L57

function stepSun(int256 deltaB, uint256 caseId) internal {
// Above peg
if (deltaB > 0) {
uint256 priorHarvestable = s.sys.fields[s.sys.activeField].harvestable;
​
C.bean().mint(address(this), uint256(deltaB));
@> LibShipping.ship(uint256(deltaB));
​
@> setSoilAbovePeg(s.sys.fields[s.sys.activeField].harvestable - priorHarvestable, caseId);
s.sys.season.abovePeg = true;
}
// Below peg
else {
setSoil(uint256(-deltaB));
s.sys.season.abovePeg = false;
}
}

And here you see that ShipmentRoute stores the fieldId to submit Beans:

function ship(uint256 beansToShip) public {
...
ShipmentRoute[] memory shipmentRoutes = s.sys.shipmentRoutes;
...
​
// Ship it.
for (uint256 i; i < shipmentAmounts.length; i++) {
if (shipmentAmounts[i] == 0) continue;
LibReceiving.receiveShipment(
shipmentRoutes[i].recipient,
shipmentAmounts[i],
@> shipmentRoutes[i].data
);
}
​
...
}
function fieldReceive(uint256 shipmentAmount, bytes memory data) private {
AppStorage storage s = LibAppStorage.diamondStorage();
​
@> uint256 fieldId = abi.decode(data, (uint256));
require(fieldId < s.sys.fieldCount, "Field does not exist");
s.sys.fields[fieldId].harvestable += shipmentAmount;
​
// Confirm successful receipt.
emit Receipt(ShipmentRecipient.FIELD, shipmentAmount, data);
}

There is obviously a discrepancy between ShipmentRoute and Sun: it must use same fieldId in both contracts. But Shipment logic uses stored fieldId and Sun uses active fieldId.

Impact

In case fieldIds differ between ShipmentRoute and activeFieldId i.e. in case of update of either ShipmentRoute or activeFieldId, it will set 0 Soil in next seasons.

0 Soil means users can't buy Pods by burning Bean to maintain Bean peg. In other words Field module completely doesn't work, i.e. core peg mechanism of Bean doesn't work after such update.

Tools Used

Manual Review

Recommendations

I propose to not store fieldId in ShipmentRoute, but use activeFieldId instead.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

T1MOH Submitter
about 1 year ago
giovannidisiena Auditor
about 1 year ago
inallhonesty 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.