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

`L2ContractMigrationFacet.redeemDepositsAndInternalBalances` sets values instead of increment

T1MOH

Summary

L2ContractMigrationFacet is used to migrate Silo deposits from smart contracts. Owner of that smart contract calls this contract to migrate.

Problem is that during migration it sets all the important values via = instead of +=.

Vulnerability Details

Here you can see that it overrides values of several important variables:

  • s.accts[account].deposits[depositId].amount

  • s.accts[account].deposits[depositId].bdv

  • s.accts[account].mowStatuses[depositData.token].bdv

  • s.sys.silo.balances[depositData.token].deposited

  • s.sys.silo.balances[depositData.token].depositedBdv

Impact

There are several impact depending on certain variable:

  • Overriden s.sys.deposits makes account to lose all his benefits from previous deposits. This single impact is already can be High.

  • Overriden s.accts.mowStatuses.bdv makes user to lose future rewards in Silo because BDV of his deposits is lower than actual.

  • Overriden s.sys.silo.balances breaks Gauge point system calculations, because they are heavily dependant on Silo balances.

Tools Used

Manual Review

Recommendations

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].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;
+ 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 about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

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

Appeal created

T1MOH Submitter
12 months ago
T1MOH Submitter
12 months ago
inallhonesty Lead Judge
12 months ago
inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

`addMigratedDepositsToAccount` Function doesn't properly aggregate the totalDeposited and totalDepositBdved

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

Support

FAQs

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