More than 1 zero address token in ```burnToTarget``` may lead to draining of ```FeeBurner.sol```

[code423n4]

Lines of code

FeeBurner.sol#L43-L88

Vulnerability details

Impact

The burnToTarget function in FeeBurner.sol allows any array of tokens_ to be used. If the tokens_ array contains more than 1 zero address, the swapAll function for the swapperRouter_ will be called more than once with the same msg.value. After the first swapAll call, the FeeBurn.sol contract will start to lose ETH.

Proof of Concept

1. Let's say the FeeBurner.sol holds 5 ETH and an attacker calls burnToTarget with a tokens_ array of 2 zero addresses and a msg.value of 5 ETH, increasing the contract's total balance to 10 ETH.

2. Assuming targetLpToken is valid, the first if block in the for loop will be triggered since token_ == address(0). msg.value is non-zero so the swapAll function is called with msg.value to swap the ETH for WETH. The contract now has approximately 5 ETH again and 5 WETH.

            if (address(token_) == address(0)) {
                if (msg.value == 0) continue;
                burningEth_ = true;
                swapperRouter_.swapAll{value: msg.value}(address(token_), _WETH);
                continue;
            }

3. After the first swap, the loop continues and the first if block is triggered again. Since msg.value is still 5 ether, swapAll is called. The contract will use its remaining 5 ETH and receive 5 WETH. The contract now has a balance of approximately 0 ETH and 10 WETH.

4. The for loop is now finished and all the WETH in the contract is then swapped for the underlying token of the target pool.

        _approve(_WETH, address(swapperRouter_));
        swapperRouter_.swapAll(_WETH, targetUnderlying_);

5. The underlying token amount is deposited into the target pool and the contract should receive a proportional amount of the target pool's LP tokens.

        // Depositing target underlying into target pool
        uint256 targetLpTokenBalance_ = _depositInPool(targetUnderlying_, targetPool_);

        // Transfering LP tokens back to sender
        IERC20(targetLpToken_).safeTransfer(msg.sender, targetLpTokenBalance_);

6. The LP tokens are then sent to the attacker and FeeBurner.sol is left with approximately 0 ETH and 0 WETH.

Recommended Mitigation Steps

The if block should not trigger if burningETH_ is true. Consider changing the if condition to address(token_) == address(0) && !burningETH_ so that ETH is not swapped more than once.

[chase-manning]

If someone sends an invalid request to the fee burner (such as two ETH pools), then it is expected that the transaction should fail. That is the intended behaviour.

The FeeBurner holds no ETH as part of normal operation. There is nothing to drain.

[GalloDaSballo]

Have to agree with the sponsor, if the function is called with the goal of burning ETH twice, the first time the burn will burn all available balance, and then the second time a revert will happen as no ETH is there to be sent to the SwapperRouter.

The contract has no tokens nor balance as such the POC is invalidated