Lines of code

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

Vulnerability details

Impact

Usually, in emergency situations, contracts will be paused by the owner to prevent further damage.

To withdraw all funds, the MyStrategy.prepareWithdrawAll function has to be manually called right before BaseStrategy.withdrawToVault can be called (see comment).

This will fail as MyStrategy.manualProcessExpiredLocks (called within MyStrategy.prepareWithdrawAll) has the modifier whenNotPaused, hence, due to the contract being paused, will revert.

Additionally, the BaseStrategy.withdrawToVault function call is initiated by vault, but vault can not unpause the contract, only authorized pausers (guardian and governance) are able to pause/unpause.

Unpausing in emergency situations is also not recommended, as this could lead to further damage.

Proof of Concept

MyStrategy._withdrawAll

function _withdrawAll() internal override {
    //NOTE: This probably will always fail unless we have all tokens expired
    require(
        balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0,
        "You have to wait for unlock or have to manually rebalance out of it"
    );

    // Make sure to call prepareWithdrawAll before _withdrawAll
}

MyStrategy.prepareWithdrawAll

function prepareWithdrawAll() external {
    manualProcessExpiredLocks();
}

MyStrategy.manualProcessExpiredLocks

function manualProcessExpiredLocks() public whenNotPaused { // @audit-info `whenNotPaused` prevents calling this function when contract is paused
    // Unlock vlAURA that is expired and redeem AURA back to this strat
    LOCKER.processExpiredLocks(false);
}

Tools Used

Manual review

Recommended mitigation steps

Call LOCKER.processExpiredLocks(false); in MyStrategy._withdrawAll directly.

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