Beanstalk Notion
Beanstalk Notion
📄

Report #24381

Report Date
September 25, 2023
Status
Closed
Payout

Recursive Reentrancy - Manipulation Still Possible

‣
Report Info

Report ID

#24381

Report type

Smart Contract

Has PoC?

Yes

Target

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

Impacts

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

Bug Description

It seems the tokenTransfer() does not follow Check-and-Effects pattern when using external to internal mode.

There are two important operations that happen when tokenTransfer() is executed:

  1. bean transfers tokens according to (internal, external mode) settings
  2. beanstalk changes internal balances to correspond with relevant transfer in (2)

To follow CEI pattern internal balances must be changed before transfers are executed. As mentioned by you in last bug report.

However,

If an attacker transfer bean using external to internal mode example: beanstalk.transferToken(address(bean), address(attacker2), 10_000_000e6, 0, 1);

Following Happens:

  1. Diamond Proxy forwards call to TokenFacet.sol diamond.tokenFacet.transferToken()
  2. TokenFacet.transferToken checks balance of beanstalk diamond Proxy by calling bean.balanceOf()
  3. bean transfers tokens by calling bean.transferFrom() emits transfer and approval events
  4. Beanstalk diamond balance is checked again with bean.balanceOf()
  5. Then only the internal balance of attacker2 is changed by LibTransfer --> LibBalance.increaseInternalBalance

(please see attached images or run POC with -vvvv to examine)

This means the tokens are transferred and then only is internal balance changed.

All other transfer modes i.e. external to external external-internal to internal etc.

Follow CEI pattern by first adjusting internal balance before executing transfers.

Hence, transferToken is still exposed to reentrancy:

  1. Attacker crafts malicious ERC20 token modified with call back hooks
  2. Attacker transfers fake tokens to beanstalk
  3. Beanstalk safeTransferFrom() calls back into attacker malicious token
  4. Attacker executes valid bean transfer in reentrancy.
  5. Here the attacker has two options:

    6. 4.1. Listen to transfer and approval events of bean token and execute malicious logic

    4.2. Listen to tokens received and execute malicious logic

  6. All this is done before internal balance is changed

I have included a POC with malicious token that indicates where reentrancy is possible. The attack can be accomplished by a sufficiently advanced and highly motivated advesary.

A similiar hack happened with Akropolis

Reference: https://peckshield.medium.com/akropolis-incident-root-cause-analysis-c11ee59e05d4

Running 1 test for src/beanExploit.sol:ContractTest
[PASS] testTransfer() (gas: 226411)
Logs:
  1. ABLE TO DEPOSIT FAKE TOKEN
  Bean Balance before             : 22601029877590
  Bean Balance this before        : 10000000000000
  ----------------------------------------------------------------------
  2. REENTERED THROUGH FAKETOKEN TRANSFERFROM
  3. MORE REENTRANCY POSSIBLE HERE
  ----------------------------------------------------------------------
  Bean Balance after              : 22601029877590
  Bean Balance this after         : 10000000000000

Impact

Again, there is not any direct impact on funds but there are multiple scenario's to consider:

  1. Cross function reentrancy
  2. Cross Contract reentrancy
  3. Ongoing changes in code that attackers can leverage
  4. Important role transferToken function plays in moving beans in and out of protocol.
  5. Griefing attacks crafted in malicious token eg recursive loops, gas consumption.

Critical points:

  1. Attacker can reenter transferToken()
  2. Attacker can reenter with carefully crafted ERC20
  3. transferToken() handles all token deposit logic for beanstalk.

Risk Breakdown

Difficulty to Exploit: Easy Weakness: CVSS2 Score:

Recommendation

Fixes are very simple:

  1. nonReentrant Modifier
  2. validate tokens.

References

Reference: https://peckshield.medium.com/akropolis-incident-root-cause-analysis-c11ee59e05d4

Proof of concept

  1. mkdir beanPOC
  2. forge init
  3. Delete Counter.sol and Counter.t.sol
  4. Place beanAttack.sol and interfaces.sol in src folder

run with forge test --contracts ./src/beanAttack.sol -vvvv

BIC Response

This is not a valid bug report because there are no funds at risk and the report does not satisfy any of the Medium Impacts in Scope.

The report's Impact is marked as "Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield" despite the report itself stating that "Again, there is not any direct impact on funds".

Due to these reasons, we are closing the submission and no reward will be issued.