code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().

Proof of Concept

The FeeBurner.burnToTarget function will try to swap more of an ERC20 token than the contract actually received and due to transfering the token into the SwapperRouter contract, the token balance is insufficient and the transfer will revert.

tokenomics/FeeBurner.sol#L70

token_.safeTransferFrom(msg.sender, address(this), tokenBalance_); // @audit-info less tokens will be received in the contract when using fee-on transfer tokens
if (address(token_) == targetUnderlying_) continue;
_approve(address(token_), address(swapperRouter_));
swapperRouter_.swap(address(token_), _WETH, tokenBalance_); // @audit-info the swap function transfers the `token_` to itself and due to `tokenBalance_` not reflecting the correct token amount in the contract, the swap will revert

Tools Used

Manual review

Recommended mitigation steps

As other contracts (e.g. AmmGauge.stakeFor) already handle fee-on transfer tokens correctly, make sure also FeeBurner.burnToTarget does so.

Compare the token balance before the transfer and after the transfer and use the delta as the actual swap amount to prevent the FeeBurner.burnToTarget function reverting for fee-on transfer tokens:

function burnToTarget(address[] memory tokens_, address targetLpToken_)
    public
    payable
    override
    returns (uint256 received)
{
    ...

    uint256 oldBal = token_.balanceOf(address(this));
    token_.safeTransferFrom(msg.sender, address(this), tokenBalance_);
    uint256 swapAmount = token_.balanceOf(address(this)) - oldBal;

    if (address(token_) == targetUnderlying_) continue;
    _approve(address(token_), address(swapperRouter_));
    swapperRouter_.swap(address(token_), _WETH, swapAmount); // @audit-info use `swapAmount`

    ...
}

chase-manning commented 2 years ago

We don't support fee-on-transfer tokens

GalloDaSballo commented 2 years ago

I believe that the finding has validity, however in case of a revert, no funds would be loss nor stuck, for that reason QA is a more appropriate Severity

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