Skip to content
Permalink
Browse files Browse the repository at this point in the history
[to consider] dirty reentrancy lock (#189)
* Gas costs for clean reentrancy lock

* Gas costs for direct reentrancy lock

* Reentrancy test

* bytecode increase
  • Loading branch information
hensha256 committed Nov 28, 2022
1 parent 3615111 commit d82c668
Show file tree
Hide file tree
Showing 19 changed files with 136 additions and 91 deletions.
5 changes: 3 additions & 2 deletions contracts/UniversalRouter.sol
Expand Up @@ -7,8 +7,9 @@ import {RouterParameters, RouterImmutables} from './base/RouterImmutables.sol';
import {Constants} from './libraries/Constants.sol';
import {Commands} from './libraries/Commands.sol';
import {IUniversalRouter} from './interfaces/IUniversalRouter.sol';
import {ReentrancyLock} from './base/ReentrancyLock.sol';

contract UniversalRouter is RouterImmutables, IUniversalRouter, Dispatcher, RewardsCollector {
contract UniversalRouter is RouterImmutables, IUniversalRouter, Dispatcher, RewardsCollector, ReentrancyLock {
modifier checkDeadline(uint256 deadline) {
if (block.timestamp > deadline) revert TransactionDeadlinePassed();
_;
Expand All @@ -26,7 +27,7 @@ contract UniversalRouter is RouterImmutables, IUniversalRouter, Dispatcher, Rewa
}

/// @inheritdoc IUniversalRouter
function execute(bytes calldata commands, bytes[] calldata inputs) public payable {
function execute(bytes calldata commands, bytes[] calldata inputs) public payable isNotLocked {
bool success;
bytes memory output;
uint256 numCommands = commands.length;
Expand Down
15 changes: 15 additions & 0 deletions contracts/base/ReentrancyLock.sol
@@ -0,0 +1,15 @@
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity ^0.8.17;

contract ReentrancyLock {
error ContractLocked();

uint256 private isLocked = 1;

modifier isNotLocked() {
if (isLocked != 1) revert ContractLocked();
isLocked = 2;
_;
isLocked = 1;
}
}
11 changes: 11 additions & 0 deletions contracts/test/ReenteringProtocol.sol
@@ -0,0 +1,11 @@
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity ^0.8.15;

contract ReenteringProtocol {
error NotAllowedReenter();

function callAndReenter(address universalRouter, bytes calldata data) public payable {
(bool success,) = universalRouter.call(data);
if (!success) revert NotAllowedReenter();
}
}
34 changes: 34 additions & 0 deletions test/integration-tests/UniversalRouter.test.ts
Expand Up @@ -2,6 +2,7 @@ import { UniversalRouter, Permit2, ERC20, MockLooksRareRewardsDistributor, ERC72
import { BigNumber, BigNumberish } from 'ethers'
import { Pair } from '@uniswap/v2-sdk'
import { expect } from './shared/expect'
import { abi as ROUTER_ABI } from '../../artifacts/contracts/UniversalRouter.sol/UniversalRouter.json'
import { abi as TOKEN_ABI } from '../../artifacts/solmate/tokens/ERC20.sol/ERC20.json'
import NFTX_ZAP_ABI from './shared/abis/NFTXZap.json'
import deployUniversalRouter, { deployPermit2 } from './shared/deployUniversalRouter'
Expand All @@ -16,6 +17,7 @@ import {
SOURCE_MSG_SENDER,
MAX_UINT160,
MAX_UINT,
ETH_ADDRESS,
} from './shared/constants'
import {
seaportOrders,
Expand All @@ -34,6 +36,7 @@ import hre from 'hardhat'

const { ethers } = hre
const nftxZapInterface = new ethers.utils.Interface(NFTX_ZAP_ABI)
const routerInterface = new ethers.utils.Interface(ROUTER_ABI)

describe('UniversalRouter', () => {
let alice: SignerWithAddress
Expand Down Expand Up @@ -131,6 +134,37 @@ describe('UniversalRouter', () => {
await expect(router['execute(bytes,bytes[])'](commands, inputs)).to.be.revertedWith('InvalidBips()')
})

it('reverts if a malicious contract tries to reenter', async () => {
const reentrantProtocol = await (await ethers.getContractFactory('ReenteringProtocol')).deploy()

router = (
await deployUniversalRouter(
permit2,
mockLooksRareRewardsDistributor.address,
mockLooksRareToken.address,
reentrantProtocol.address
)
).connect(alice) as UniversalRouter

planner.addCommand(CommandType.SWEEP, [ETH_ADDRESS, alice.address, 0])
let { commands, inputs } = planner

const sweepCalldata = routerInterface.encodeFunctionData('execute(bytes,bytes[])', [commands, inputs])
const reentrantCalldata = reentrantProtocol.interface.encodeFunctionData('callAndReenter', [
router.address,
sweepCalldata,
])

planner = new RoutePlanner()
planner.addCommand(CommandType.NFTX, [0, reentrantCalldata])
;({ commands, inputs } = planner)

const notAllowedReenterSelector = '0xb418cb98'
await expect(router['execute(bytes,bytes[])'](commands, inputs)).to.be.revertedWith(
`ExecutionFailed(0, "` + notAllowedReenterSelector + `")`
)
})

describe('partial fills', async () => {
let nftxValue: BigNumber
let numCovens: number
Expand Down
Expand Up @@ -3,27 +3,27 @@
exports[`Check Ownership Gas gas: checks ownership after a seaport trade, one NFT 1`] = `
Object {
"calldataByteLength": 2212,
"gasUsed": 203708,
"gasUsed": 206020,
}
`;

exports[`Check Ownership Gas gas: checks ownership after a seaport trade, two NFTs 1`] = `
Object {
"calldataByteLength": 5028,
"gasUsed": 339196,
"gasUsed": 341508,
}
`;

exports[`Check Ownership Gas gas: does not check ownership after a seaport trade, one NFT 1`] = `
Object {
"calldataByteLength": 2052,
"gasUsed": 200173,
"gasUsed": 202485,
}
`;

exports[`Check Ownership Gas gas: just ownership check 1`] = `
Object {
"calldataByteLength": 356,
"gasUsed": 30615,
"gasUsed": 32927,
}
`;
Expand Up @@ -3,6 +3,6 @@
exports[`Cryptopunks purchases 1 cryptopunk gas 1`] = `
Object {
"calldataByteLength": 356,
"gasUsed": 105779,
"gasUsed": 109868,
}
`;
Expand Up @@ -3,6 +3,6 @@
exports[`Foundation Gas Tests Buy a mental worlds NFT from Foundation gas: token id 32 of mental worlds 1`] = `
Object {
"calldataByteLength": 612,
"gasUsed": 179658,
"gasUsed": 183748,
}
`;
Expand Up @@ -3,13 +3,13 @@
exports[`LooksRare Gas Tests ERC-721 Purchase gas: buy 1 ERC-721 on looks rare 1`] = `
Object {
"calldataByteLength": 1316,
"gasUsed": 246469,
"gasUsed": 248781,
}
`;

exports[`LooksRare Gas Tests ERC-1155 Purchase gas: buy 1 ERC-1155 on looks rare 1`] = `
Object {
"calldataByteLength": 1348,
"gasUsed": 266494,
"gasUsed": 268806,
}
`;
Expand Up @@ -3,6 +3,6 @@
exports[`NFT20 Buy 3 alphabetties from NFT20 gas: purchases token ids 129, 193, 278 of Alphabetties 1`] = `
Object {
"calldataByteLength": 836,
"gasUsed": 391360,
"gasUsed": 393672,
}
`;
Expand Up @@ -3,6 +3,6 @@
exports[`NFTX Gas Tests gas: buyAndRedeem w/ specific selection 1`] = `
Object {
"calldataByteLength": 740,
"gasUsed": 490835,
"gasUsed": 494924,
}
`;
Expand Up @@ -3,48 +3,48 @@
exports[`Payments Gas Tests Individual Command Tests gas: SWEEP with ERC20 1`] = `
Object {
"calldataByteLength": 356,
"gasUsed": 37194,
"gasUsed": 39506,
}
`;

exports[`Payments Gas Tests Individual Command Tests gas: SWEEP_WITH_FEE 1`] = `
Object {
"calldataByteLength": 516,
"gasUsed": 66436,
"gasUsed": 68748,
}
`;

exports[`Payments Gas Tests Individual Command Tests gas: TRANSFER with ERC20 1`] = `
Object {
"calldataByteLength": 356,
"gasUsed": 36168,
"gasUsed": 38480,
}
`;

exports[`Payments Gas Tests Individual Command Tests gas: TRANSFER with ETH 1`] = `
Object {
"calldataByteLength": 356,
"gasUsed": 31759,
"gasUsed": 34071,
}
`;

exports[`Payments Gas Tests Individual Command Tests gas: UNWRAP_WETH 1`] = `
Object {
"calldataByteLength": 324,
"gasUsed": 44647,
"gasUsed": 46959,
}
`;

exports[`Payments Gas Tests Individual Command Tests gas: UNWRAP_WETH_WITH_FEE 1`] = `
Object {
"calldataByteLength": 644,
"gasUsed": 52003,
"gasUsed": 54315,
}
`;

exports[`Payments Gas Tests Individual Command Tests gas: WRAP_ETH 1`] = `
Object {
"calldataByteLength": 324,
"gasUsed": 53491,
"gasUsed": 57580,
}
`;

This file was deleted.

Expand Up @@ -3,13 +3,13 @@
exports[`Seaport Gas Tests gas: fulfillAdvancedOrder 1`] = `
Object {
"calldataByteLength": 2052,
"gasUsed": 200173,
"gasUsed": 202485,
}
`;

exports[`Seaport Gas Tests gas: fulfillAvailableAdvancedOrders 1 orders 1`] = `
Object {
"calldataByteLength": 4708,
"gasUsed": 332120,
"gasUsed": 334432,
}
`;
Expand Up @@ -3,6 +3,6 @@
exports[`Sudoswap Gas Tests Buy 3 sudolets from sudoswap gas: purchases token ids 80, 35, 93 of Sudolets 1`] = `
Object {
"calldataByteLength": 836,
"gasUsed": 190323,
"gasUsed": 192635,
}
`;

0 comments on commit d82c668

Please sign in to comment.