search

code-423n4 / 2022-01-openleverage-findings

0 stars 0 forks source link

Two pairs can have same tokens #259

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

sirhashalot

Vulnerability details

Impact

The createLPoolPair() function in ControllerV1.sol tries to prevent a pair from being create if it already exists. It does this with the statement require(lpoolPairs[token0][token1].lpool0 == address(0) || lpoolPairs[token1][token0].lpool0 == address(0), 'pool pair exists');. This statement actually allows two pairs to be created with the same tokens. Uniswap v2 core and other major DEXes have code to prevent this situation.

A DEX with duplicate pairs has a foundational flaw. Multiple pairs of the same assets would split liquidity for the asset pair between two contracts. A user can easily mix up which of the duplicate pairs they are interacting with because it depends on the order of ERC20 token parameters in the pair, and because the exchange rate may be different in the two pools, a user may not get the rate they expected. Other operations in the contract can also get mixed up in this situation because it is unclear which of the duplicate pairs the user wants to interact with.

Proof of Concept

The issue is in line 68 of the createLPoolPair() function in ControllerV1.sol

require(lpoolPairs[token0][token1].lpool0 == address(0) || lpoolPairs[token1][token0].lpool0 == address(0), 'pool pair exists');

This require statement will NOT revert if one LP exists with the token pair provided. It will only revert if both of the two LPs with this token pair exist. This means that if lpoolPairs[token0][token1].lpool0 exists, the LP lpoolPairs[token1][token0].lpool0 can be created. By creating two LPs with the same tokens, liquidity will be split between the pools and this can confuse users, who would have the be cautious about the order of the tokens they use so they interact with the proper LP.

Recommended Mitigation Steps

Uniswap V2 Core solves this by first sorting the addresses before checking if the pair exists. This solution keeps pairs consistent and removes confusion. https://github.com/Uniswap/v2-core/blob/4dd59067c76dea4a0e8e4bfdda41877a6b16dedc/contracts/UniswapV2Factory.sol#L25-L27 Uniswap v2 also maps a pair to both orderings of the asset pair in the getPair mapping with these two lines. This prevents users of the DEX from needing to order the assets in a specific order https://github.com/Uniswap/v2-core/blob/4dd59067c76dea4a0e8e4bfdda41877a6b16dedc/contracts/UniswapV2Factory.sol#L34-L35

0xleastwood commented 2 years ago

The createLPoolPair function will set both lpoolPairs[token0][token1] and lpoolPairs[token1][token0] upon creating a new token pair. This means the order of token0 and token1 isn't so important.