Potential Reentrancy Risk TokenFacet.sol: function transferToken()
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
https://www.halborn.com/blog/post/explained-the-grim-finance-hack-december-2021 https://rekt.news/grim-finance-rekt/https://twitter.com/RugDocIO/status/1472293712185151492
Proof of concept
Proof of Concept:
- Call beanstalk.transferToken() with fromMode 0, and toMode 1.
- Causing Attacker1 to transferToken amount to an internal balance of attacker 2.
example:(beanstalk.transferToken(address(bean), address(attacker2), 100e18, 0, 1); - 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
- Cause the internal balance of attacker 2 to be artificially inflated.
- Call transferToken with inflated amount as attacker 2 with internal to external mode example:
(beanstalk.transferToken(address(bean), address(attacker2), 2000e18, 1, 0) - 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.