Beanstalk

In the FertilizerFacet, the mintFertilizer() function is used to buy Fertilizer in exchange for USDC:

FertilizerFacet.sol

function mintFertilizer(
    uint128 amount,
    uint256 minLP,
    LibTransfer.From mode
) external payable {
    uint256 remaining = LibFertilizer.remainingRecapitalization();
    uint256 _amount = uint256(amount);
    if (_amount > remaining) _amount = remaining;
    LibTransfer.receiveToken(
        C.usdc(),
        uint256(amount).mul(1e6),
        msg.sender,
        mode
    );
    uint128 id = LibFertilizer.addFertilizer(
        uint128(s.season.current),
        amount,
        minLP
    );
    C.fertilizer().beanstalkMint(msg.sender, uint256(id), amount, s.bpf);
}

\color{black} \color{white}

This function has the same issue that was described in HAL01 - INTERNAL BALANCE TOKENS CAN BE DRAINED THROUGH THE CURVEFACET.EXCHANGEUNDERLYING FUNCTION as the value returned by receiveToken() is not checked, users can abuse this and buy Fertilizer with the USDC of other users internal balance through the INTERNAL_TOLERANT fromMode.

SOLVED: The Beanstalk team corrected the issue by considering the returned value of the receiveToken() call:

FertilizerFacet.sol

function mintFertilizer(
    uint128 amount,
    uint256 minLP,
    LibTransfer.From mode
) external payable {
    uint128 remaining = uint128(LibFertilizer.remainingRecapitalization()); // remaining <= 77_000_000 so downcasting is safe.
    if (amount > remaining) amount = remaining;
    amount = uint128(LibTransfer.receiveToken(
        C.usdc(),
        uint256(amount).mul(1e6),
        msg.sender,
        mode
    ).div(1e6)); // return value <= amount, so downcasting is safe.
    uint128 id = LibFertilizer.addFertilizer(
        uint128(s.season.current),
        amount,
        minLP
    );
    C.fertilizer().beanstalkMint(msg.sender, uint256(id), amount, s.bpf);
}

\color{black} \color{white}

In the library LibTransfer, used by the TokenFacet contract, the transferToken() function assume that the amount of token is transferred to the smart contract after calling token.safeTransferFrom(sender, address(this), amount - receivedAmount); (and thus it updates the states variables accordingly). For example:

LibTransfer.sol

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

function receiveToken(
    IERC20 token,
    uint256 amount,
    address sender,
    From mode
) internal returns (uint256 receivedAmount) {
    if (amount == 0) return 0;
    if (mode != From.EXTERNAL) {
        receivedAmount = LibBalance.decreaseInternalBalance(
            sender,
            token,
            amount,
            mode != From.INTERNAL
        );
        if (amount == receivedAmount || mode == From.INTERNAL_TOLERANT)
            return receivedAmount;
    }
    token.safeTransferFrom(sender, address(this), amount - receivedAmount);
    return amount;
}

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);
}

\color{black} \color{white}

However, this may not be true if the token is a transfer-on-fee token or a deflationary/rebasing token, causing the received amount to be less than the accounted amount in the different state variables.

SOLVED: The Beanstalk team addressed the issue and now supports transfer-on-fee tokens:

LibTransfer.sol

function transferToken(
    IERC20 token,
    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(msg.sender, recipient, amount);
        return token.balanceOf(recipient).sub(beforeBalance);
    }
    amount = receiveToken(token, amount, msg.sender, fromMode);
    sendToken(token, amount, recipient, toMode);
    return amount;
}

function receiveToken(
    IERC20 token,
    uint256 amount,
    address sender,
    From mode
) internal returns (uint256 receivedAmount) {
    if (amount == 0) return 0;
    if (mode != From.EXTERNAL) {
        receivedAmount = LibBalance.decreaseInternalBalance(
            sender,
            token,
            amount,
            mode != From.INTERNAL
        );
        if (amount == receivedAmount || mode == From.INTERNAL_TOLERANT)
            return receivedAmount;
    }
    uint256 beforeBalance = token.balanceOf(address(this));
    token.safeTransferFrom(sender, address(this), amount - receivedAmount);
    return receivedAmount.add(token.balanceOf(address(this)).sub(beforeBalance));
}

\color{black} \color{white}

In the FertilizerFacet contract, the mintFertilizer() function checks if the amount provided by the user is higher than the remaining amount of Fertilizer and if that is the case, _amount is overwritten with the remaining Fertilizer preventing users to buy more Fertilizer than what is remaining:

FertilizerFacet.sol

function mintFertilizer(
    uint128 amount,
    uint256 minLP,
    LibTransfer.From mode
) external payable {
    uint256 remaining = LibFertilizer.remainingRecapitalization();
    uint256 _amount = uint256(amount);
    if (_amount > remaining) _amount = remaining;
    LibTransfer.receiveToken(
        C.usdc(),
        uint256(amount).mul(1e6),
        msg.sender,
        mode
    );
    uint128 id = LibFertilizer.addFertilizer(
        uint128(s.season.current),
        amount,
        minLP
    );
    C.fertilizer().beanstalkMint(msg.sender, uint256(id), amount, s.bpf);
}

\color{black} \color{white}

Although, the contract wrongly uses the amount variable instead of _amount allowing users to mint more Fertilizer than what is remaining:

SOLVED: The Beanstalk team corrected the issue:

FertilizerFacet.sol

function mintFertilizer(
    uint128 amount,
    uint256 minLP,
    LibTransfer.From mode
) external payable {
    uint128 remaining = uint128(LibFertilizer.remainingRecapitalization()); // remaining <= 77_000_000 so downcasting is safe.
    if (amount > remaining) amount = remaining;
    amount = uint128(LibTransfer.receiveToken(
        C.usdc(),
        uint256(amount).mul(1e6),
        msg.sender,
        mode
    ).div(1e6)); // return value <= amount, so downcasting is safe.
    uint128 id = LibFertilizer.addFertilizer(
        uint128(s.season.current),
        amount,
        minLP
    );
    C.fertilizer().beanstalkMint(msg.sender, uint256(id), amount, s.bpf);
}

\color{black} \color{white}

The Fertilizer contract contains the following _beforeTokenTransfer() hook:

Fertilizer.sol

function _beforeTokenTransfer(
    address, // operator,
    address from,
    address to,
    uint256[] memory ids,
    uint256[] memory, // amounts
    bytes memory // data
) internal virtual override {
    uint256 bpf = uint256(IBS(owner()).beansPerFertilizer());
    if (from != address(0)) _update(from, ids, bpf);
    _update(to, ids, bpf);
}

This hook will be called with every safeTransferFrom() or safeBatchTransferFrom() call and will claim the fertilizer claimable amount automatically on behalf of the sender:

If the amount of claimable fertilizer is zero, the receiver will get the full unfertilized amount as expected:

This could allow the following scenario:

1. By making use of a third-party marketplace, user1 puts for sale his Fertilizer at a low price. That fertilizer id can be fully claimed at that time.
2. User2 buys the fertilizer planning to claim it afterwards and make some profit, but the fertilizer is claimed automatically on behalf of user1 during the safeTransferFrom() call and the user2 just receives an already claimed fertilizer.

RISK ACCEPTED: The Beanstalk team accepts this risk.

In the SeasonFacet contract, the incentivize() function is used to send some Beans to the user that successfully called sunrise() to start a new season:

SeasonFacet.sol

function incentivize(address account, uint256 amount) private {
    uint256 timestamp = block.timestamp.sub(
        s.season.start.add(s.season.period.mul(season()))
    );
    if (timestamp > 300) timestamp = 300;
    uint256 incentive = LibIncentive.fracExp(amount, 100, timestamp, 1);
    C.bean().mint(account, amount);
    emit Incentivization(account, incentive);
}

\color{black} \color{white}

As we can see, the rewards/timestamp is capped at a maximum of 300 seconds and makes use of exponential rewards. But then, in the mint call, the amount parameter is incorrectly used instead of incentive, which means that the caller will always receive a fixed amount of beans (100):

SOLVED: The Beanstalk team corrected the issue and updated the code as suggested.

In the TokenFacet contract, the wrapEth(uint256 amount, LibTransfer.To mode) function wraps the amountof Ether into WETH and sends it to the user internal/external balance:

TokenFacet.sol

function wrapEth(uint256 amount, LibTransfer.To mode) external payable {
    LibWeth.wrap(amount, mode);
}

LibWeth.sol

function wrap(uint256 amount, LibTransfer.To mode) internal {
    deposit(amount);
    LibTransfer.sendToken(IERC20(WETH), amount, msg.sender, mode);
}

\color{black} \color{white}

As the msg.value is never compared to the amount parameter, if the msg.value sent by the user was higher than the amount the difference would be taken by the contract and any other user would be able to steal it.

SOLVED: The Beanstalk team corrected the issue. Ether refunds were added instead of a require check. If there is leftover Ether in the contract, then it will be refunded.

In the FertilizerPreMint, the function buy() is used to swap Ether into USDC through the UniswapV3 router:

FertilizerPreMint.sol

function buy(uint256 minAmountOut) private returns (uint256 amountOut) {
    IWETH(WETH).deposit{value: msg.value}();
    ISwapRouter.ExactInputSingleParams memory params =
        ISwapRouter.ExactInputSingleParams({
            tokenIn: WETH,
            tokenOut: USDC,
            fee: POOL_FEE,
            recipient: CUSTODIAN,
            deadline: block.timestamp,
            amountIn: msg.value,
            amountOutMinimum: minAmountOut,
            sqrtPriceLimitX96: 0
        });
    amountOut = ISwapRouter(SWAP_ROUTER).exactInputSingle(params);
}

\color{black} \color{white}

The amountOutMinimum is set with a user controlled parameter minAmountOut. If the Ether sent through msg.value is higher than the minAmountOut in USDC the transaction may get sandwiched causing the user to swap Ether for USDC at a higher cost, receiving less USDC for the same amount of Ether.

The issue was flagged as informational, as there is a function in the FertilizerPreMint contract that allows to get the exact amount of USDC for a given amount of Ether after swap. We assume that this function is used in the backend mitigating the issue. Only users interacting with the smart contract directly may have the problem described.

SOLVED: The Beanstalk team documented their code mentioning that any slippage should be properly accounted by the users:

FertilizerPreMint.sol

// Note: Slippage should be properly be accounted for in
// minBuyAmount when calling the buyAndMint function directly.
function buyAndMint(uint256 minBuyAmount) external payable nonReentrant {
    uint256 amount = buy(minBuyAmount);
    require(IUSDC.balanceOf(CUSTODIAN) <= MAX_RAISE, "Fertilizer: Not enough remaining");
    __mint(amount);
}

\color{black} \color{white}

In the MarketplaceFacet, the functions createPodListing() and createPodOrder() make use of an uint24 to hold the pricePerPod parameter.

As the maximum value that an uint24 can hold is 16_777215 the users will not be able to set a price higher than that for a Pod.

MarketplaceFacet.sol

function createPodListing(
    uint256 index,
    uint256 start,
    uint256 amount,
    uint24 pricePerPod,
    uint256 maxHarvestableIndex,
    LibTransfer.To mode
) external payable {
    _createPodListing(
        index,
        start,
        amount,
        pricePerPod,
        maxHarvestableIndex,
        mode
    );
}

\color{black} \color{white}

MarketplaceFacet.sol

function createPodOrder(
    uint256 beanAmount,
    uint24 pricePerPod,
    uint256 maxPlaceInLine,
    LibTransfer.From mode
) external payable returns (bytes32 id) {
    beanAmount = LibTransfer.receiveToken(C.bean(), beanAmount, msg.sender, mode);
    return _createPodOrder(beanAmount, pricePerPod, maxPlaceInLine);
}

\color{black} \color{white}

SOLVED: The Beanstalk team documented their code mentioning that the highest price to list a Pod for is 16_777215 Beans:

MarketplaceFacet.sol

// Note: pricePerPod is bounded by 16_777_215 Beans.
function createPodListing(
    uint256 index,
    uint256 start,
    uint256 amount,
    uint24 pricePerPod,
    uint256 maxHarvestableIndex,
    LibTransfer.To mode
) external payable {
    _createPodListing(
        index,
        start,
        amount,
        pricePerPod,
        maxHarvestableIndex,
        mode
    );
}

\color{black} \color{white}

MarketplaceFacet.sol

// Note: pricePerPod is bounded by 16_777_215 Beans.
function createPodOrder(
    uint256 beanAmount,
    uint24 pricePerPod,
    uint256 maxPlaceInLine,
    LibTransfer.From mode
) external payable returns (bytes32 id) {
    beanAmount = LibTransfer.receiveToken(C.bean(), beanAmount, msg.sender, mode);
    return _createPodOrder(beanAmount, pricePerPod, maxPlaceInLine);
}

\color{black} \color{white}

The FarmFacet allows performing multiple delegatecalls inside a for loop:

FarmFacet.sol

function _farm(bytes calldata data) private {
    LibDiamond.DiamondStorage storage ds;
    bytes32 position = LibDiamond.DIAMOND_STORAGE_POSITION;
    assembly {
        ds.slot := position
    }
    bytes4 functionSelector;
    assembly {
        functionSelector := calldataload(data.offset)
    }
    address facet = ds
        .selectorToFacetAndPosition[functionSelector]
        .facetAddress;
    require(facet != address(0), "Diamond: Function does not exist");
    (bool success, ) = address(facet).delegatecall(data);
    require(success, "FarmFacet: Function call failed!");
}

function farm(bytes[] calldata data) external payable {
    for (uint256 i = 0; i < data.length; i++) {
        _farm(data[i]);
    }
    if (msg.value > 0 && address(this).balance > 0) {
        (bool success, ) = msg.sender.call{value: address(this).balance}(
            new bytes(0)
        );
        require(success, "Farm: Eth transfer Failed.");
    }
}

\color{black} \color{white}

In this situation, msg.sender and msg.value would be persisted across the different iterations/delegatecalls in the loop. For example, a user could submit 1 Ether as msg.value to the farm(bytes[] calldata data) call and in the data array add 3 different calls that each of those made use of that Ether. If the Diamond contract had some Ether, user would be paying just that Ether and the 2 remaining Ether would be taken from the smart contract balance.

Currently, there is no exploitation path for this issue, as the contracts should never be holding any Ether. Also, the remaining Ether in the contract is sent back to msg.sender after the _farm() calls.

For this reason, we have set this risk as informational.

Multi Delegatecall: Solidity 0.8 samczsun's blog post

SOLVED/ACKNOWLEDGED: The Beanstalk team is aware of the issue and will take this into account in future upgrades.

In the CurveFacet there are multiple functions that make use of the approveToken() function, for example:

CurveFacet.sol

function exchange(
    address pool,
    address fromToken,
    address toToken,
    uint256 amountIn,
    uint256 minAmountOut,
    bool stable,
    LibTransfer.From fromMode,
    LibTransfer.To toMode
) external payable nonReentrant {
    (int128 i, int128 j) = getIandJ(fromToken, toToken, pool, stable);
    amountIn = IERC20(fromToken).receiveToken(
        amountIn,
        msg.sender,
        fromMode
    );
    IERC20(fromToken).approveToken(pool, amountIn);

    if (toMode == LibTransfer.To.EXTERNAL) {
        ICurvePoolR(pool).exchange(
            i,
            j,
            amountIn,
            minAmountOut,
            msg.sender
        );
    } else {
        uint256 amountOut = ICurvePool(pool).exchange(
            i,
            j,
            amountIn,
            minAmountOut
        );
        msg.sender.increaseInternalBalance(IERC20(toToken), amountOut);
    }
}

\color{black} \color{white}

pool and fromToken are user controlled parameters. On the other hand, the LibTransfer.From fromMode set to INTERNAL_TOLERANT would allow anyone to bypass this receiveToken() call.

The only blocker to avoid an attacker of approving his own address and extract all the tokens of the contract is the following require statement:

CurveFacet.sol

function getIandJ(
    address from,
    address to,
    address pool,
    bool stable
) private view returns (int128 i, int128 j) {
    address factory = stable ? STABLE_FACTORY : CRYPTO_FACTORY;
    address[4] memory coins = ICurveFactory(factory).get_coins(pool);
    i = 4;
    j = 4;
    for (uint256 _i = 0; _i < 4; ++_i) {
        if (coins[_i] == from) i = int128(_i);
        else if (coins[_i] == to) j = int128(_i);
        else if (coins[_i] == address(0)) break;
    }
    require(i < 4 && j < 4, "Curve: Tokens not in pool");
}

In case of a malicious Curve Metapool Factory (0xB9fC157394Af804a3578134A6585C0dc9cc990d4 or 0x0959158b6040D32d04c301A72CBFD6b39E21c9AE), all the tokens in the contracts could be drained.

ACKNOWLEDGED: The Beanstalk team acknowledges this.

SafeERC20.safeTransferFrom() is used in all the code base. Although in the LibTransfer.transferToken() function, the standard ERC20.transferFrom() is still used.

Code Location

LibTransfer.sol

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

SOLVED: Beanstalk team uses now SafeERC20 in all the token transfers.

In the following require statements some typos were detected:

LibBalance.sol

• Line 73: require(allowPartial || (currentBalance >= amount), "Balance: Insufficnent internal balance");

TokenSilo.sol

• Line 285: require(season <= s.season.current, "Claim: Withdrawal not recievable.");

LibFertilizer.sol

• Line 153: require(s.activeFertilizer == 0, "Still active fertliizer");

SOLVED: Beanstalk team corrected the typos suggested.

Currently, the FertilizerPreMint contract is deployed behind a TransparentUpgradeableProxy.

After the replanting, when Beanstalk is unpaused, the BCM will call the function addFertilizerOwner() which will handle the process of adding BEAN:3CRV liquidity and minting new Deposited Beans for all the Fertilizer minted prior to unpause.

At the same time, the TransparentUpgradeableProxy contract will be upgraded to a new Fertilizer contract, instead of the FertilizerPreMint implementation used before. This will move the mintFertilizer() functionality to Beanstalk itself, instead of happening in the FertilizerPreMint contract.

At this point, Beanstalk will automatically add new liquidity for Unripe LP holders and new Beans in the same transaction as when Fertilizer is minted.

The new Fertilizer contract that will be used contains an initialize() function:

Fertilizer.sol

function decreaseInternalBalance(
function initialize() public initializer { //@audit can be removed
    __Internallize_init("");
}

As the TransparentUpgradeableProxy holds all the storage variables and will be already initialized in the FertilizerPreMint implementation, any call to this function will revert as the contract will be already initialized, hence this initialize() function can be removed from the Fertilizer contract.

SOLVED: Beanstalk team removed the initialize() function from the Fertilizer contract.

As i is an uint256, it is already initialized to 0. uint256 i = 0 reassigns the 0 to i which wastes gas.

Code Location

Internalizer.sol

• Line 62: for (uint256 i = 0; i < accounts.length; i++) {

Fertilizer.sol

• Line 77: for (uint256 i = 0; i < ids.length; i++) {
• Line 90: for (uint256 i = 0; i < ids.length; i++) {
• Line 99: for (uint256 i = 0; i < ids.length; i++) {

Fertilizer1155.sol

• Line 67: for (uint256 i = 0; i < ids.length; ++i) {

TokenSilo.sol

• Line 210: for (uint256 i = 0; i < seasons.length; i++) {
• Line 268: for (uint256 i = 0; i < seasons.length; i++) {
• Line 325: for (uint256 i = 0; i < seasons.length; i++) {

SiloFacet.sol

• Line 140: for (uint256 i = 0; i < seasons.length; ++i) {

TokenFacet.sol

• Line 82: for (uint256 i = 0; i < tokens.length; i++) {
• Line 103: for (uint256 i = 0; i < tokens.length; i++) {
• Line 124: for (uint256 i = 0; i < tokens.length; i++) {
• Line 147: for (uint256 i = 0; i < tokens.length; i++) {

FieldFacet.sol

• Line 84: for (uint256 i = 0; i < plots.length; i++) {

FarmFacet.sol

• Line 44: for (uint256 i = 0; i < data.length; i++) {

CurveFacet.sol

• Line 109: for (uint256 i = 0; i < nCoins; ++i) {
• Line 167: for (uint256 i = 0; i < nCoins; i++)
• Line 174: for (uint256 i = 0; i < nCoins; i++)
• Line 186: for (uint256 i = 0; i < nCoins; i++)
• Line 191: for (uint256 i = 0; i < nCoins; ++i) {
• Line 246: for (uint256 i = 0; i < nCoins; ++i) {
• Line 296: for (uint256 _i = 0; _i < 4; ++_i) {
• Line 313: for (uint256 _i = 0; _i < 8; ++_i) {
• Line 329: for (uint256 _i = 0; _i < 4; ++_i) {

LibPlainCurveConvert.sol

• Line 79: for (uint256 k = 0; k < 256; k++) {

LibCurve.sol

• Line 56: for (uint256 _i = 0; _i < N_COINS; _i++) {
• Line 68: for (uint256 _i = 0; _i < 255; _i++) {
• Line 85: for (uint256 _i = 0; _i < xp.length; _i++) {
• Line 92: for (uint256 _i = 0; _i < 256; _i++) {
• Line 94: for (uint256 _j = 0; _j < xp.length; _j++) {

LibIncentive.sol

• Line 34: for (uint256 i = 0; i < p; ++i) {

SOLVED: Beanstalk team followed Halborn's suggestion reducing the gas costs.