code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

Users that accidentally send Ether to contracts, or have rebasing rewards that the contract has stole (because it doesn't properly track rebasing tokens), have their funds (now latent) stolen, so they can't be returned by an admin.

Proof of Concept

One example is in burnToTarget() where the function doesn't verify that tokens_ doesn't have any duplicates. If the caller specifies address(0) (indicating Ether) twice, and provides a msg.value exactly equal to the latent balance of the contract, the first swapAll() will work because of the funds actually transferred, and the second swapAll() will also work because msg.value has the same value in a single call, no matter how many times it's checked or things change, and the funds for the actual transfer are available latently.

File: protocol/contracts/tokenomics/FeeBurner.sol   #1

43       function burnToTarget(address[] memory tokens_, address targetLpToken_)
44           public
45           payable
46           override
47           returns (uint256 received)
48       {
49           require(tokens_.length != 0, "No tokens to burn");
50   
51           // Swapping tokens for WETH
52           ILiquidityPool targetPool_ = _addressProvider.getPoolForToken(targetLpToken_);
53           address targetUnderlying_ = targetPool_.getUnderlying();
54           ISwapperRouter swapperRouter_ = _swapperRouter();
55           bool burningEth_;
56           for (uint256 i; i < tokens_.length; i = i.uncheckedInc()) {
57               IERC20 token_ = IERC20(tokens_[i]);
58   
59               // Handling ETH
60               if (address(token_) == address(0)) {
61                   if (msg.value == 0) continue;
62                   burningEth_ = true;
63                   swapperRouter_.swapAll{value: msg.value}(address(token_), _WETH);
64                   continue;
65               }

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

Tools Used

Code inspection

Recommended Mitigation Steps

Don't allow duplicate entries when allowing the user to provide arrays and track balance changes after each transfer

chase-manning commented 2 years ago

We don't support rebasing tokens. The FeeBurner holds no ETH as part of normal operation. If a user randomly sends ETH to the FeeBurner contract, it's fine if someone else randomly gets it.

GalloDaSballo commented 2 years ago

Disagree that contract allows latent funds to be stolen, this is a router contract, and funds are meant to be multi-called / transferFrom into it.

Any token sent to the contract will be MEVd just like with any other router-like contract

For those reasons I believe the finding to be invalid

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