Beanstalk: The Finale

Summary

This report identifies a potential issue in tFarmFacet.sol that could lead to locked ETH within the contract. The issue is related to the withEth modifier, which is designed to prevent Beanstalk functions called from FarmFacet from refunding ETH. However, the modifier itself could unintentionally lock ETH sent to the FarmFacet contract.

Vulnerability Details

The FarmFacet contract has two functions, farm and advancedFarm, that accept multiple calls to other contracts. These functions are both marked with the withEth modifier. This modifier checks if any ETH was sent with the function call (msg.value > 0). If so, it sets a flag (s.sys.isFarm) to 2 and clears the flag back to 1 before refunding any ETH using LibEth.refundEth.

The potential issue lies in the logic around ETH refunds. The withEth modifier only refunds ETH if the flag (s.sys.isFarm) is set to 1. However, the modifier itself sets the flag to 2 before the function call and only resets it to 1 after a potential refund. This means that any ETH sent directly to the FarmFacet contract (not through the farm or advancedFarm functions) will not trigger the refund logic and could become locked.

Impact

Loss of funds: Any ETH accidentally or intentionally sent directly to the FarmFacet contract will be locked and inaccessible.

Tools Used

Manual code review

Recommendations

1. Modify the withEth modifier to only set the flag (s.sys.isFarm) to 2 if a non-zero msg.value is encountered during the execution of the wrapped function call. This ensures the flag is only set when ETH is intended to be used.

2. Alternatively, consider adding a new function specifically designed to handle ETH deposits into the FarmFacet contract. This function should implement proper controls and a secure withdrawal mechanism for authorized users.

3. Review the usage of LibEth.refundEth to ensure it functions as intended within the context of the FarmFacet contract.