Beanstalk Part 3

Summary

The LibConvert library allows unauthorized users to change the deposits of other accounts. This issue arises from the implementation of a feature designed to accommodate the Tractor specification, which permits specifying an account other than msg.sender for conversions. While initially intended to enhance flexibility, this design flaw enables anyone to initiate conversions on behalf of others without consent, leading to unauthorized position swaps and even worse would allow anyone to grief/ or cause DOS to the Tractor specification.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-05-Beanstalk-3/blob/662d26f12ee219ee92dc485c06e01a4cb5ee8dfb/protocol/contracts/libraries/Convert/LibConvert.sol#L36-L42

The above documentation, now show cases that protocol allows for anyone to update the deposit of another account.

This can be further proven by this instance in the ConvertFacet: https://github.com/Cyfrin/2024-05-Beanstalk-3/blob/662d26f12ee219ee92dc485c06e01a4cb5ee8dfb/protocol/contracts/beanstalk/silo/ConvertFacet.sol#L86

Now from the BIP-X: Misc Improvements it was hinted that this implementation was done in order to allow the tractor specification of not passing in the msg.sender but rather with the input parameter of _convert, quoting them:

Note: This is additionally developed with the Tractor specification in mind; if this is developed after Tractor, we can replace msg.sender with the input parameter of _convert.

However having this implementation now means that anyone can call this and without permission of the "account", this leads to multiple issues:

• First unauthorized convertion of users positions, since there is still no assurance that all positions are going to be converted due to it being permisionless except if the team cynchronizedalls it themselves, then why not just restrict this to the account themselves and emergency admins that then call necessary accounts to ensure the BDV calculations are not too des

• Secondly, after the implementation of the Tractor specification this then means that anyone can grief valid attempts of the calling this function via the any context applied in the Tractor specification , during the contest it was relayed by the sponsors that this context is going to be called by the Tractor specification after being set and then the address passed in the the input parameter of _convert is going to be the cp.account which would showcase why we can't have this restricted to msg.sender since the msg.sender in that context is going to be the Tractor specification, however since this functionality can be called by anyone, then an attacker can always cause the attempt at any functionality that queries this via the Tractor specification to be DOS'd/griefed as they can just front run the the call from the Tractor specification and when the function is called via the Tractor specification it's going to revert since the state has already been set.

Impact

As hinted in Proof Of Concept this causes not only the unauthorized nature of swapping users positions, but would potentially also make core functionalities in the Tractor specification unavailable as an attacker can just front run the calls and have the Tractor specification's attempt revert.

Tools Used

Manual review

Recommendations

Consider applying some sort of access controls to the functionality, an emergency admin the Tractor specification if not any of this parties is not the one querying the functionality then ensure that the caller is the msg.sender.