Inconsistency in paused functionalities

[code423n4]

Lines of code

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

Vulnerability details

Impact

Pausing the contract should stop all permissionless functionalities. But the function performUpkeep lacks whenNotPaused, so keepers will still be able to call it and process expired locks even if the contract is paused, leading to a loss of fund as keeper would still run, and preventing the contract from being fully paused as intended.

Proof of Concept

From the function manualProcessExpiredLocks, we see that calling processExpiredLocks should be forbidden when the contract is paused. But the exact same function does not contains the whenNotPaused modifier: https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L391

• this will lead to a loss of funds for the DAO as they'll pay for the ChainLink keeper cost
• this prevents the DAO from pausing this functionality although it seems clear that it's intended to be pausable

Recommended Mitigation Steps

Add the whenNotPaused modifier or reuse manualProcessExpiredLocks in performUpkeep

[GalloDaSballo]

Disagree with severity in lack of a POC, allowing to unlock is a good thing, if anything we should remove the extra checks