Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fee-on transfer tokens in FeeBurner.burnToTarget will revert transaction #133

Open
code423n4 opened this issue Jun 3, 2022 · 2 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

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`

    ...
}
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jun 3, 2022
code423n4 added a commit that referenced this issue Jun 3, 2022
@chase-manning
Copy link
Collaborator

We don't support fee-on-transfer tokens

@chase-manning chase-manning added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Jun 6, 2022
@GalloDaSballo
Copy link
Collaborator

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

@GalloDaSballo GalloDaSballo added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants