Report #16898

Report ID

#16898

Target

https://etherscan.io/address/0xd231498144c5b53b65b782343cdfb366472c7bf7

Report type

Smart Contract

Impacts

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

Has PoC?

Yes

Bug Description

In CurveFacet contract, a malicious pool can drain all the funds in the contract. There are several places in the contract where Beanstalk trusts the return of calls to the Curve pool. I focused for the description of the bug and for the PoC in the exchange function, but the other impacted functions are also outlined in the Recommendations section of this report. The exchange function in the CurveFacet contract aims to exchange fromToken for toToken. Ultimately, the amount to transfer back to the sender is given by the call to the Curve pool.

As seen above, in the last line there is a transfer to the user of amountOut tokens. If hasNoReturnValue(pool) == true, the amount is calculated correctly, because the contract just sends to the caller the increment in tokens after the call to exchange. But if it is false, there is no calculation, the contract would trust blindlessly the return from the call to exchange in the Curve pool, and if it is malicious, it could drain all the funds.

Impact

Critical. All the funds from the Beanstalk contract (valued at more than $228,000,000 at the time of writing this report) are at risk. Attacker must be able to create a Pool in curve with a custom pool implementation. Having an attack vector this critical that relies on a third party should not be allowed by Beanstalk.

Risk Breakdown

Difficulty to Exploit: Medium

Recommendation

Use the same approach seen above when hasNoReturnValue(pool) == true of always store the balanceBefore of the toToken and just transfer to the user amountOut = IERC20(toToken).balanceOf(address(this)).sub(beforeBalance);. The updated exchange function would be like this:

Other places in the contract that trust blindlessly the return of calls to the pool are:

exchangeUnderlying Change this snippet:

...
    } else {
		uint256 amountOut = ICurvePool(pool).exchange_underlying(
			i,
			j,
			amountIn,
			minAmountOut
		);
		msg.sender.increaseInternalBalance(IERC20(toToken), amountOut);
	}
...

With this:

...
    } else {
		uint256 beforeBalance = IERC20(toToken).balanceOf(address(this));
		ICurvePool(pool).exchange_underlying(
			i,
			j,
			amountIn,
			minAmountOut
		);
		uint256 amountOut = IERC20(toToken).balanceOf(address(this)).sub(beforeBalance);
		msg.sender.increaseInternalBalance(IERC20(toToken), amountOut);
	}
...

addLiquidity Change this snippet:

With this:

removeLiquidityOneToken Change this snippet:

...
} else {
	uint256 amountOut = ICurvePool(pool).remove_liquidity_one_coin(
		amountIn,
		i,
		minAmountOut
	);
	msg.sender.increaseInternalBalance(IERC20(toToken), amountOut);
}
...

With this:

...
} else {
	uint256 beforeBalance = IERC20(toToken).balanceOf(address(this));
	ICurvePool(pool).remove_liquidity_one_coin(
		amountIn,
		i,
		minAmountOut
	);
	uint256 amountOut = IERC20(toToken).balanceOf(address(this)).sub(beforeBalance);
	msg.sender.increaseInternalBalance(IERC20(toToken), amountOut);
}
...

References

https://etherscan.io/address/0xd231498144c5b53b65b782343cdfb366472c7bf7#code

Proof of concept

I wrote a hardhat test to prove how a user that can create a fake pool could steal all the funds. The PoC, for simplicity only steals one token (in this case the urBEAN3CRV token), but the same method could be used by an attacker to steal the other holdings of the Beanstalk contract. First we have the contract that acts as our fake pool:

And then the hardhat test that deploys the fake pool contract, creates the pool in Curve registry and steals the funds.