search

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

0 stars 0 forks source link

prepareWithdrawAll should not be external #148

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#L168-177

Vulnerability details

Impact

Despite commented as Internal Core Implementations, prepareWithdrawAll() is external. There also lack guarantee that prepareWithdrawAll is called before _withdrawAll.

Proof of Concept

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

    /// ===== Internal Core Implementations =====
    function prepareWithdrawAll() external {
        manualProcessExpiredLocks();
    }

    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
    }

Recommended Mitigation Steps

Make it internal and place prepareWithdrawAll in _withdrawAll

GalloDaSballo commented 2 years ago

Disagree with what is an opinion more so than a vulnerability, the warden didn't break the require in withdrawAll nor break the code in any way