Open c4-bot-6 opened 4 months ago
Hey @ZanyBonzy, thank you for the PJQA contribution. I will preface all validation repository finding responses by stating that they are not evaluated by judges directly and are only evaluated by the validators if they are deemed unsatisfactory.
Similarly to #58 of the validation repository, the operations are wrapped as an optimization rather than as a security requirement.
This paragraph is included in all of my responses and confirms that no further feedback is expected in this submission as PJQA has concluded. You are free to refute any of my statements factually, however, I strongly implore you to do this with actual code references and examples.
Lines of code
https://github.com/code-423n4/2024-06-vultisig/blob/7fb0da757c98116090f35810146ea742ca3b94da/hardhat.config.ts#L6 https://github.com/code-423n4/2024-06-vultisig/blob/7fb0da757c98116090f35810146ea742ca3b94da/hardhat-vultisig/contracts/oracles/uniswap/uniswapv0.8/FullMath.sol#L2 https://github.com/code-423n4/2024-06-vultisig/blob/7fb0da757c98116090f35810146ea742ca3b94da/hardhat-vultisig/contracts/oracles/uniswap/uniswapv0.8/FullMath.sol#L22 https://github.com/code-423n4/2024-06-vultisig/blob/7fb0da757c98116090f35810146ea742ca3b94da/hardhat-vultisig/contracts/oracles/uniswap/uniswapv0.8/FullMath.sol#L111 https://github.com/code-423n4/2024-06-vultisig/blob/7fb0da757c98116090f35810146ea742ca3b94da/hardhat-vultisig/contracts/oracles/uniswap/uniswapv0.8/TickMath.sol#L2 https://github.com/code-423n4/2024-06-vultisig/blob/7fb0da757c98116090f35810146ea742ca3b94da/hardhat-vultisig/contracts/oracles/uniswap/uniswapv0.8/TickMath.sol#L24 https://github.com/code-423n4/2024-06-vultisig/blob/7fb0da757c98116090f35810146ea742ca3b94da/hardhat-vultisig/contracts/oracles/uniswap/uniswapv0.8/TickMath.sol#L63 https://github.com/code-423n4/2024-06-vultisig/blob/7fb0da757c98116090f35810146ea742ca3b94da/hardhat-vultisig/contracts/oracles/uniswap/uniswapv0.8/OracleLibrary.sol#L75
Vulnerability details
Impact
Uniswap math libraries rely on wrapping behaviour for conducting arithmetic operations and as thus are compiled with solidity version < 0.8.0. This is because solidity versions => 0.8.0 introduced checked arithmetic by default where operations that cause an overflow would revert. The protocol hardhart vultisig part of the codebase uses solidity 0.8.24 to compile the contracts. It also holds a fork of three of uniswap's libraries, FullMath.sol, TickMath.sol and OracleLibrary.sol. Although this code was adapted from Uniswap, and is to be compiled with solc 0.8.24, the arithmetic operations are not wrapped unchecked blocks, which means that the default => 0.8.0 revert on overflow behaviour is active and will cause these operations to always revert contrary to the desired behaviour of uniswap libraries. This will cause unexpected behaviour including potentially dossing the protocol's operations.
Proof of Concept
From the readme, it can be deduced that the codebase has two contract sets, Vultisig and ILO contracts.
Vultisig contracts are written in Hardhat while using solc version 0.8.24 for compilation.
Now, in the three forked uniswap libraries, FullMath.sol, TickMath.sol and OracleLibrary.sol, they appear to be directly copied from uniswap v3 main repo directly not the 0.8 repo. And upon comparing the sets of contracts, it can be observed that although, these libraries will be compiled with solidity 0.8.24, they are missing the unchecked blocks in important places.
In FullMath.sol, Vultisig's
mulDiv
function doesn't hold the unchecked block, in comparison to Uniswap'smulDiv
function.Vultisig's
Uniswap's
The same can be observed in the
mulDivRoundingUp
function, Vultisig's and Uniswap'sThe same goes for TickMath.sol
getSqrtRatioAtTick
in Vultisig is missing the unchecked blockwhich is present in Uniswap's
and also in the
getTickAtSqrtRatio
function. Vultisig and Uniswap.FInally in OracleLibrary.sol, in the
getOldestObservationSecondsAgo
Vultisig
Uniswap
The desired wrapping behaviour will never be achieved as the libraries do not handle the overflow needed which will lead to unexpected reversions dossing the functions dependent on these libraries.
Tools Used
Manual Review
Recommended Mitigation Steps
Add the unchecked blocks to the needed functions.
Assessed type
Under/Overflow