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

Possible loss of user's balances after calling `addMigratedDepositsToAccount()`.

bladesec

Github link

https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/9c7b9fd521ad7cbe65cc788df181887c0eb39c6d/protocol/contracts/beanstalk/silo/L2ContractMigrationFacet.sol#L169-L170

https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/9c7b9fd521ad7cbe65cc788df181887c0eb39c6d/protocol/contracts/beanstalk/silo/L2ContractMigrationFacet.sol#L190-L191

Summary

While migrating deposits to L2, addMigratedDepositsToAccount() doesn't accumulate the user's balances properly.

Vulnerability Details

Users can redeem their deposits and internal balances on L2 using redeemDepositsAndInternalBalances().

And redeemDepositsAndInternalBalances() calls addMigratedDepositsToAccount() to manage each deposit.

But in addMigratedDepositsToAccount(), it just overwrites the user's balances and status without considering already existing amounts.

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");
​
// add deposit to account.
s.accts[account].deposits[depositId].amount = depositData.amounts[i]; //@audit overwrite
s.accts[account].deposits[depositId].bdv = depositData.bdvs[i]; //@audit overwrite
​
// increment totalBdvForAccount by bdv of deposit:
totalBdvForAccount += depositData.bdvs[i];
​
// increment by grown stalk of deposit.
accountStalk += uint96(stemTip - stem) * depositData.bdvs[i];
​
// emit events.
emit AddDeposit(
account,
depositData.token,
stem,
depositData.amounts[i],
depositData.bdvs[i]
);
emit TransferSingle(msg.sender, address(0), account, depositId, depositData.amounts[i]);
}
​
// update mowStatuses for account and token.
s.accts[account].mowStatuses[depositData.token].bdv = totalBdvForAccount; //@audit overwrite
s.accts[account].mowStatuses[depositData.token].lastStem = stemTip; //@audit overwrite
​
// set global state
s.sys.silo.balances[depositData.token].deposited = totalDeposited;
s.sys.silo.balances[depositData.token].depositedBdv = totalDepositedBdv;
​
// increment stalkForAccount by the stalk issued per BDV.
// placed outside of loop for gas effiency.
accountStalk += stalkIssuedPerBdv * totalBdvForAccount;
}

So if a user has positve balances before the migration or multiple migrations are executed for the same receiver, user balances will be overwritten and the previous state will be lost.

Impact

Users might lose some balances after migrating to L2.

Tools Used

Manual Review

Recommendations

addMigratedDepositsToAccount() should accmulate balances instead of overwriting.

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");
​
// add deposit to account.
- s.accts[account].deposits[depositId].amount = depositData.amounts[i];
- s.accts[account].deposits[depositId].bdv = depositData.bdvs[i];
​
+ s.accts[account].deposits[depositId].amount += depositData.amounts[i];
+ s.accts[account].deposits[depositId].bdv += depositData.bdvs[i];
​
// increment totalBdvForAccount by bdv of deposit:
totalBdvForAccount += depositData.bdvs[i];
​
// increment by grown stalk of deposit.
accountStalk += uint96(stemTip - stem) * depositData.bdvs[i];
​
// emit events.
emit AddDeposit(
account,
depositData.token,
stem,
depositData.amounts[i],
depositData.bdvs[i]
);
emit TransferSingle(msg.sender, address(0), account, depositId, depositData.amounts[i]);
}
​
// update mowStatuses for account and token.
- s.accts[account].mowStatuses[depositData.token].bdv = totalBdvForAccount;
- s.accts[account].mowStatuses[depositData.token].lastStem = stemTip;
​
+ s.accts[account].mowStatuses[depositData.token].bdv = totalBdvForAccount;
+ s.accts[account].mowStatuses[depositData.token].lastStem = stemTip;
​
// set global state
s.sys.silo.balances[depositData.token].deposited = totalDeposited;
s.sys.silo.balances[depositData.token].depositedBdv = totalDepositedBdv;
​
// increment stalkForAccount by the stalk issued per BDV.
// placed outside of loop for gas effiency.
accountStalk += stalkIssuedPerBdv * totalBdvForAccount;
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

`L2ContractMigrationFacet` overwrites user's deposit, instead of increasing it

Support

FAQs

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