Lack of approval on withdrawal requests can be abused

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/UserWithdrawalManager.sol#L101-L103

Vulnerability details

Impact

Any user can submit withdrawal requests on behalf of other users, but there isn't an approval system. This can be abused to DoS specific users, as there are multiple checks on how many request a user can make.

Proof of Concept

Any user can submit a withdrawal request on behalf of other users, this is the function used to make new requests:

	function requestWithdraw(uint256 _ethXAmount, address _owner) external override whenNotPaused returns (uint256) {
	    if (_owner == address(0)) revert ZeroAddressReceived();
	    uint256 assets = IStaderStakePoolManager(staderConfig.getStakePoolManager()).previewWithdraw(_ethXAmount);
	    if (assets < staderConfig.getMinWithdrawAmount() || assets > staderConfig.getMaxWithdrawAmount()) {
	        revert InvalidWithdrawAmount();
	    }
->	    if (requestIdsByUserAddress[_owner].length + 1 > maxNonRedeemedUserRequestCount) {
	        revert MaxLimitOnWithdrawRequestCountReached();
	    }
	    IERC20Upgradeable(staderConfig.getETHxToken()).safeTransferFrom(msg.sender, (address(this)), _ethXAmount);
	    ethRequestedForWithdraw += assets;
	    userWithdrawRequests[nextRequestId] = UserWithdrawInfo(payable(_owner), _ethXAmount, assets, 0, block.number);
->	    requestIdsByUserAddress[_owner].push(nextRequestId);
	    emit WithdrawRequestReceived(msg.sender, _owner, nextRequestId, _ethXAmount, assets);
	    nextRequestId++;
	    return nextRequestId - 1;
	}

There is a hard limit on how many requests a user can make:

	if (requestIdsByUserAddress[_owner].length + 1 > maxNonRedeemedUserRequestCount) {
	    revert MaxLimitOnWithdrawRequestCountReached();
	}

Bob may decide to "donate" their funds by making maxNonRedeemedUserRequestCount small withdrawal requests to Alice: as a consequence, Alice can't make new withdrawal requests to herself, as her transactions would exceed the max.

Alice is effectively under DoS until Bob decides to stop making new requests, as Alice's requests can be easily front-run.

Tools Used

Manual review

Recommended Mitigation Steps

Consider modifying the function so that requests are under the requester address:

	function requestWithdraw(uint256 _ethXAmount, address _owner) external override whenNotPaused returns (uint256) {
	    if (_owner == address(0)) revert ZeroAddressReceived();
	    uint256 assets = IStaderStakePoolManager(staderConfig.getStakePoolManager()).previewWithdraw(_ethXAmount);
	    if (assets < staderConfig.getMinWithdrawAmount() || assets > staderConfig.getMaxWithdrawAmount()) {
	        revert InvalidWithdrawAmount();
	    }
+	    if (requestIdsByUserAddress[msg.sender].length + 1 > maxNonRedeemedUserRequestCount) {
-	    if (requestIdsByUserAddress[_owner].length + 1 > maxNonRedeemedUserRequestCount) {
	        revert MaxLimitOnWithdrawRequestCountReached();
	    }
	    IERC20Upgradeable(staderConfig.getETHxToken()).safeTransferFrom(msg.sender, (address(this)), _ethXAmount);
	    ethRequestedForWithdraw += assets;
	    userWithdrawRequests[nextRequestId] = UserWithdrawInfo(payable(_owner), _ethXAmount, assets, 0, block.number);
+	    requestIdsByUserAddress[msg.sender].push(nextRequestId);
-	    requestIdsByUserAddress[_owner].push(nextRequestId);
	    emit WithdrawRequestReceived(msg.sender, _owner, nextRequestId, _ethXAmount, assets);
	    nextRequestId++;
	    return nextRequestId - 1;
	}

Assessed type

Access Control

[c4-judge]

Picodes marked the issue as duplicate of #221

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as unsatisfactory:
Invalid