search

code-423n4 / 2021-10-tally-findings

0 stars 0 forks source link

Contract does not work well with fee-on transfer tokens #40

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

cmichel

Vulnerability details

There exist ERC20 tokens that made certain customizations to their contract. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().

The Swap.swapByQuote function performs a safeTransferFrom(msg.sender, this, amountToSell) and then approves the same amountToSell which will be more than what the Swap contract actually received due to fees.

Impact

The zrxAllowanceTarget contract is allowed to transfer more than what was transferred to the contract post fees. If the Swap contract accumulated swap fees in this token, part of them (amountToSell - amountToSellPostFees) can be stolen/are allowed to be transferred from the Swap contract.

Recommended Mitigation Steps

One possible mitigation is to measure the asset change right before and after the asset-transferring IERC20(zrxSellTokenAddress).safeTransferFrom(msg.sender, address(this), amountToSell) call, and then only approve the zrxAllowanceTarget with the difference.

Shadowfiend commented 2 years ago

We may tackle this as part of mitigating #37, since we're considering how to rework the allowance/transfer pattern here.