Skip to content

Commit

Permalink
Merge pull request #310 from 1inch/feat/audit-fixes
Browse files Browse the repository at this point in the history 
Feat/audit fixes
  • Loading branch information
@ZumZoom
ZumZoom authored Apr 10, 2024
2 parents c848767 + 8c19fb9 commit 2358bd0
Show file tree
Hide file tree
Showing 2 changed files with 157 additions and 22 deletions.
89 changes: 77 additions & 12 deletions contracts/extensions/FeeTaker.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,62 @@ pragma solidity 0.8.23;

import { Address, AddressLib } from "@1inch/solidity-utils/contracts/libraries/AddressLib.sol";
import { SafeERC20 } from "@1inch/solidity-utils/contracts/libraries/SafeERC20.sol";
import { UniERC20 } from "@1inch/solidity-utils/contracts/libraries/UniERC20.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";

import { IOrderMixin } from "../interfaces/IOrderMixin.sol";
import { IPostInteraction } from "../interfaces/IPostInteraction.sol";
import { MakerTraits, MakerTraitsLib } from "../libraries/MakerTraitsLib.sol";

/// @title Helper contract that adds feature of collecting fee in takerAsset
contract FeeTaker is IPostInteraction {
contract FeeTaker is IPostInteraction, Ownable {
using AddressLib for Address;
using SafeERC20 for IERC20;
using UniERC20 for IERC20;
using MakerTraitsLib for MakerTraits;

uint256 internal constant _FEE_BASE = 1e7;
/**
* @dev The caller is not the limit order protocol contract.
*/
error OnlyLimitOrderProtocol();

/**
* @dev Eth transfer failed. The target fallback may have reverted.
*/
error EthTransferFailed();

/// @dev Allows fees in range [1e-5, 0.65535]
uint256 internal constant _FEE_BASE = 1e5;

address private immutable _LIMIT_ORDER_PROTOCOL;
address private immutable _WETH;

/// @dev Modifier to check if the caller is the limit order protocol contract.
modifier onlyLimitOrderProtocol {
if (msg.sender != _LIMIT_ORDER_PROTOCOL) revert OnlyLimitOrderProtocol();
_;
}

/**
* @notice Initializes the contract.
* @param limitOrderProtocol The limit order protocol contract.
*/
constructor(address limitOrderProtocol, address weth, address owner) Ownable(owner) {
_LIMIT_ORDER_PROTOCOL = limitOrderProtocol;
_WETH = weth;
}

/**
* @notice Fallback function to receive ETH.
*/
receive() external payable {}

/**
* @notice See {IPostInteraction-postInteraction}.
* @dev Takes the fee in taking tokens and transfers the rest to the maker.
* `extraData` consists of:
* 3 bytes — fee percentage (in 1e7)
* 2 bytes — fee percentage (in 1e5)
* 20 bytes — fee recipient
* 20 bytes — receiver of taking tokens (optional, if not set, maker is used)
*/
Expand All @@ -33,21 +72,47 @@ contract FeeTaker is IPostInteraction {
uint256 takingAmount,
uint256 /* remainingMakingAmount */,
bytes calldata extraData
) external {
uint256 fee = takingAmount * uint256(uint24(bytes3(extraData))) / _FEE_BASE;
address feeRecipient = address(bytes20(extraData[3:23]));
) external onlyLimitOrderProtocol {
uint256 fee = takingAmount * uint256(uint16(bytes2(extraData))) / _FEE_BASE;
address feeRecipient = address(bytes20(extraData[2:22]));

address receiver = order.maker.get();
if (extraData.length > 23) {
receiver = address(bytes20(extraData[23:43]));
if (extraData.length > 22) {
receiver = address(bytes20(extraData[22:42]));
}

if (fee > 0) {
IERC20(order.takerAsset.get()).safeTransfer(feeRecipient, fee);
bool isEth = order.takerAsset.get() == address(_WETH) && order.makerTraits.unwrapWeth();

if (isEth) {
if (fee > 0) {
_sendEth(feeRecipient, fee);
}
unchecked {
_sendEth(receiver, takingAmount - fee);
}
} else {
if (fee > 0) {
IERC20(order.takerAsset.get()).safeTransfer(feeRecipient, fee);
}
unchecked {
IERC20(order.takerAsset.get()).safeTransfer(receiver, takingAmount - fee);
}
}
}

/**
* @notice Retrieves funds accidently sent directly to the contract address
* @param token ERC20 token to retrieve
* @param amount amount to retrieve
*/
function rescueFunds(IERC20 token, uint256 amount) external onlyOwner {
token.uniTransfer(payable(msg.sender), amount);
}

unchecked {
IERC20(order.takerAsset.get()).safeTransfer(receiver, takingAmount - fee);
function _sendEth(address target, uint256 amount) private {
(bool success, ) = target.call{value: amount}("");
if (!success) {
revert EthTransferFailed();
}
}
}
90 changes: 80 additions & 10 deletions test/FeeTaker.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const { ethers } = hre;
const { expect } = require('@1inch/solidity-utils');
const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
const { deploySwapTokens } = require('./helpers/fixtures');
const { buildOrder, buildTakerTraits, signOrder } = require('./helpers/orderUtils');
const { buildOrder, buildTakerTraits, signOrder, buildMakerTraits } = require('./helpers/orderUtils');
const { ether, trim0x } = require('./helpers/utils');

describe('FeeTaker', function () {
Expand All @@ -30,7 +30,7 @@ describe('FeeTaker', function () {
await inch.connect(addr1).approve(swap, ether('1000000'));

const FeeTaker = await ethers.getContractFactory('FeeTaker');
const feeTaker = await FeeTaker.deploy();
const feeTaker = await FeeTaker.deploy(swap, weth, addr);

return { dai, weth, inch, swap, chainId, feeTaker };
};
Expand All @@ -53,7 +53,7 @@ describe('FeeTaker', function () {
takingAmount,
},
{
postInteraction: await feeTaker.getAddress() + trim0x(ethers.solidityPacked(['uint24', 'address'], [fee, feeRecipient])),
postInteraction: await feeTaker.getAddress() + trim0x(ethers.solidityPacked(['uint16', 'address'], [fee, feeRecipient])),
},
);

Expand Down Expand Up @@ -86,7 +86,7 @@ describe('FeeTaker', function () {
},
{
postInteraction: await feeTaker.getAddress() +
trim0x(ethers.solidityPacked(['uint24', 'address', 'address'], [fee, feeRecipient, makerReceiver])),
trim0x(ethers.solidityPacked(['uint16', 'address', 'address'], [fee, feeRecipient, makerReceiver])),
},
);

Expand All @@ -104,8 +104,8 @@ describe('FeeTaker', function () {

const makingAmount = ether('300');
const takingAmount = ether('0.3');
const fee = BigInt(1e6);
const feeCalculated = takingAmount * fee / BigInt(1e7);
const fee = BigInt(1e4);
const feeCalculated = takingAmount * fee / BigInt(1e5);
const feeRecipient = addr2.address;

const order = buildOrder(
Expand All @@ -118,7 +118,7 @@ describe('FeeTaker', function () {
takingAmount,
},
{
postInteraction: await feeTaker.getAddress() + trim0x(ethers.solidityPacked(['uint24', 'address'], [fee, feeRecipient])),
postInteraction: await feeTaker.getAddress() + trim0x(ethers.solidityPacked(['uint16', 'address'], [fee, feeRecipient])),
},
);

Expand All @@ -136,8 +136,8 @@ describe('FeeTaker', function () {

const makingAmount = ether('300');
const takingAmount = ether('0.3');
const fee = BigInt(1e6);
const feeCalculated = takingAmount * fee / BigInt(1e7);
const fee = BigInt(1e4);
const feeCalculated = takingAmount * fee / BigInt(1e5);
const feeRecipient = addr2.address;
const makerReceiver = addr3.address;

Expand All @@ -152,7 +152,7 @@ describe('FeeTaker', function () {
},
{
postInteraction: await feeTaker.getAddress() +
trim0x(ethers.solidityPacked(['uint24', 'address', 'address'], [fee, feeRecipient, makerReceiver])),
trim0x(ethers.solidityPacked(['uint16', 'address', 'address'], [fee, feeRecipient, makerReceiver])),
},
);

Expand All @@ -164,4 +164,74 @@ describe('FeeTaker', function () {
await expect(fillTx).to.changeTokenBalances(dai, [addr, addr1], [makingAmount, -makingAmount]);
await expect(fillTx).to.changeTokenBalances(weth, [addr, addr1, addr2, addr3], [-takingAmount, 0, feeCalculated, takingAmount - feeCalculated]);
});

it('should charge fee in eth', async function () {
const { dai, weth, swap, chainId, feeTaker } = await loadFixture(deployContractsAndInit);

const makingAmount = ether('300');
const takingAmount = ether('0.3');
const fee = BigInt(1e4);
const feeCalculated = takingAmount * fee / BigInt(1e5);
const feeRecipient = addr2.address;

const order = buildOrder(
{
maker: addr1.address,
receiver: await feeTaker.getAddress(),
makerAsset: await dai.getAddress(),
takerAsset: await weth.getAddress(),
makingAmount,
takingAmount,
makerTraits: buildMakerTraits({ unwrapWeth: true }),
},
{
postInteraction: await feeTaker.getAddress() + trim0x(ethers.solidityPacked(['uint16', 'address'], [fee, feeRecipient])),
},
);

const { r, yParityAndS: vs } = ethers.Signature.from(await signOrder(order, chainId, await swap.getAddress(), addr1));
const takerTraits = buildTakerTraits({
extension: order.extension,
});
const fillTx = swap.fillOrderArgs(order, r, vs, makingAmount, takerTraits.traits, takerTraits.args);
await expect(fillTx).to.changeTokenBalances(dai, [addr, addr1], [makingAmount, -makingAmount]);
await expect(fillTx).to.changeTokenBalance(weth, addr, -takingAmount);
await expect(fillTx).to.changeEtherBalances([addr1, addr2], [takingAmount - feeCalculated, feeCalculated]);
});

it('should charge fee in eth and send the rest to the maker receiver', async function () {
const { dai, weth, swap, chainId, feeTaker } = await loadFixture(deployContractsAndInit);

const makingAmount = ether('300');
const takingAmount = ether('0.3');
const fee = BigInt(1e4);
const feeCalculated = takingAmount * fee / BigInt(1e5);
const feeRecipient = addr2.address;
const makerReceiver = addr3.address;

const order = buildOrder(
{
maker: addr1.address,
receiver: await feeTaker.getAddress(),
makerAsset: await dai.getAddress(),
takerAsset: await weth.getAddress(),
makingAmount,
takingAmount,
makerTraits: buildMakerTraits({ unwrapWeth: true }),
},
{
postInteraction: await feeTaker.getAddress() +
trim0x(ethers.solidityPacked(['uint16', 'address', 'address'], [fee, feeRecipient, makerReceiver])),
},
);

const { r, yParityAndS: vs } = ethers.Signature.from(await signOrder(order, chainId, await swap.getAddress(), addr1));
const takerTraits = buildTakerTraits({
extension: order.extension,
});
const fillTx = swap.fillOrderArgs(order, r, vs, makingAmount, takerTraits.traits, takerTraits.args);
await expect(fillTx).to.changeTokenBalances(dai, [addr, addr1], [makingAmount, -makingAmount]);
await expect(fillTx).to.changeTokenBalance(weth, addr, -takingAmount);
await expect(fillTx).to.changeEtherBalances([addr1, addr2, addr3], [0, feeCalculated, takingAmount - feeCalculated]);
});
});

0 comments on commit 2358bd0

Please sign in to comment.