Beanstalk: The Finale
Beanstalk
DeFiHardhatFoundry
250,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

L2ContractMigrationFacet.sol uses constant Roots amount during migration which is wrong

T1MOH

Summary

As reward for staking in Silo users receive 2 currencies: Stalk and Roots. Stalk is governance token and Roots is some form of future Beans.

L2ContractMigrationFacet.sol is used to migrate Silo deposits to L2 and not forfeit users' previous rewards. Merkle root is used to verify that deposits provided by user are valid. Worth noting is that user migrates deposits by himself, so it can't be calculated in advance which user in which time will migrate.

For this reason grown Stalk is calculated to current moment, i.e. from deposit start to current cumulated value:
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/df2dd129a878d16d4adc75049179ac0029d9a96b/protocol/contracts/beanstalk/silo/L2ContractMigrationFacet.sol#L176

function addMigratedDepositsToAccount(
address account,
AccountDepositData calldata depositData
) internal returns (uint256 accountStalk) {
@> int96 stemTip = LibTokenSilo.stemTipForToken(depositData.token);
uint256 stalkIssuedPerBdv = s.sys.silo.assetSettings[depositData.token].stalkIssuedPerBdv;
uint128 totalBdvForAccount;
uint128 totalDeposited;
uint128 totalDepositedBdv;
for (uint256 i; i < depositData.depositIds.length; i++) {
// verify that depositId is valid.
uint256 depositId = depositData.depositIds[i];
@> (address depositToken, int96 stem) = depositId.unpackAddressAndStem();
require(depositToken == depositData.token, "Migration: INVALID_DEPOSIT_ID");
require(stemTip >= stem, "Migration: INVALID_STEM");
​
...
​
// increment by grown stalk of deposit.
@> accountStalk += uint96(stemTip - stem) * depositData.bdvs[i];
​
...
}
​
...
}

As you understood accountStalk increases with time. Problem is that Roots have the same logic, but it is hardcoded for each user. So Roots doesn't increase before migration is executed by user.

As a result, users lose accrued Roots from yet-not-migrated deposits because Roots is hardcoded.

Vulnerability Details

Here you can see that Roots value is strictly written in Merkle root and can grow with time:

function redeemDepositsAndInternalBalances(
address owner,
address reciever,
AccountDepositData[] calldata deposits,
AccountInternalBalance[] calldata internalBalances,
@> uint256 ownerRoots,
bytes32[] calldata proof,
uint256 deadline,
bytes calldata signature
) external payable fundsSafu noSupplyChange nonReentrant {
// verify deposits are valid.
// note: if the number of contracts that own deposits is small,
// deposits can be stored in bytecode rather than relying on a merkle tree.
@> verifyDepositsAndInternalBalances(owner, deposits, internalBalances, ownerRoots, proof);
​
...
​
// set stalk for account.
@> setStalk(reciever, accountStalk, ownerRoots);
}

Here you can read that roots reward to user increases with time:
https://docs.bean.money/developers/misc/faq#what-are-roots

And here is how it is done currently in Beanstalk:
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/df2dd129a878d16d4adc75049179ac0029d9a96b/protocol/contracts/libraries/Silo/LibSilo.sol#L161-L200

Impact

Users lose accrued Roots from yet-not-migrated deposits.

Tools Used

Manual Review

Recommendations

Develop a way to correctly migrate Roots which is not sensitive to migration time.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

T1MOH Submitter
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Root loss via L2ContractMigrationFacet migration

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.