Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield
Description
Lack of reentrancy check in advanceFarmCall function can lead to loss of eth for beanstalk and also for victims with extra eth value attached to transfers of ERC115 to malicious contract.
Vulnerability Details
A similiar bug was reported by Cyfrin in an audit report, but the ability to steal eth value directly from beanstalk was not highlighted.
In addition, over its lifetime beanstalk Diamond contract had over 10 eth and this the loss of this eth was highly possible. (see snapshot attached)
Basically, the function advanceFarmCall does not have reentrancy gaurd and therefore allows anyone transferring eth to re-enter beanstalk address and take any eth associated with the call including beanstalk eth balance if any.
Stealing eth from Victim:
Victim calls advanceFarmCall to send ERC1155 to a malicious contract
Malicious contract calls back into onERC1155Received and removes all ETH
Steal eth from beanstalk
Beanstalk get's balance of ETH
Malicious Actors sends 1 wei to himself
Malicious actor reenters onERC1155Received and removes all eth balance.
Impact Details
Any address that interacts with a malicious contract and has extra eth attached to the call and happens to call advanceFarmCall function the eth can be stolen.
If for any reasons beanstalk holds a balance eth, (like in the past) it can directly be stolen
Although beanstalk does not have eth balance now, historically it had up to 10 ether. In addition, fertilizer is actively traded on OpenSea currently and users risk interacting with malicious actors.
References
AdvanceFarmCall function:
function advancedFarm(AdvancedFarmCall[] calldata data)
external
payable
withEth
returns (bytes[] memory results)
{
results = new bytes[](data.length);
for (uint256 i = 0; i < data.length; ++i) {
results[i] = _advancedFarm(data[i], results);
}
}
Proof of concept
Proof of Concept:
To illustrate the risk we deal beanstalk.Diamond contract 10 ether.
mkdir beanPOC
cd beanPOC
forge init
delete Counter.sol and Counter.t.sol and test folder
place POC and interface in src folder
run with forge test --contracts ./src/beanAttack.sol -vv --evm-version shanghai
advancedFarm does not have a reentrancy guard because the purpose of the function is to call multiple functions within Beanstalk in a single transaction. If advancedFarm had a reentrancy guard, the function would fail upon calling any nonReentrant function (rendering it effectively useless). Furthermore, the issue described in the report requires someone to send ETH directly to Beanstalk, which is considered user error.
Ran 1 test for src/beanAttack3.sol:ContractTest
[PASS] testFarm() (gas: 624995)
Logs:
Exploiter balance before : : 1
BEANSTALK balance before : : 10000000000000000000
Exploiter balance before : : 1
Remaining Capitalization before : : 38444145899132
entered exploiter onERC1155Received
Exploiter balance after : : 0
BEANSTALK balance after : : 0
Exploiter balance after : : 10000000000000000002
Remaining Capitalization after : : 38411881899132
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 848.78ms (16.23ms CPU time)