search

Uniswap / v3-periphery

🦄 🦄 🦄 Peripheral smart contracts for interacting with Uniswap v3
https://uniswap.org
GNU General Public License v2.0
1.17k stars 1.09k forks source link

Incorrectly decoding revert reason in Multicall #254

Open maAPPsDEV opened 2 years ago

maAPPsDEV commented 2 years ago

Background

In Multicall.sol, it tries to decode revert messages in order to bubble the reason up. Originally, inspired by https://ethereum.stackexchange.com/a/83577

Description

The current implementation for decoding revert messages is incorrect. It screws up the length of _returnData before passing it to abi.decode. The screwed up length is much bigger than it should be. As long as abi.decode() ignores the length of bytes data input, it doesn't lead to any flaws. But it doesn't need to rely on it.

JorgeAtPaladin commented 2 years ago

We completely agree, with our clients we recommend updating the multicall revert to:

assembly {
    revert(add(result,32), mload(result))
}

Just blindly passing the revert bytes seems the most desirable solution in our opinion. The uniswap multicall only works with revert("string") errors and not with revert error(); errors, which in our opinion seems highly undesirable. It also does weird assembly magic as it shifts the error signature into the result.length, which is obviously undesirable.