_updatePendingWithdrawalWithQueue() should accumulate pendingForQueue

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/pools/Pool.sol#L678

Vulnerability details

Vulnerability details

_queueClaimAll() is used to allocate getTotalReceived[]

The main calculation is in the _updatePendingWithdrawalWithQueue() method

This method has an important mechanism to calculate All future queues/pools also have a piece of it.

    function _queueClaimAll(uint256 _totalToBeWithdrawn, uint256 _cachedPendingQueueIndex) private {
        _reallocateOnWithdrawal(_totalToBeWithdrawn);
        uint256 totalQueues = (getMaxTotalWithdrawalQueues + 1);
        uint256 oldestQueueIdx = (_cachedPendingQueueIndex + 1) % totalQueues;
        uint256[] memory pendingWithdrawal = new uint256[](totalQueues);
        for (uint256 i; i < pendingWithdrawal.length;) {
            uint256 idx = (oldestQueueIdx + i) % totalQueues;
@>          _updatePendingWithdrawalWithQueue(idx, _cachedPendingQueueIndex, pendingWithdrawal);
            unchecked {
                ++i;
            }
        }
        getAvailableToWithdraw = 0;

        for (uint256 i; i < pendingWithdrawal.length;) {
            if (pendingWithdrawal[i] == 0) {
                unchecked {
                    ++i;
                }
                continue;
            }
            address queueAddr = _deployedQueues[i].contractAddress;
            uint256 amount = pendingWithdrawal[i];

            asset.safeTransfer(queueAddr, amount);
            emit QueueClaimed(queueAddr, amount);
            unchecked {
                ++i;
            }
        }
    }
....

    function _updatePendingWithdrawalWithQueue(
        uint256 _idx,
        uint256 _cachedPendingQueueIndex,
        uint256[] memory _pendingWithdrawal
    ) private returns (uint256[] memory) {
        uint256 totalReceived = getTotalReceived[_idx];
        uint256 totalQueues = getMaxTotalWithdrawalQueues + 1;
        /// @dev Nothing to be returned
        if (totalReceived == 0) {
            return _pendingWithdrawal;
        }
        getTotalReceived[_idx] = 0;

        /// @dev We go from idx to newer queues. Each getTotalReceived is the total
        /// returned from loans for that queue. All future queues/pool also have a piece of it.
        /// X_i: Total received for queue `i`
        /// X_1  = Received * shares_1 / totalShares_1
        /// X_2 = (Received - (X_1)) * shares_2 / totalShares_2 ...
        /// Remainder goes to the pool.
        for (uint256 i; i < totalQueues;) {
            uint256 secondIdx = (_idx + i) % totalQueues;
            QueueAccounting memory queueAccounting = _queueAccounting[secondIdx];
            if (queueAccounting.thisQueueFraction == 0) {
                unchecked {
                    ++i;
                }
                continue;
            }
            /// @dev We looped around.
            if (secondIdx == _cachedPendingQueueIndex + 1) {
                break;
            }
            uint256 pendingForQueue = totalReceived.mulDivDown(queueAccounting.thisQueueFraction, PRINCIPAL_PRECISION);
            totalReceived -= pendingForQueue;

@>          _pendingWithdrawal[secondIdx] = pendingForQueue;
            unchecked {
                ++i;
            }
        }
        return _pendingWithdrawal;
    }

Currently _updatePendingWithdrawalWithQueue() uses _pendingWithdrawal[secondIdx] = pendingForQueue; The correct one would be to accumulate: _pendingWithdrawal[secondIdx] += pendingForQueue;

because it can be accumulated multiple times when loop calculating all queues for All future queues/pools also have a piece of it

Impact.

Without using accumulation, the actual amount allocated is partially lost

Recommended Mitigation

    function _updatePendingWithdrawalWithQueue(
        uint256 _idx,
        uint256 _cachedPendingQueueIndex,
        uint256[] memory _pendingWithdrawal
    ) private returns (uint256[] memory) {

            /// @dev We looped around.
            if (secondIdx == _cachedPendingQueueIndex + 1) {
                break;
            }
            uint256 pendingForQueue = totalReceived.mulDivDown(queueAccounting.thisQueueFraction, PRINCIPAL_PRECISION);
            totalReceived -= pendingForQueue;

-           _pendingWithdrawal[secondIdx] = pendingForQueue;
+           _pendingWithdrawal[secondIdx] += pendingForQueue;
            unchecked {
                ++i;
            }
        }
        return _pendingWithdrawal;
    }

Assessed type

Context

[c4-judge]

0xA5DF marked the issue as duplicate of #46

[c4-judge]

0xA5DF marked the issue as satisfactory