search

code-423n4 / 2022-06-badger-findings

0 stars 0 forks source link

All withdrawal functionality is paused when contract is paused #24

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L182-L190

Vulnerability details

Impact

When the strategy contract is paused, all withdrawal functionality will be paused. Based on the comments in MyStrategy.sol and baseStrategy.sol, withdrawToVault() should not be affected by the pause functionality. This is not the case due to the whenNotpaused modifier all the way down the execution chain in manualProcessExpiredLocks().

Proof of Concept

If the strategy contract is paused, the following withdraw calls will be paused as well:

BaseStrategy.withdrawToVault() calls MyStrategy._withdrawAll() which doesn't do anything other than checking balances. Per the comments of _withdrawAll(), "Make sure to call prepareWithdrawAll before _withdrawAll".

prepareWithdrawAll() calls manualProcessExpiredLocks(); which has the whenNotPaused modifier.

Therefore, if a vulnerability is discovered or is currently being exploited and Badger must decide to pause the contract, they will not be able to withdraw from any Aura locks. If this were not known previously, Badger would be required to unpause, process locks, and then withdraw. An exploiter could easily front run this, bypassing the protection granted by the pausing functionality.

Tools Used

Manual review.

Recommended Mitigation Steps

Either remove the whenNotPaused modifier from manualProcessExpiredLocks() or call LOCKER.processExpiredLocks(false); within _withdrawAll(), similar to how it is done in _withdrawSome().

GalloDaSballo commented 2 years ago

Completely disagree with the finding as that's the point of the Pause, additionally the Vault has a "pausedWithdrawals" mode

Also see Quantstamp Audit

Screenshot 2022-06-17 at 21 57 51
fatherGoose1 commented 2 years ago

I take your word for it, but this comment in the top-level BaseStrategy.sol made me think that the _withdrawAll() function should not be susceptible to the pause.

    /// @dev This can be called even when paused, and strategist can trigger this via the vault.
    function withdrawToVault() external {
        _onlyVault();

        _withdrawAll();

        uint256 balance = IERC20Upgradeable(want).balanceOf(address(this));
        _transferToVault(balance);
    }