Uniswap Reentrancy Vulnerability Disclosure

By the Dedaub team!

Uniswap Reentrancy | Uniswap Labs recently advertised a boosted $3M bounty program for bug reports over their smart contracts, and especially the new UniversalRouter and Permit2 functionality. We submitted a bug report and received a bounty - thank you! To our knowledge, ours was the only bug report that Uniswap acted upon (i.e., the only one to have apparently resulted in a commit-fix to smart contract code and a [new deployment of the UniversalRouter](https://app.dedaub.com/ethereum/address/0xef1c6e67703c7bd7107eed8303fb e6ec2554bf6b/bytecode)).

The explanation of the issue is fairly straightforward, so we begin with the verbatim text of our bug report.

Clearly, the UniversalRouter should not hold any balances between transactions, or these can be emptied by anyone (e.g., by calling dispatch with a Commands.TRANSFER, or Commands.SWEEP). This property is dangerous when combined with any possibility of untrusted parties gaining control in the middle of a user transaction. This can happen, e.g., with tainted ERC20 tokens in Uniswap pools, or with callbacks from token transfers (e.g., 721s). E.g., simple attack scenario: a user sends to the UniversalRouter funds and calls it with 3 commands:

1. Commands.V3_SWAP_EXACT_IN, swap to a specific token
2. Commands.LOOKS_RARE_721 (or any of a large number of commands that trigger recipient callbacks), purchase NFT, send it to recipientX
3. sweep amount remaining after purchase to original caller. In this case, recipientX can easily reenter the contract (by calling transfer or sweep inside its onERC721Received handler) and drain the entire amount, i.e., also the amount of step 3.

In essence, the UniversalRouter is a scripting language for all sorts of token transfers, swaps, and NFT purchases. One can perform several actions in a row, by supplying the right sequence of commands. These commands could include transfers to untrusted recipients. In this case, it is natural to expect that the transfer should send to the recipient only what the call parameters specify, and nothing more.

However, this is not always what would happen. If untrusted code is invoked at any point in the transfer, the code can re-enter the UniversalRouter and claim any tokens already in the UniversalRouter contract. Such tokens can, for instance, exist because the user intends to later buy an NFT, or transfer tokens to a second recipient, or because the user swaps a larger amount than needed and intends to “sweep” the remainder to themselves at the end of the UniversalRouter call. And there is no shortage of scenarios in which an untrusted recipient may be called: WETH unwrapping triggers a callback, there are tokens that perform callbacks, and tokens could be themselves untrusted, executing arbitrary code when their functions get called.

Our proof-of-concept demonstrated an easy scenario:

Attached are two files. One is a replacement (slight addition) to your foundry test file, universal-router/test/foundry-tests/UniversalRouter.t.sol. The other should be dropped into the “mock” subdirectory. If you then run your standard forge test -vvv you will see for the relevant test the output:
======
[PASS] testSweepERC1155Attack() (gas: 126514)
Logs: 1000000000000000000
======
The last line is a console.log from the attacker showing that she got a balance in an erc20 token, although she was only sent an ERC1155 token.
The test case is simply:

function testSweepERC1155Attack() public {
bytes memory commands = abi.encodePacked(bytes1(uint8(Commands.SWEEP_ERC1155)));
bytes[] memory inputs = new bytes[](1);
uint256 id = 0;
inputs[0] = abi.encode(address(erc1155), attacker, id, 0);

erc1155.mint(address(router), id, AMOUNT);
erc20.mint(address(router), AMOUNT);

router.execute(commands, inputs);
}

That is, the attacker is being sent an erc1155 token amount, but the router also has an erc20 token amount. (In reality, this would probably be due to other transfers, to happen later.) The attacker manages to steal the erc20 amount as well.

The remedy we suggested was easy:

… add a Uniswap reentrancy lock, although inelegant: no dispatching commands while in the middle of dispatching commands.

We got immediate confirmation that the issue is being examined and will be assessed for the bounty program. A couple of weeks later, we received the bounty assessment:

we would like to award you a one-time bounty of $40,000 (payable in USDC) for your contribution. This amount includes a $30,000 bounty for the report, as well as a 33% bonus for reporting the issue during our current bonus period. We have classified the issue as medium severity and appreciate your assistance in helping us improve the safety of our users and the web3 ecosystem as a whole.

Further communication clarified that the bug was assessed to have High impact and Low likelihood: the possibility of a user sending NFTs to an untrusted recipient directly (although present in the UniversalRouter unit tests) is considered “user error”. Therefore, only much more complex (and less likely) scenarios were considered valid for Uniswap reentrancy, resulting in the “low likelihood” rating.

We want to thank Uniswap Labs for the bounty and are happy to have helped!