From 4ce5b585972b6530b31b427df4e165b9f5bcde4d Mon Sep 17 00:00:00 2001 From: Eric Zhong Date: Fri, 28 Jun 2024 12:02:42 -0400 Subject: [PATCH] feat: update the FOT detector to check for external fees (#24) * Add support for checking if token has external fees * use factoryV2 as sentinel test address * Add mock fot token and test cases * flat fee for external fees token and fix tests * detect external transfer failure * Rename to feeTakenOnTransfer and add mock token test * forge fmt * comment * remove debug logs * foundry.toml fix * fix foundry toml * remove rpc in foundry toml * comments * nit readability * forge fmt * custom error and comment * func name * comments - readability * var names * bubble up reason in unknown transfer error case * nit readability --------- Co-authored-by: ConjunctiveNormalForm --- src/FeeOnTransferDetector.sol | 105 ++++++++++++++++++--- test/FeeOnTransferDetector.t.sol | 56 +++++++++++ test/mock/MockFotTokenWithExternalFees.sol | 39 ++++++++ test/mock/MockReentrancyFotToken.sol | 47 +++++++++ 4 files changed, 233 insertions(+), 14 deletions(-) create mode 100644 test/mock/MockFotTokenWithExternalFees.sol create mode 100644 test/mock/MockReentrancyFotToken.sol diff --git a/src/FeeOnTransferDetector.sol b/src/FeeOnTransferDetector.sol index 7cbfad9..64d8f76 100644 --- a/src/FeeOnTransferDetector.sol +++ b/src/FeeOnTransferDetector.sol @@ -10,6 +10,8 @@ import "./lib/UniswapV2Library.sol"; struct TokenFees { uint256 buyFeeBps; uint256 sellFeeBps; + bool feeTakenOnTransfer; + bool externalTransferFailed; } /// @notice Detects the buy and sell fee for a fee-on-transfer token @@ -18,6 +20,7 @@ contract FeeOnTransferDetector { error SameToken(); error PairLookupFailed(); + error UnknownExternalTransferFailure(string reason); uint256 constant BPS = 10_000; address internal immutable factoryV2; @@ -69,18 +72,18 @@ contract FeeOnTransferDetector { (uint256 amount0Out, uint256 amount1Out) = token == token0Address ? (amountToBorrow, uint256(0)) : (uint256(0), amountToBorrow); - uint256 balanceBeforeLoan = ERC20(token).balanceOf(address(this)); + uint256 detectorBalanceBeforeLoan = ERC20(token).balanceOf(address(this)); IUniswapV2Pair pair = IUniswapV2Pair(pairAddress); - try pair.swap(amount0Out, amount1Out, address(this), abi.encode(balanceBeforeLoan, amountToBorrow)) {} + try pair.swap(amount0Out, amount1Out, address(this), abi.encode(detectorBalanceBeforeLoan, amountToBorrow)) {} catch (bytes memory reason) { result = parseRevertReason(reason); } } function parseRevertReason(bytes memory reason) private pure returns (TokenFees memory) { - if (reason.length != 64) { + if (reason.length != 128) { assembly { revert(add(32, reason), mload(reason)) } @@ -95,24 +98,81 @@ contract FeeOnTransferDetector { ERC20 tokenBorrowed = ERC20(amount0 > 0 ? token0 : token1); - (uint256 balanceBeforeLoan, uint256 amountRequestedToBorrow) = abi.decode(data, (uint256, uint256)); - uint256 amountBorrowed = tokenBorrowed.balanceOf(address(this)) - balanceBeforeLoan; + (uint256 detectorBalanceBeforeLoan, uint256 amountRequestedToBorrow) = abi.decode(data, (uint256, uint256)); + uint256 amountBorrowed = tokenBorrowed.balanceOf(address(this)) - detectorBalanceBeforeLoan; - uint256 buyFeeBps = (amountRequestedToBorrow - amountBorrowed) * BPS / amountRequestedToBorrow; - balanceBeforeLoan = tokenBorrowed.balanceOf(address(pair)); - uint256 sellFeeBps; + uint256 buyFeeBps = _calculateBuyFee(amountRequestedToBorrow, amountBorrowed); + + (bool feeTakenOnTransfer, bool externalTransferFailed) = + tryExternalTransferAndRevert(tokenBorrowed, amountBorrowed); + + uint256 sellFeeBps = _calculateSellFee(pair, tokenBorrowed, amountBorrowed, buyFeeBps); + + bytes memory fees = abi.encode( + TokenFees({ + buyFeeBps: buyFeeBps, + sellFeeBps: sellFeeBps, + feeTakenOnTransfer: feeTakenOnTransfer, + externalTransferFailed: externalTransferFailed + }) + ); + + // revert with the abi encoded fees + assembly { + revert(add(32, fees), mload(fees)) + } + } + + /// @notice simple helper function to calculate the buy fee in bps + function _calculateBuyFee(uint256 amountRequestedToBorrow, uint256 amountBorrowed) + internal + pure + returns (uint256 buyFeeBps) + { + buyFeeBps = (amountRequestedToBorrow - amountBorrowed) * BPS / amountRequestedToBorrow; + } + + /// @notice helper function to calculate the sell fee in bps + /// - in the case where the transfer fails, we set the sell fee to be the same as the buy fee + function _calculateSellFee(IUniswapV2Pair pair, ERC20 tokenBorrowed, uint256 amountBorrowed, uint256 buyFeeBps) + internal + returns (uint256 sellFeeBps) + { + uint256 pairBalanceBeforeSell = tokenBorrowed.balanceOf(address(pair)); try this.callTransfer(tokenBorrowed, address(pair), amountBorrowed) { - uint256 sellFee = amountBorrowed - (tokenBorrowed.balanceOf(address(pair)) - balanceBeforeLoan); + uint256 amountSold = tokenBorrowed.balanceOf(address(pair)) - pairBalanceBeforeSell; + uint256 sellFee = amountBorrowed - amountSold; sellFeeBps = sellFee * BPS / amountBorrowed; } catch (bytes memory) { sellFeeBps = buyFeeBps; } + } - bytes memory fees = abi.encode(TokenFees({buyFeeBps: buyFeeBps, sellFeeBps: sellFeeBps})); - - // revert with the abi encoded fees - assembly { - revert(add(32, fees), mload(fees)) + /// @notice some tokens take fees even when not buy/selling to the pair, + /// or they fail when transferred within the context of an existing swap + /// @return feeTakenOnTransfer boolean indicating whether or not a fee is taken on transfers not involving the pair + /// @return externalTransferFailed boolean indicating whether or not the external transfer failed + function tryExternalTransferAndRevert(ERC20 tokenBorrowed, uint256 amountBorrowed) + internal + returns (bool feeTakenOnTransfer, bool externalTransferFailed) + { + uint256 balanceBeforeLoan = tokenBorrowed.balanceOf(factoryV2); + try this.callTransfer(tokenBorrowed, factoryV2, amountBorrowed, balanceBeforeLoan + amountBorrowed) {} + catch (bytes memory revertData) { + if (revertData.length > 32) { + // transfer itself failed so we did not return abi-encoded `feeTakenOnTransfer` boolean variable + assembly { + revertData := add(revertData, 0x04) + } + string memory reason = abi.decode(revertData, (string)); + if (keccak256(bytes(reason)) == keccak256(bytes("TRANSFER_FAILED"))) { + externalTransferFailed = true; + } else { + revert UnknownExternalTransferFailure(reason); + } + } else { + feeTakenOnTransfer = abi.decode(revertData, (bool)); + } } } @@ -121,4 +181,21 @@ contract FeeOnTransferDetector { function callTransfer(ERC20 token, address to, uint256 amount) external { token.safeTransfer(to, amount); } + + // function that reverts with a boolean indicating whether or not a fee is taken on the token transfer + // bubbles up any reverts from the token transfer + function callTransfer(ERC20 token, address to, uint256 amount, uint256 expectedBalance) external { + try this.callTransfer(token, to, amount) {} + catch (bytes memory revertData) { + if (revertData.length < 68) revert(); + assembly { + revertData := add(revertData, 0x04) + } + revert(abi.decode(revertData, (string))); + } + bytes memory feeTakenOnTransfer = abi.encode(token.balanceOf(to) != expectedBalance); + assembly { + revert(add(32, feeTakenOnTransfer), mload(feeTakenOnTransfer)) + } + } } diff --git a/test/FeeOnTransferDetector.t.sol b/test/FeeOnTransferDetector.t.sol index b4bd4cc..223cd42 100644 --- a/test/FeeOnTransferDetector.t.sol +++ b/test/FeeOnTransferDetector.t.sol @@ -6,6 +6,8 @@ import {stdError} from "forge-std/StdError.sol"; import {TokenFees, FeeOnTransferDetector} from "../src/FeeOnTransferDetector.sol"; import {MockV2Factory} from "./mock/MockV2Factory.sol"; import {MockFotToken} from "./mock/MockFotToken.sol"; +import {MockFotTokenWithExternalFees} from "./mock/MockFotTokenWithExternalFees.sol"; +import {MockReentrancyFotToken} from "./mock/MockReentrancyFotToken.sol"; import {MockToken} from "./mock/MockToken.sol"; interface IUniswapV2Pair { @@ -33,6 +35,40 @@ contract FeeOnTransferDetectorTest is Test { TokenFees memory fees = detector.validate(address(fotToken), address(otherToken), 1 ether); assertEq(fees.buyFeeBps, 200); assertEq(fees.sellFeeBps, 500); + assertEq(fees.feeTakenOnTransfer, false); + assertEq(fees.externalTransferFailed, false); + } + + function testBasicFotTokenWithExternalFees() public { + MockFotTokenWithExternalFees fotToken = new MockFotTokenWithExternalFees(500); + MockToken otherToken = new MockToken(); + address pair = factory.deployPair(address(fotToken), address(otherToken)); + fotToken.setPair(pair); + fotToken.mint(pair, 100 ether); + otherToken.mint(pair, 100 ether); + IUniswapV2Pair(pair).sync(); + + TokenFees memory fees = detector.validate(address(fotToken), address(otherToken), 1 ether); + assertEq(fees.buyFeeBps, 500); + assertEq(fees.sellFeeBps, 500); + assertEq(fees.feeTakenOnTransfer, true); + assertEq(fees.externalTransferFailed, false); + } + + function testReentrancyFotToken() public { + MockReentrancyFotToken fotToken = new MockReentrancyFotToken(500); + MockToken otherToken = new MockToken(); + address pair = factory.deployPair(address(fotToken), address(otherToken)); + fotToken.setPair(pair); + fotToken.mint(pair, 100 ether); + otherToken.mint(pair, 100 ether); + IUniswapV2Pair(pair).sync(); + + TokenFees memory fees = detector.validate(address(fotToken), address(otherToken), 1 ether); + assertEq(fees.buyFeeBps, 500); + assertEq(fees.sellFeeBps, 500); + assertEq(fees.feeTakenOnTransfer, false); + assertEq(fees.externalTransferFailed, true); } function testBasicFotTokenFuzz(uint16 buyFee, uint16 sellFee) public { @@ -49,6 +85,26 @@ contract FeeOnTransferDetectorTest is Test { TokenFees memory fees = detector.validate(address(fotToken), address(otherToken), 1 ether); assertEq(fees.buyFeeBps, buyFee); assertEq(fees.sellFeeBps, sellFee); + assertEq(fees.feeTakenOnTransfer, false); + assertEq(fees.externalTransferFailed, false); + } + + function testBasicFotTokenWithExternalFeesFuzz(uint16 fee) public { + fee = uint16(bound(fee, 0, 10000)); + MockFotTokenWithExternalFees fotToken = new MockFotTokenWithExternalFees(fee); + MockToken otherToken = new MockToken(); + address pair = factory.deployPair(address(fotToken), address(otherToken)); + fotToken.setPair(pair); + fotToken.mint(pair, 100 ether); + otherToken.mint(pair, 100 ether); + IUniswapV2Pair(pair).sync(); + + TokenFees memory fees = detector.validate(address(fotToken), address(otherToken), 1 ether); + assertEq(fees.buyFeeBps, fee); + assertEq(fees.sellFeeBps, fee); + bool feeTakenOnTransfer = (fee == 0 && fee == 0) ? false : true; + assertEq(fees.feeTakenOnTransfer, feeTakenOnTransfer); + assertEq(fees.externalTransferFailed, false); } function testTransferFailsErrorPassthrough() public { diff --git a/test/mock/MockFotTokenWithExternalFees.sol b/test/mock/MockFotTokenWithExternalFees.sol new file mode 100644 index 0000000..b34368c --- /dev/null +++ b/test/mock/MockFotTokenWithExternalFees.sol @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +pragma solidity =0.8.19; + +import {ERC20} from "solmate/tokens/ERC20.sol"; + +contract MockFotTokenWithExternalFees is ERC20 { + uint256 public taxBps; + address public pair; + + constructor(uint256 _taxBps) ERC20("MockFotToken", "MFOT", 18) { + taxBps = _taxBps; + } + + function setPair(address _pair) external { + pair = _pair; + } + + function mint(address to, uint256 amount) external { + _mint(to, amount); + } + + // this token takes fees on ALL transfers, treating transfers to other addresses as sells + function transfer(address to, uint256 amount) public override returns (bool) { + balanceOf[msg.sender] -= amount; + + // Cannot overflow because the sum of all user + // balances can't exceed the max uint256 value. + uint256 feeAmount; + unchecked { + feeAmount = amount * taxBps / 10000; + balanceOf[to] += amount - feeAmount; + balanceOf[address(this)] += feeAmount; + } + + emit Transfer(msg.sender, to, amount - feeAmount); + + return true; + } +} diff --git a/test/mock/MockReentrancyFotToken.sol b/test/mock/MockReentrancyFotToken.sol new file mode 100644 index 0000000..1ca2c5d --- /dev/null +++ b/test/mock/MockReentrancyFotToken.sol @@ -0,0 +1,47 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +pragma solidity =0.8.19; + +import {ERC20} from "solmate/tokens/ERC20.sol"; +import "v2-core/interfaces/IUniswapV2Pair.sol"; + +contract MockReentrancyFotToken is ERC20 { + uint256 public taxBps; + address public pair; + + constructor(uint256 _taxBps) ERC20("MockReentrancyFotToken", "MFOT", 18) { + taxBps = _taxBps; + } + + function setPair(address _pair) external { + pair = _pair; + } + + function mint(address to, uint256 amount) external { + _mint(to, amount); + } + + // this token takes a fee on all transfers, and swaps the fee on the V2 pair for external transfers (not buy/sells) + function transfer(address to, uint256 amount) public override returns (bool) { + balanceOf[msg.sender] -= amount; + + // Cannot overflow because the sum of all user + // balances can't exceed the max uint256 value. + uint256 feeAmount; + unchecked { + feeAmount = amount * taxBps / 10000; + balanceOf[to] += amount - feeAmount; + balanceOf[address(this)] += feeAmount; + } + + // Only add in extra swap if is not buy/sell on the pair + if (to != pair && msg.sender != pair) { + IUniswapV2Pair(pair).token0() == address(this) + ? IUniswapV2Pair(pair).swap(0, feeAmount, address(this), new bytes(0)) + : IUniswapV2Pair(pair).swap(feeAmount, 0, address(this), new bytes(0)); + } + + emit Transfer(msg.sender, to, amount - feeAmount); + + return true; + } +}