Summary
It is possible to create pods with 0 beans but can not be removed from the s.accts[account].fields[s.sys.activeField].plotIndexes
Relevant GitHub Links:
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/main/protocol/contracts/beanstalk/field/FieldFacet.sol#L108-L127
Vulnerability Details
It is possible to sow 0 beans because there is no check for it. When 0 beans are sown the s.accts[account].fields[s.sys.activeField].plots[index] is set to 0 (which changes nothing) and the pod index is pushed in the s.accts[account].fields[s.sys.activeField].plotIndexes.
These empty indexes can only be created by a user himself and there is no way to remove them from the array. That means that once someone has sowed 0 beans by accident or on purpose will not be able to remove it ever again. That is because when harvesting, filling a pod listing or transfering it checks for the amount of pods, since it is 0 reverts.
function _harvestPlot(
address account,
uint256 fieldId,
uint256 index
) private returns (uint256 harvestablePods) {
uint256 pods = s.accts[account].fields[fieldId].plots[index];
require(pods > 0, "Field: no plot");
...
}
function _fillListing(
PodListing calldata podListing,
address filler,
uint256 beanPayAmount
) internal {
...
uint256 plotSize = s.accts[podListing.lister].fields[podListing.fieldId].plots[
podListing.index
];
require(podListing.podAmount > 0, "Marketplace: Invalid Amount.");
require(
plotSize >= (podListing.start + podListing.podAmount),
"Marketplace: Invalid Plot."
);
...
}
function transferPlot(
address sender,
address recipient,
uint256 fieldId,
uint256 index,
uint256 start,
uint256 end
) external payable fundsSafu noNetFlow noSupplyChange nonReentrant {
...
uint256 transferAmount = validatePlotAndReturnPods(fieldId, sender, index, start, end);
...
}
function validatePlotAndReturnPods(
uint256 fieldId,
address sender,
uint256 id,
uint256 start,
uint256 end
) internal view returns (uint256 amount) {
amount = s.accts[sender].fields[fieldId].plots[id];
require(amount > 0, "Field: Plot not owned by user.");
require(end > start && amount >= end, "Field: Pod range invalid.");
amount = end - start;
}
Having indexes in the array with 0 beans will make it gas expensive to loop through all the indexes array. This loop needs to be executed when harvest, filling a listing or transfering pods.
function removePlotIndexFromAccount(
address account,
uint256 fieldId,
uint256 plotIndex
) internal {
AppStorage storage s = LibAppStorage.diamondStorage();
uint256 i = findPlotIndexForAccount(account, fieldId, plotIndex);
Field storage field = s.accts[account].fields[fieldId];
field.plotIndexes[i] = field.plotIndexes[field.plotIndexes.length - 1];
field.plotIndexes.pop();
}
function findPlotIndexForAccount(
address account,
uint256 fieldId,
uint256 plotIndex
) internal view returns (uint256 i) {
AppStorage storage s = LibAppStorage.diamondStorage();
Field storage field = s.accts[account].fields[fieldId];
uint256[] memory plotIndexes = field.plotIndexes;
uint256 length = plotIndexes.length;
while (plotIndexes[i] != plotIndex) {
i++;
if (i >= length) {
revert("Id not found");
}
}
return i;
}
This problem will lead to the user having to pay extra gas for harvesting or transfering his pods or buyers of a pod listing because a user that buys a pod listing pays for the transfer of the pods of the seller to his position. This last scenario could be used by the seller to spam a lot of 0 bean pods in order to make the buyer spend a lot of gas looping through his array of pods.
Impact
Medium, users can mistakenly create this positions that will make them burn gas forever or do it to make pod listing buyers spend more gas when they want to buy one of his positions.
Tools Used
Manual review
Recommendations
Constrain the sow function to be a minimum amount of beans to sow, it can perfectly be 1 because it would be harvestable:
function sowWithMin(
uint256 beans,
uint256 minTemperature,
uint256 minSoil,
LibTransfer.From mode
) public payable fundsSafu noSupplyIncrease oneOutFlow(C.BEAN) returns (uint256 pods) {
+ require(beans >= C.MIN_SOWING_BEANS);
// `soil` is the remaining Soil
(uint256 soil, uint256 _morningTemperature, bool abovePeg) = _totalSoilAndTemperature();
require(soil >= minSoil && beans >= minSoil, "Field: Soil Slippage");
require(_morningTemperature >= minTemperature, "Field: Temperature Slippage");
// If beans >= soil, Sow all of the remaining Soil
if (beans < soil) {
soil = beans;
}
// 1 Bean is Sown in 1 Soil, i.e. soil = beans
pods = _sow(soil, _morningTemperature, abovePeg, mode);
}
