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

`L2ContractMigrationFacet.addMigratedDepositsToAccount()` doesn't push depositId to `depositIdList`

T1MOH

Summary

depositIdList is array of Silo deposits owned by account. Ids are pushed on new deposits and deleted on withdrawals. depositIdList is used to track deposits of user off-chain.

Problem is that L2ContractMigrationFacet doesn't update this array when migrates deposits. You can check it here:
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/df2dd129a878d16d4adc75049179ac0029d9a96b/protocol/contracts/beanstalk/silo/L2ContractMigrationFacet.sol#L168-L170

Issue arises at the moment when such migrated deposit is removed from account (during transfer or withdrawal). It tries to remove such depositId from depositIdList, as a result full removal will revert:
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/df2dd129a878d16d4adc75049179ac0029d9a96b/protocol/contracts/libraries/Silo/LibTokenSilo.sol#L383-L387

function removeDepositFromAccount(
address account,
address token,
int96 stem,
uint256 amount
) internal returns (uint256 crateBDV) {
AppStorage storage s = LibAppStorage.diamondStorage();
uint256 depositId = LibBytes.packAddressAndStem(token, stem);
​
uint256 crateAmount = s.accts[account].deposits[depositId].amount;
crateBDV = s.accts[account].deposits[depositId].bdv;
require(amount <= crateAmount, "Silo: Crate balance too low.");
​
// Partial remove
if (amount < crateAmount) {
...
}
// Full remove
if (crateAmount > 0) {
delete s.accts[account].deposits[depositId];
@> removeDepositIDfromAccountList(account, token, depositId);
}
​
...
}
function removeDepositIDfromAccountList(
address account,
address token,
uint256 depositId
) internal {
AppStorage storage s = LibAppStorage.diamondStorage();
@> uint256 i = findDepositIdForAccount(account, token, depositId);
...
}
​
function findDepositIdForAccount(
address account,
address token,
uint256 depositId
) internal view returns (uint256 i) {
AppStorage storage s = LibAppStorage.diamondStorage();
uint256[] memory depositIdList = s.accts[account].depositIdList[token];
uint256 length = depositIdList.length;
while (depositIdList[i] != depositId) {
i++;
if (i >= length) {
@> revert("Id not found");
}
}
return i;
}

Vulnerability Details

Impact

Accounts which migrated via L2ContractMigrationFacet.sol can't perform full withdrawal in Silo. Worth noting is that L2ContractMigrationFacet manages migration of deposits owned by smart contracts.

So it is highly likely that smart contracts don't have functionality to perform partial withdraw to avoid issue. In case it lacks such functionality, its deposits can't be withdrawn and therefore frozen forever.

Tools Used

Manual Review

Recommendations

function addMigratedDepositsToAccount(
address account,
AccountDepositData calldata depositData
) internal returns (uint256 accountStalk) {
...
for (uint256 i; i < depositData.depositIds.length; i++) {
...
​
// add deposit to account.
+ if (s.accts[account].deposits[depositId].amount == 0) {
+ s.accts[account].depositIdList[token].push(depositId);
+ }
s.accts[account].deposits[depositId].amount = depositData.amounts[i];
s.accts[account].deposits[depositId].bdv = depositData.bdvs[i];
​
...
}
​
...
}
Updates

Lead Judging Commences

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

`removeDepositFromAccount` won't work after L2Migration `redeemDepositsAndInternalBalances`via

Appeal created

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

`removeDepositFromAccount` won't work after L2Migration `redeemDepositsAndInternalBalances`via

Support

FAQs

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