Description
The SiloFacet::transferDeposit function allows any user to transfer their deposit to another user. However, the function does not check the amount being transfered. Worse still, if this amount is 0, it repeatedly pushes the same depositId into s.accts[account].depositIdList[token]:
function addDepositToAccount(
...
) internal {
AppStorage storage s = LibAppStorage.diamondStorage();
uint256 depositId = LibBytes.packAddressAndStem(token, stem);
@> if (s.accts[account].deposits[depositId].amount == 0) {
@> s.accts[account].depositIdList[token].push(depositId);
}
...
}
A user can call SiloFacet::transferDeposit with an amount of 0 and any recipient multiple times to inflate the recipient's array length, causing a DoS for all functions that use s.accts[account].depositIdList[token] in memory (due to excessive gas costs for memory) or in a loop (due to excessive gas costs for executing instructions).
The real issue is that this variable is used in critical parts of the code (tree representing where functions are called):
- SiloGettersFacet::getTokenDepositIdsForAccount - Full DoS
- SiloGettersFacet::getTokenDepositsForAccount - Full DoS
- SiloGettersFacet::getDepositsForAccount - Full DoS
- LibTokenSilo::findDepositIdForAccount - Full DoS
- LibTokenSilo::removeDepositIDfromAccountList - Full DoS
- LibTokenSilo::removeDepositFromAccount - DoS if all assets of a token is removed/withdraw/transfer
- LibConvert::_withdrawTokens - DoS if all assets of a token is removed/withdraw/transfer
- PipelineConvertFacet::pipelineConvert - DoS if all assets of a token is removed/withdraw/transfer
- ConvertFacet::convert - DoS if all assets of a token is removed/withdraw/transfer
- LibSilo::_removeDepositFromAccount - DoS if all assets of a token is removed/withdraw/transfer
- TokenSilo::_withdrawDeposit - DoS if all assets of a token is removed/withdraw/transfer
- SiloFacet::withdrawDeposit - DoS if all assets of a token is removed/withdraw/transfer
- TokenSilo::_transferDeposit - DoS if all assets of a token is removed/withdraw/transfer
- SiloFacet::transferDeposit - DoS if all assets of a token is removed/withdraw/transfer
- TokenSilo::_transferDeposits - DoS if all assets of a token is removed/withdraw/transfer
- SiloFacet::transferDeposits - DoS if all assets of a token is removed/withdraw/transfer
- LibSilo::_removeDepositsFromAccount - DoS if all assets of a token is removed/withdraw/transfer
- TokenSilo::_withdrawDeposits - DoS if all assets of a token is removed/withdraw/transfer
- SiloFacet::withdrawDeposits - DoS if all assets of a token is removed/withdraw/transfer
- EnrootFacet::enrootDeposit - DoS if all assets of a token is removed/withdraw/transfer
This bug only causes a full withdrawal/transfer/removal DoS because LibTokenSilo::removeDepositIDfromAccountList is called in LibTokenSilo::removeDepositFromAccount with this condition:
if (crateAmount > 0) {
delete s.accts[account].deposits[depositId];
removeDepositIDfromAccountList(account, token, depositId);
}
Risk
Likelyhood: High
Before the migration, executing this attack will be costly, but once the migration is completed, it can be executed on anyone at a very low cost.
Impact: Medium
Recommended Mitigation
Before pushing any new ID into the depositIdList array, check if the amount is 0.
function addDepositToAccount(
...
) internal {
AppStorage storage s = LibAppStorage.diamondStorage();
uint256 depositId = LibBytes.packAddressAndStem(token, stem);
// add a depositId to an account's depositList, if there is not an existing deposit.
- if (s.accts[account].deposits[depositId].amount == 0) {
+ if (s.accts[account].deposits[depositId].amount == 0 && amount > 0) {
s.accts[account].depositIdList[token].push(depositId);
}
...
}
