Beanstalk: The Finale

Summary

fundsSafu modifier is used to ensure that Protocol has enough balances to cover all the obligations after functino execution.

Problem is that it doesn't include Bean amounts contributed while creating PodOrder in Market module. It introduces significant flaw to invariant logic:

1. Suppose Attacker found the way to drain Bean from protocol.

2. fundsSafu modifier should prevent such exploit

3. Attacker performs exploit and drains Bean. He can drain as much Bean as deposited to Market. Because Beans deposited to Market are not accounted in fundsSafu

4. Moreover user can submit his own Beans prior to exploit and perform Attack from different address. In this case he doesn't risk funds, however he expects compensation for deposited funds from DAO. In this case DAO can't distinguish real users' addresses and attacker's addresses and there is no easy way to compensate users of Market.

Vulnerability Details

getTokenEntitlementsAndBalances doesn't account Beans deposited via Market:
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/df2dd129a878d16d4adc75049179ac0029d9a96b/protocol/contracts/beanstalk/Invariable.sol#L182-L190

And Market transfer Beans to address(this) during PodOrder creation:
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/df2dd129a878d16d4adc75049179ac0029d9a96b/protocol/contracts/beanstalk/market/MarketplaceFacet/MarketplaceFacet.sol#L70

Impact

Invariable.sol is included in scope to this audit, however severity classification for this contract should differ from standard one. This contract is circuit breaker which must halt execution when funds are drained so that Critical exploit can't occur.

I believe it's High severity issue if Invariable fails to prevent drain of funds because the way it calculates entitlements can be circumvented. This issue shows exactly how during Critical exploit attacker Invariable can be circumvented/

I believe Invariable must prevent drain of funds and if it fails to do, it is High severity vulnerability regardless of the fact whether there is critical exploit or not.

Let me explain this logic where exists drain of funds by 2 examples:

1. Invariable works correctly. It makes issue with drain funds invalid - drain of funds just can't occur.

2. Invariable works incorrectly. It makes drain of funds Critical issue - and therefore issue in Invariable logic is by definition Critical issue.

Tools Used

Manual Review

Recommendations

Introduce storage variable tom track Bean amount deposited to Market on order creation. And add handle logic to Invariable to account it.