M-02 MitigationConfirmed

[c4-bot-8]

Lines of code

Vulnerability details

The fix applied by the team fully mitigates M-02.

[alcueca]

The mitigation review should include more than just links to the issue and the fix. Not much is needed, but at least a description of both.

[c4-judge]

alcueca marked the issue as unsatisfactory:
Insufficient quality

[s1n1st3r01]

Original vulnerability

---

The WithdrawQueue contract inherits PausableUpgrade to provide the ability for an admin to pause users' withdrawals and claims. It also provides access to the internal functions _pause() and _unpaise() through permissioned external functions (pause() and unpause()).

// @POC: WithdrawQueue inherits PausableUpgradeable
contract WithdrawQueue is
    Initializable,
    PausableUpgradeable,
    ReentrancyGuardUpgradeable,
    WithdrawQueueStorageV1
{
    // ...

    function pause() external onlyWithdrawQueueAdmin {
        _pause();
    }

    function unpause() external onlyWithdrawQueueAdmin {
        _unpause();
    }

    // ...
}

The issue is that the user-accessible claim() and withdraw() functions in the same contract do not implement the whenNotPaused modifier, resulting in these functions being unpausable by administrators when they want to do so.

contract WithdrawQueue is
    Initializable,
    PausableUpgradeable,
    ReentrancyGuardUpgradeable,
    WithdrawQueueStorageV1
{
    // ...

    function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
        // ...
    }

    function claim(uint256 withdrawRequestIndex) external nonReentrant {
        // ...
    }
}

Mitigation analysis

---

The mitigation successfully fixes this issue by introducing the whenNotPaused modifier to both functions: WithdrawQueue::claim() and WithdrawQueue::withdraw().

-    function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
+    function withdraw(uint256 _amount, address _assetOut) external nonReentrant whenNotPaused {

-    function claim(uint256 withdrawRequestIndex) external nonReentrant {
+    function claim(uint256 withdrawRequestIndex) external nonReentrant whenNotPaused {

This mitigation ensures that mean admins decide to pause these functionalities, they'll be able to do so by calling the permissioned function WithdrawQueue::pause().

[c4-judge]

alcueca marked the issue as satisfactory