If token == BADGER, the _handleRewardTransfer function does not work

[code423n4]

Lines of code

MyStrategy.sol#L405-L413
BaseStrategy.sol#L346-L353
Vault.sol#L396-L415

Vulnerability details

Impact

In MyStrategy.sol, when claiming bribes or sweeping reward tokens, the _handleRewardTransfer function is called which calls _sendToBadgerTree to send the amount of BADGER in the contract to the BADGER_TREE. However, the _processExtraToken (in BaseStrategy.sol) called after the transfer to BADGER_TREE will attempt to transfer the same amount of BADGER to the vault.

This leads to the sweepRewardToken, sweepRewards and claimBribesFromHiddenHand functions not working properly if BADGER is claimed.

Proof of Concept

The tests do not consider when the contract contains BADGER tokens, so the BADGER token should be added in the conftest.py test file.

Test code added to conftest.py:

@pytest.fixture
def badger(deployer):
    TOKEN_ADDRESS = BADGER
    token = interface.IERC20Detailed(TOKEN_ADDRESS)
    BADGER_WHALE = accounts.at(BADGER_WHALE_ADDRESS, force=True)  ## Address with tons of token

    token.transfer(deployer, token.balanceOf(BADGER_WHALE), {"from": BADGER_WHALE})
    return token

BADGER tokens should then be sent to the strategy contract and an attempt to sweep the BADGER should fail.

Test code added to test_custom.py:

def test_sweep_BADGER(badger, strategy, strategist, deployer):
    badger.transfer(strategy, badger.balanceOf(deployer), {"from": deployer})
    # test the strategy has BADGER tokens
    assert badger.balanceOf(strategy) > 0
    # test that sweep reverts
    with brownie.reverts():
        strategy.sweepRewards([badger], {"from": strategist})
    # sanity check the sweep was unsuccessful
    assert badger.balanceOf(strategy) > 0

Recommended Mitigation Steps

The purpose of the _processExtraToken function in BaseStrategy.sol seems to be to forward the extra token (BADGER) to the BADGER_TREE and to process fees. This should mean that the transfer to BADGER_TREE in _sendBadgerToTree is not necessary and can be removed.

[GalloDaSballo]

This is a basic developer oversight, I agree with the finding

[GalloDaSballo]

Maybe Dup of M-02 @jack-the-pug