DeFiHardhatFoundry
250,000 USDC
View results
Submission Details
Severity: low
Valid

Plot allowances should have defined the index and start

Summary

Plot allowances are not completely constrained and can lead to users move pods that approvers would not like to

Relevant GitHub Links:

https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/main/protocol/contracts/beanstalk/market/MarketplaceFacet/PodTransfer.sol#L102-L109

Vulnerability Details

When someone buys a huge amount of pods, the ones that are closer to the harvestable counter will be more valuable than the farest ones. That is because those pods will become harvestable and exchangable for beans sooner than the others.
However, the pod allowances only determines the amount of pods that someone approved to move on his behalf. But this does not state at which start and which index, hence he can transfer much more value than the user wanted to.
In the pod marketplace, we can see that there is a protection for fillPodOrder for the maxPlaceLine.

function _fillPodOrder(
PodOrder calldata podOrder,
address filler,
uint256 index,
uint256 start,
uint256 podAmount,
LibTransfer.To mode
) internal {
...
require(
(index + start + podAmount - s.sys.fields[podOrder.fieldId].harvestable) <=
podOrder.maxPlaceInLine,
"Marketplace: Plot too far in line."
);
...
}

This protection is coded because if a user creates a pod order to buy x pods, somebody could buy a large amount of pods and fill the order with the most far pods.
With this protection, the orderer can determine a limit to the pods he is willing to buy for them to not be really far from the harvestable index.
However, for the allowance to transfer pods on behalf of somebody else there is no protection for that.
Imagine that somebody want to allow an other user to move x pods thinking that these pods will be the farest ones and will have a certain value, the approved address can move the same amount of pods from the closest index to the harvestable. Essentially this would allow the approved user to move more value than the user initially wanted.
It happens the same with the index, if a user has multiple plot indexes, an approved user can move whichever index he wants and at the start he wants.

Impact

Low

Tools Used

Manual review

Recommendations

I would recommend to constrain the approval of plots by adding the index the user is approving the other user to move plots from. And also add the start index to completely determine which pods the approved user can move. That's because not all pods have the same value and should be constrained properly in order to give the user the full control of his pods.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Plot allowances should have defined the index and start. Not all plots are the same judging by their place in harvestable queue

Support

FAQs

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