In WithdrawalQueue, the user may not be able to withdraw token

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

In some cases, users are unable to withdraw token, resulting in the loss of user funds.

Proof of Concept

WithdrawalQueue uses _getAvailable to calculate the token that a certain _tokenId can withdraw.

    function _getAvailable(uint256 _tokenId) private view returns (uint256) {
        return getShares[_tokenId] * _getWithdrawablePerShare() - getWithdrawn[_tokenId];
    }

    function _getWithdrawablePerShare() private view returns (uint256) {
        return (_totalWithdrawn + _asset.balanceOf(address(this))) / getTotalShares;
    }

This function will get a negative number in some cases, but since it is uint256, it will revert.

_getWithdrawablePerShare gets a ratio based on balanceOf(this) and getTotalShares.

withdrawablePerShare is a variable value, Suppose the value is 2 when the user first withdraw, and becomes 1 when the user again withdraw. Since getWithdrawn is also calculated based on userShare * withdrawablePerShare, getWithdrawn[_tokenId] is the number of tokens that have been withdraw.

    uint256 available = _getAvailable(_tokenId);
    getWithdrawn[_tokenId] += available;

For the first withdraw, getWithdrawn[_tokenId] may be a larger value, When withdraw again, withdrawablePerShare is reduced, so userShare * withdrawablePerShare is likely to be smaller than getWithdrawn[_tokenId].

It will be revert, as a result, users can't get rewards.

Tools Used

vscode, manual

Recommended Mitigation Steps

Record the difference in share when two withdrawals are made, Instead of recording the number of tokens that have been extracted

Assessed type

Other

[c4-judge]

0xA5DF marked the issue as primary issue

[0xend]

I don't think this is a problem. Withdrawable per share only increases (it's the total amount received over time which is the current balance + all withdrawn).

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Invalid

[zhaojio]

_getWithdrawablePerShare doesn't increase all the time, getTotalShares is the denominator, if getTotalShares increases and the numerator stays the same,_getWithdrawablePerShare decreases,

    function mint(address _to, uint256 _shares) external returns (uint256) {
        if (msg.sender != getPool) {
            revert PoolOnlyCallableError();
        }
        _mint(_to, getNextTokenId);
        getShares[getNextTokenId] = _shares;
@>      getTotalShares += _shares;

        emit WithdrawalPositionMinted(getNextTokenId, _to, _shares);
        return getNextTokenId++;
    }

    // Pool.withdraw -> _withdraw
    function _withdraw(address owner, address receiver, uint256 assets, uint256 shares) private {
        beforeWithdraw(assets, shares);

        _burn(owner, shares);

        emit Withdraw(msg.sender, receiver, owner, assets, shares); 
@>      WithdrawalQueue(_deployedQueues[_pendingQueueIndex].contractAddress).mint(receiver, shares);
    }

The withdraw function in Pool simply calls WithdrawalQueue.mint without adding _asset.balanceOf(Queue).

Therefore, when users calls Pool.withdraw it will cause _getWithdrawablePerShare to decrease.

[0xA5DF]

The withdraw function in Pool simply calls WithdrawalQueue.mint without adding _asset.balanceOf(Queue).

In what scenario does the pool do that?

Also, the impact is that under that scenario the user wouldn't be able to withdraw due to underflow. But given that the balance of the user is negative they have nothing to withdraw in the first place. It's true that an underflow is not the right way to handle such a case, but if this is the only impact then it's not more than a low severity issue.