📄

Report #24062

Report Date
September 12, 2023
Status
Closed
Payout

Potential Reentrancy Risk TokenFacet.sol: function transferToken()

Report Info

Report ID

#24062

Report type

Smart Contract

Has PoC?

Yes

Target

Impacts

Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield

Bug Description

Currently, almost all external callable functions of Beanstalk utilizes Oppenzepplin Reentrancy Gaurd with nonReentrant modifier. Except for transferToken function of TokenFacet.sol which is interesting and concerning. As this function plays a critical role in the movement of funds in and out of beanstalk. Current deposit TVL value of $22 Million

Although ERC20 tokens are not susceptible to reentrancy, safeTransferFrom() is.

Grim Finance 2021, was hacked of $30 Million due to safeTransferFrom() reentrancy loop. (see references)

Key Point is: In case of Grim, a call back hook was not needed just an important unguarded depositFor() function that utilized safeTransferFrom().

The following function: TokenFacet.sol

    function transferToken(
        IERC20 token,
        address recipient,
        uint256 amount,
        LibTransfer.From fromMode,
        LibTransfer.To toMode
    ) external payable {
        LibTransfer.transferToken(
            token,
            msg.sender,
            recipient,
            amount,
            fromMode,
            toMode
        );
    }

Calls: LibTransfer.sol

    function transferToken(
        IERC20 token,
        address sender,
        address recipient,
        uint256 amount,
        From fromMode,
        To toMode
    ) internal returns (uint256 transferredAmount) {
        if (fromMode == From.EXTERNAL && toMode == To.EXTERNAL) {
            uint256 beforeBalance = token.balanceOf(recipient);
            token.safeTransferFrom(sender, recipient, amount);
            return token.balanceOf(recipient).sub(beforeBalance);
        }
        amount = receiveToken(token, amount, sender, fromMode);
        sendToken(token, amount, recipient, toMode);
        return amount;
    }

that calls:

    function sendToken(
        IERC20 token,
        uint256 amount,
        address recipient,
        To mode
    ) internal {
        if (amount == 0) return;
        if (mode == To.INTERNAL)
            LibBalance.increaseInternalBalance(recipient, token, amount);
        else token.safeTransfer(recipient, amount);
    }

Impact

A well executed reentrancy attack can lead to direct loss of funds. An additional risk vector is in block reorgs and miner block manipulation that happens probabilistically. How an attacker may use this is limited to capability because of the non-protected function. Unless there is a really good functional reason not to have the protective layer a mutex should be added.

Risk Breakdown

Difficulty to Exploit: Easy Weakness: CVSS2 Score:

Recommendation

Add nonReentrant modifier to transferToken() of TokenFacet.sol as in all other external functions within the contract.

References

Proof of concept

Proof of Concept:

  1. Call beanstalk.transferToken() with fromMode 0, and toMode 1.
  2. Causing Attacker1 to transferToken amount to an internal balance of attacker 2. example:(beanstalk.transferToken(address(bean), address(attacker2), 100e18, 0, 1);
  3. Call same function in loop until loop breaks.

simple idea: to transfer amount to internal balance while the other amount is still being processed by transferToken() function flow

  1. Cause the internal balance of attacker 2 to be artificially inflated.
  2. Call transferToken with inflated amount as attacker 2 with internal to external mode example: (beanstalk.transferToken(address(bean), address(attacker2), 2000e18, 1, 0)
  3. Withdraw larger funds than was originally deposited.

Again this maybe depended on chain state manipulation like block reorgs and time.stamp manipulation, or other creative variables introduced by a skilled hacker. Which is very hard to simulate using Foundry.

I can work on a POC using Foundry vm.rewind and skip state manipulations as closest simulations of a possible attack.

BIC Response

After significant investigation we have found the following:

The Grim exploit is different in that regardless of what token address is input, the same state variables are accessed. Thus, by using a custom contract with a transferFrom function that reenters the same function, they are able to rerun the logic that gets run when the actual token is input. Thus, they can reenter as many times as they want, send the desired tokens to the contract and then perform the mint operation.

transferToken only accesses token state specific info, so there is no way to use a custom contract to perform a reentrancy attack on state corresponding to real ERC-20 tokens.

There is an ability to reenter transferToken if an ERC-777 token is used, but given that balance state variables are decremented before tokens are received and balance state variables are incremented before tokens are sent out, it does not seem to be possible to perform manipulation.

Thus, the POC does not work in the case of Beanstalk.

Thank you for you thorough report, but for the above reasons, we are closing the submission and no reward will be issued.