Summary

The issue with the handling of s.a[account].sop.roots in the burnStalk function is related to the assignment of the total roots of an account directly to a specialized roots tracking variable (sop.roots) without accounting for the roots that are being burned. This could potentially lead to inaccuracies in the tracking of sop.roots, which seems to be intended to track a specific subset or state of roots (like those affected by oversaturation).

Vulnerability Details

`/**
 * @notice Burns Stalk and Roots from `account`.
 * @dev assumes all stalk are in the same `state`. If not the case,
 * use `burnActiveStalk` and `burnGerminatingStalk` instead.
 */
function burnStalk(address account, uint256 stalk, LibGerminate.Germinate germ) internal {
    AppStorage storage s = LibAppStorage.diamondStorage();
    if (stalk == 0) return;


    // increment user and total stalk and roots if not germinating:
    if (germ == LibGerminate.Germinate.NOT_GERMINATING) {
        uint256 roots = burnActiveStalk(account, stalk);


        // Oversaturated was previously referred to as Raining and thus
        // code references mentioning Rain really refer to Oversaturation
        // If Beanstalk is Oversaturated, subtract Roots from both the
        // account's and Beanstalk's Oversaturated Roots balances.
        // For more info on Oversaturation, See {Weather.handleRain}
        if (s.season.raining) {
            s.r.roots = s.r.roots.sub(roots);
            s.a[account].sop.roots = s.a[account].roots;
        }
    } else {
        burnGerminatingStalk(account, uint128(stalk), germ);
    }
}`

Suppose we have the following scenario:

An account has a total of 1000 roots.
The account decides to burn 200 roots.

In the function, the line:
s.a[account].sop.roots = s.a[account].roots;

is executed before or after the roots are burned. This line directly sets sop.roots to the current total roots (1000 in this case) without subtracting the 200 roots that are being burned. As a result, sop.roots would incorrectly still reflect 1000 instead of the correct 800.

Correct Approach:
A more accurate handling would involve adjusting sop.roots based on the roots being burned, similar to how stalk is adjusted. The code should look something like this:

// Assuming s.a[account].sop.roots should also be decremented by the amount of roots burned s.a[account].sop.roots -= amountOfRootsBeingBurned;

Impact

/**
* @notice Burns Stalk and Roots from account.
* @dev assumes all stalk are in the same state. If not the case,
* use burnActiveStalk and burnGerminatingStalk instead.
*/
impact: No roots will be burned which is the incorrect state.

Tools Used

Manual Review

Recommendations

// Assuming s.a[account].sop.roots should also be decremented by the amount of roots burned s.a[account].sop.roots -= amountOfRootsBeingBurned;