code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/FeeBurner.sol#L56-L65

Vulnerability details

Impact

Attacker can drain all ETH from FeeBurner.sol. (Technically msg.value gets sent to swapperRouter_, but since this contract is out of scope FeeBurner.sol will be treated as the victim)

Proof of Concept

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;
    } 
...

Attacker can pass an address[] memory tokens_ array containing multiple address(0). The same msg.value will be reused for every iteration, leading to the contract having to use its own balance to fulfil the swapAll transactions.

Example:

Attacker calls burnToTarget() with 1 ether and inputs a token array [address(0),address(0),address(0)].
In the first iteration of the for loop (i = 0) swapperRouter_.swapAll{value: msg.value}(address(token_), _WETH); will swap 1 ether for WETH.
In the second iteration of the for loop (i = 1) swapperRouter_.swapAll() will again use the same msg.value and swap another 1 ether for WETH.
The same thing happens for the third and last iteration (i = 2).
Now even though the attacker only deposited 1 ether, he has 3 WETH that will be converted into the equivalent in targetLpToken_.

Attacker can make the token input array as big as necessary to drain all ether from contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a separate function for processing ether (or any other native token) and make sure the msg.value is not reused under a for loop.

chase-manning commented 2 years ago

The FeeBurner doesn't hold any ETH as part of normal use. If someone randomly transfers ETH to the contract, it's fine if some random gets it.

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