Medium4-UpkeepInWithdrawalPoolIsBroken.md

Vulnerability Details

The performUpkeep() function in the WithdrawalPool, when called by the upkeep, it will always revert.

This is because in checkUpkeep() the following return values is returned whenever upkeep is needed: return (true, "");. See here.

The second argument is the performUpkeep() calldata, which if empty it will be empty.

And this is the performUpkeep() logic used:

@>    //                          👁️🔴⏬ this is empty bytes
    function performUpkeep(bytes calldata _performData) external {
        uint256 canWithdraw = priorityPool.canWithdraw(address(this), 0);
        uint256 totalQueued = _getStakeByShares(totalQueuedShareWithdrawals);
        if (
            totalQueued == 0 ||
            canWithdraw == 0 ||
            block.timestamp <= timeOfLastWithdrawal + minTimeBetweenWithdrawals
        ) revert NoUpkeepNeeded();

        timeOfLastWithdrawal = uint64(block.timestamp);

        uint256 toWithdraw = totalQueued > canWithdraw ? canWithdraw : totalQueued;
        
@>      //                          👁️🔴⏬ It has not been altered along the function, this is still empty bytes, thus reverts
        bytes[] memory data = abi.decode(_performData, (bytes[]));

        priorityPool.executeQueuedWithdrawals(toWithdraw, data);
        _finalizeWithdrawals(toWithdraw);
    }

Proof Of Concept

Copy-paste this code in remix and see decoding of empty bytes reverts:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;

contract Caller {
  function simulateEmptyCall(address toCall) external {
        bytes memory empty = "";
        SimpleDecoder(toCall).performUpkeep(empty);
    }

}

contract SimpleDecoder {
    // Function to decode the provided bytes calldata
    function performUpkeep(bytes calldata _performData) public returns (bytes[] memory) {
        // Decode the input data as an array of bytes
        bytes[] memory data = abi.decode(_performData, (bytes[]));
        return data;
    }
}

Impact

The upkeep in WithdrawalPool will not work. Even in reverts Chainlink automation charges your subscription a fee, so this will drain the subscription slowly. You can read more about Chainlink fees in automation here.

Recommendations

Return the proper calldata in checkUpkeep() so performUpkeep() can work properly. This calldata are the vault IDs that will be withdrawn from in case of needing to withdraw.