Withdrawing deposits within a batch transaction can lead to issues with deposit ids

Summary

The deposit ids change whenever a deposit is withdrawn. This can cause issues when using deposit ids in batch transactions.

Vulnerability Detail

Deposit ids are based on the index of a deposit within the depositsOf[msg.sender] array. On withdrawal, deposits are reordered - the last deposit from a user will be placed at the position of the withdrawn deposit with id _depositId and the last deposit is removed. Hence the deposit id of the last deposit has changed. This can lead to issues when withdrawing deposits and, for example, extending deposit locks within a batch transaction. Then the change of deposit ids has to be taken into account by the user calling the batch transaction. Otherwise, if the deposit id of the previously last deposit is referenced, the transaction will revert with an out-of-bounds error.

Consider the following example:

Alice has 10 deposits with ids 0, 1, 2, 3, 4, 5, 6, 7, 8, 9.
Alice creates and executes the following batch transaction:
1. Withdraw the deposit with id 6
2. Withdraw the deposit with id 9
The batch transaction will revert due to no deposit exists with id 9 (the previous deposit with id 9 has its deposit id changed to id 6)

Impact

Wrong and non-existent deposits are referenced by deposit ids, leading batch transactions to revert.

Code Snippet

TimeLockPool.sol#L127

function withdraw(uint256 _depositId, address _receiver) external {
    if (_depositId >= depositsOf[_msgSender()].length) {
        revert NonExistingDepositError();
    }
    Deposit memory userDeposit = depositsOf[_msgSender()][_depositId];
    if (block.timestamp < userDeposit.end) {
        revert TooSoonError();
    }

    // remove Deposit
    depositsOf[_msgSender()][_depositId] = depositsOf[_msgSender()][depositsOf[_msgSender()].length - 1]; // @audit-info this will change deposit ids
    depositsOf[_msgSender()].pop();

    // burn pool shares
    _burn(_msgSender(), userDeposit.shareAmount);

    // return tokens
    depositToken.safeTransfer(_receiver, userDeposit.amount);
    emit Withdrawn(_depositId, _receiver, _msgSender(), userDeposit.amount);
}

Tool used

Manual review

Recommendation

Consider using immutable deposit ids instead of referencing the deposit as the index in the array.

sherlock-audit / 2022-10-merit-circle-judging

berndartmueller - Withdrawing deposits within a batch transaction can lead to issues with deposit ids #48