BurnToTarget can be exploited to receive more LP tokens

[code423n4]

Lines of code

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

Vulnerability details

Issue: _depositInPool uses address(this).balance

Consequences: exploiter will get more lp tokens

File: FeeBurner.sol
81:         // Depositing target underlying into target pool
82:         uint256 targetLpTokenBalance_ = _depositInPool(targetUnderlying_, targetPool_);
83: 
84:         // Transfering LP tokens back to sender
85:         IERC20(targetLpToken_).safeTransfer(msg.sender, targetLpTokenBalance_);
86:         emit Burned(targetLpToken_, targetLpTokenBalance_);
87:         return targetLpTokenBalance_;

File: FeeBurner.sol
096:     function _depositInPool(address underlying_, ILiquidityPool pool_)
097:         internal
098:         returns (uint256 received)
099:     {
100:         // Handling ETH deposits
101:         if (underlying_ == address(0)) {
102:             uint256 ethBalance_ = address(this).balance;
103:             return pool_.deposit{value: ethBalance_}(ethBalance_);
104:         }
105: 
106:         // Handling ERC20 deposits
107:         _approve(underlying_, address(pool_));
108:   }

Proof of Concept

• external action deposits ETH to FeeBurner (FeeBurner accepts it as it has receive())
• an exploiter watches the mempool and once he sees the ETH balance changed he calls burnToTarget with a small ETH value to take advantage of the _depositInPool (which uses address(this).balance)
• which will result into receiving more LP tokens than the value he sent with the burnToTarget call

[chase-manning]

No user should randomly transfer ETH the FeeBurner contract. That is not part of standard functionality. If they do, they should expect to lose their ETH. And if someone else gets their ETH, that's better than it being stuck in the contract.

[GalloDaSballo]

I have to agree with the sponsor that the contract is meant to be called with a msg.value, and for the sake of not wasting resources it checks the balance of token / ETH it has at the time of the call and it accredits it to the caller, this is very similar to how most periphery / router would work

I fail to see how the contract would receive free ETH and in lack of that argument, I don't believe there's any vulnerability here