code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/RewardHandler.sol#L40-L50

Vulnerability details

Impact

If more than one pool has underlying = address(0) then RewardHandler.burnFees() will fail or use ETH balance from FeeBurner.sol.

Proof of Concept

RewardHandler.sol#L40-L50

uint256 ethBalance = address(this).balance;
        address[] memory tokens = new address[](pools.length);
        for (uint256 i; i < pools.length; i = i.uncheckedInc()) {
            ILiquidityPool pool = ILiquidityPool(pools[i]);
            address underlying = pool.getUnderlying();
            if (underlying != address(0)) {
                _approve(underlying, address(feeBurner));
            }
            tokens[i] = underlying;
        } 
        feeBurner.burnToTarget{value: ethBalance}(tokens, targetLpToken);

FeeBurner.sol#L56-L65

for (uint256 i; i < tokens_.length; i = i.uncheckedInc()) {
            IERC20 token_ = IERC20(tokens_[i]);

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

RewardHandler.burnFees() calls feeBurner.burnToTarget() with its entire ethBalance.
If two pools have underlying = address(0) then feeBurner.burnToTarget() will try to swap the entire ethBalance from RewardHandler twice.
The call will either revert or use FeeBurners own balance.

Tools Used

Manual Review

Recommended Mitigation Steps

Don't loop over using the same msg.value when dealing with multiple pools using underlying = address(0). Instead make the swap based on an individual per token basis.

chase-manning commented 2 years ago

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.

GalloDaSballo commented 2 years ago

The second call will fail as there will be no ETH to send to the SwapperRouter, for that reason I believe the finding to be invalid

code-423n4 / 2022-05-backd-findings