Lines of code

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L205

Vulnerability details

Impact

The withdrawal safety check in https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L205 seems unreasonable.

Proof of Concept

I don’t understand why max >= _amount*99.8% need to be confirmed. max should be larger than _amount. And _amount <= max*99.8% seems more reasonable. https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L205

    function _withdrawSome(uint256 _amount) internal override returns (uint256) {
        uint256 max = balanceOfWant();

        if (_amount > max) {
            // Try to unlock, as much as possible
            // @notice Reverts if no locks expired
            LOCKER.processExpiredLocks(false);
            max = balanceOfWant();
        }

        if (withdrawalSafetyCheck) {
            require(max >= _amount.mul(9_980).div(MAX_BPS), "Withdrawal Safety Check"); // 20 BP of slippage
        }

        if (_amount > max) {
            return max;
        }

        return _amount;
    }

Tools Used

None

Recommended Mitigation Steps

The withdrawal safety check should be

            require(_amount <= max.mul(9_980).div(MAX_BPS), "Withdrawal Safety Check"); // 20 BP of slippage

The check is necessary as the value of shares is calculated based on Vault.balance() because a part of Vault.balance() is Strategy.balanceOfPool() then the amount want that the strategy has liquid and available to send back (max) could be less than the amount that the caller is requesting.

Because this is a locking strategy we cannot at all times honor attempts to withdraw, for that reason, instead of unjustly burning withdrawer shares, we will run a check to make sure that they will not get rekt for more than 20 BPS

Removing the check will allow a withdrawer to redeem all shares at any time, but they will get only the want available which most of the times means they will lose all their tokens

For this explanation I must completely disagree with the finding

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