FieldFacet Manual Review Findings

FFT-01M: Dangerous Self-Transfer

Type	Severity	Location
Logical Fault		FieldFacet.sol:L74, L75

Description:

The transferPlot can lead to complete loss of a plot in case of misuse. A transfer of an arbitrary id, a start value of 0 and an arbitrary value of end to self will cause the plot to be completely nullified in the execution flow of insertPlot and removePlot. In detail, insertPlot will overwrite whatever data point existed in the user's id entry with the amount being transferred and the logical checks within removePlot will assume the whole amount is being transferred from the user thus completely erasing the data entry via the delete operator.

Example:

Copy
64function transferPlot(address sender, address recipient, uint256 id, uint256 start, uint256 end)
65    external
66{
67    require(sender != address(0), "Field: Transfer from 0 address.");
68    require(recipient != address(0), "Field: Transfer to 0 address.");
69    require(end > start, "Field: Pod range invalid.");
70    uint256 amount = plot(sender, id);
71    require(amount > 0, "Field: Plot not owned by user.");
72    require(amount >= end, "Field: Pod range too long.");
73    amount = end.sub(start);
74    insertPlot(recipient,id.add(start),amount);
75    removePlot(sender,id,start,end);
76    if (msg.sender != sender && allowancePods(sender, msg.sender) != uint256(-1)) {
77            decrementAllowancePods(sender, msg.sender, amount);
78    }
79    emit PlotTransfer(sender, recipient, id.add(start), amount);
80}

Recommendation:

We strongly advise the Beanstalk team to introduce a require check prohibiting a transfer to self within the transferPlot function.

Alleviation:

The code has been refactored to a dedicated MarketplaceFacet and now contains the logic check to prevent plot transfers to the same party alleviating this exhibit.