Platform: Code4rena
Start Date: 01/12/2022
Pot Size: $60,500 USDC
Total HM: 8
Participants: 27
Period: 11 days
Judge: kirk-baird
Total Solo HM: 6
Id: 187
League: ETH
Rank: 10/27
Findings: 2
Award: $1,087.24
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: Jeiwan
Also found by: peritoflores
1027.3973 USDC - $1,027.40
https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Factory.sol#L58-L62 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/router-v1/contracts/Router.sol#L124-L128 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/router-v1/contracts/Router.sol#L232 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/router-v1/contracts/Router.sol#L307
Users can lose their funds
In UniswapV3 decodeFirstPool() returns the tuple (address tokenOut, address tokenIn, uint24 fee) . From there it lookups the corresponding pool address with getPool(tokenIn, tokenOut, fee) which may not exist. See https://github.com/Uniswap/v3-periphery/blob/6cce88e63e176af1ddb6cc56e029110289622317/contracts/SwapRouter.sol#L96-L101
However, in your implementation decodeFirstPool() retrieves the address of the pool (instead of the fee that is unique in UniswapV3) which is always passed as an input parameter. But you never check that this pool belongs to the protocol so this can be a random malicious address or also another pool from other protocol.
The same problem for these functions
function addLiquidityToPool( IPool pool, uint256 tokenId, IPool.AddLiquidityParams[] calldata params, uint256 minTokenAAmount, uint256 minTokenBAmount, uint256 deadline ) function removeLiquidity( IPool pool, address recipient, uint256 tokenId, IPool.RemoveLiquidityParams[] calldata params, uint256 minTokenAAmount, uint256 minTokenBAmount, uint256 deadline ) external checkDeadline(deadline) returns (uint256 tokenAAmount, uint256 tokenBAmount, IPool.BinDelta[] memory binDelta
Be sure that after you call decodeFirstPool() the pool belongs to the protocol. For example you have a variable mapping(IPool => bool) public override isFactoryPool; in Factory.sol that you can use for this
In my opinion you should see the way to have a tuple (fee, tickspacing, tokenA, tokenB) to identify every pool and then a function getPool from those parameters. Maybe you do not even need separate pools for different lookups values but this is just a personal opinion.
#0 - hansfriese
2022-12-13T14:51:37Z
If a wrong pool is provided in the parameter, no harm to the protocol. It is very likely to revert. If it's a malicious pool address that will not revert it's the caller's responsibility.
#1 - gte620v
2022-12-15T23:56:50Z
Disagree. For this to be a problem, a user has to choose to call this function with a malicious pool as one of the inputs. There are many ways for a user to misuse a smart contract. This is analogous to inputing more tokens than the user intended to swap; ie, it is equivalent to a user error.
There are even use cases where a user values this feature because it lets them include a path that has a pool address from another protocol that implements the pool interface that the router expects.
#2 - c4-sponsor
2022-12-15T23:56:55Z
gte620v marked the issue as sponsor disputed
#3 - peritoflores
2022-12-16T18:33:02Z
Hi @gte620v @hansfriese. Let me share my thoughts on that.
I see that your protocol has the intention to be an improvement of Uniswap V3. Routers (or Agregattor) are designed to have a single point of failure where user will trust. Users pass "tokens" and they get routed to the proper pair.
If not, you could just remove the router and have all the logic in your pair and then just let users to select the proper pair which is of course pretty odd and risky.
@gte620v regarding the comment about comparison of responsibility of users passing pool address and ERC20 address. It is not the same because there are many databases of trusted tokens that are public everywhere. It is much easier to protect users for scams for example of fake USDC than for a incorrect pool. For example, if you use a Ledger the hardware will tell you that you are transferring USDC, you dont need to know USDC contract address. You cannot have that level of protection in your protocol as pairs are created all the time. Again, this is the reason why router was created.
Finally if you plan to keep this design you need to add it to your README and warn users of their responsibility. There are other contest here that have been considered medium risk because missed that comment. Still, In my opinion this is high and I recommend you to change it.
#4 - peritoflores
2022-12-16T19:29:59Z
One final word I extracted this from your web site ............. where says that "intelligently routes" "pools are not diferentiated for swappers"
https://docs.mav.xyz/guides/liquidity-providers/deploying-a-new-pool
For every token pair in Maverick AMM, there can be multiple pools with different fee tiers and bin widths. This is in order to fit the needs and strategies of different liquidity providers. These pools are not differentiated for swappers—instead, the AMM will intelligently rout their swaps to whichever pool can currently offer them the best overall price. For more information on fees, please see the section in our FAQ.
#5 - gte620v
2022-12-16T22:48:14Z
This is a feature, not a bug. It let's use use other compatible pools in the swap path.
If not, you could just remove the router and have all the logic in your pair and then just let users to select the proper pair which is of course pretty odd and risky.
There is no room in the pool for this.
It is not the same because there are many databases of trusted tokens that are public everywhere
It is the same. the vast majority of users with use a web interface to interact with the contract where this is not an issue. Users interacting directly with the contract can always check with the factory to see if a pool is a factory pool. Or, they can fork this router and add that functionality.
"intelligently routes" "pools are not differentiated for swappers"
Not sure what point you are making here. Routes are typically calculated off-chain and passed into this Router contract.
#6 - peritoflores
2022-12-16T23:43:04Z
Not sure what point you are making here. Routes are typically calculated off-chain and passed into this Router contract.
I mean documentation says that AMM will intelligently route their swaps to whichever pool .
Here is the point as my vision is that off chain calculations are not part of the AMM. But that is ok. Thanks for the reply I would like to hear C4 judge opinion.
#7 - kirk-baird
2023-01-12T21:59:32Z
I see this issue as very similar to #64.
Allowing the use or 3rd party pools is a valid design decision but there are risks associated with it.
However, the Warden has not provided a PoC that shows how an attacker may drain funds or otherwise harm the protocol by using a malicious Pool. The burden of proof is increased for a High severity issue and thus I do not consider it a duplicate of #64 or a valid High/Medium.
#8 - c4-judge
2023-01-12T21:59:39Z
kirk-baird marked the issue as unsatisfactory: Insufficient proof
#9 - c4-judge
2023-01-19T21:59:36Z
kirk-baird changed the severity to 2 (Med Risk)
#10 - c4-judge
2023-01-19T21:59:47Z
kirk-baird marked the issue as duplicate of #64
#11 - c4-judge
2023-01-19T21:59:59Z
kirk-baird marked the issue as satisfactory
#12 - c4-judge
2023-01-19T22:00:05Z
kirk-baird marked the issue as partial-50
#13 - kirk-baird
2023-01-19T22:01:42Z
On reflection there is a duplication option which allows for partial credit to be given. I believe this is appropriate to be give as a partial credit of #64 as it is the same underlying option just lacking an exploit scenario.
🌟 Selected for report: 0xSmartContract
Also found by: 0x1f8b, 0xDecorativePineapple, Chom, Deivitto, IllIllI, Jeiwan, Josiah, Mukund, RaymondFam, Rolezn, ajtra, cccz, chrisdior4, csanuragjain, hansfriese, ladboy233, minhquanym, pedr02b2, peritoflores, rvierdiiev, sakshamguruji, sces60107
59.8382 USDC - $59.84
Phishing attacks are very easy, and gas problems for low tickSpacing and high volatile assets.
UniswapV3 allow users to create pools only to some fee-tiers. Currently (0.05 %, 0.3 and 1%) that are defined in an array that maps fee to tickSpacing .
constructor() { owner = msg.sender; emit OwnerChanged(address(0), msg.sender); feeAmountTickSpacing[500] = 10; emit FeeAmountEnabled(500, 10); feeAmountTickSpacing[3000] = 60; emit FeeAmountEnabled(3000, 60); feeAmountTickSpacing[10000] = 200; emit FeeAmountEnabled(10000, 200); }
I believe that you also have the same intention, at least the GUI in your testnet do no let users to type any random fee.
There must be a relationship on these values and it is not a good idea to let users just to select any value for the reasons I will explain.
Phishing attack: setting fee to almost 100%
An attacker can create a pool with fees to 100%. Suppose is a DAI to ETH
The attacker add some tokens for example 10000 DAI and 10ETH (Very cheap ETH) The victim tries to buy 1 ETH because she thinks it is cheap. The victim lose all her money.
Small tickSpacing for volatile assets can be a gas bomb
Small tickSpacing (bin width in you case) should have a reasonable relationship with fee and volatility of the asset. Otherwise a pool created with a small tick spacing for example 1 can make the process to swap so gas consuming as the swap function will need to iterate in to many bins (if there are many active).
These can be intentional and non intentional
I believe that you should have a table similar to Uniswap V3. However, if you want to allow same fee and different widths you could create a variable uint256 feeTickSpacing with first 128 bits for fees and the remaining 128 for tickSpacing.
Finally a mapping that stores the pairs allowed.
mapping(uint256 =>bool ) public allowedFeeAmountTickSpacing;
Every time a pool is created the contract need to check if the pair is valid
require(checkValidFeeTickSpacing(_fee,_tickSpacing)," No valid fee - tickSpacing combination"); function checkValidFeeTickSpacing (uint128 _fee, uint128 _tickSpacing) public returns (bool) { uint256 feeTickSpacing = uint256(_fee)<< 128 ; feeTickSpacing = feeTickSpacing | uint256(_tickSpacing); return(allowedFeeAmountTickSpacing[feeTickSpacing]); }
Then you can have a policy to add a new pair. Uniswap does it by governance.
#0 - gte620v
2022-12-16T02:29:41Z
Flexible fees are an intentional design feature, not a bug.
Any reasonable front end will display the pool fees and show the user how much output they can expect from a swap net of fees. Any programmatic user of a pool, can also check these things with static calls.
#1 - c4-sponsor
2022-12-16T02:29:45Z
gte620v marked the issue as sponsor disputed
#2 - peritoflores
2023-01-13T16:18:38Z
Let me explain an attacking scenario.
An attacker creates a pool with a very low fee = 1 ... and tickSpacing = 1 ... for let say ETH to DAI pool
An attacker add lot of liquidity with different deltas to be sure that there are as many active ticks as possible. (Note this it posible even for non-malicious users,).
A normal user wants to swap DAI for ETH.
The protocol, as you mentioned, will choose the lowest fee pool (the one the attacker created)
Suppose the price has changed. The swap will iterate though all active bins and in average it will cost 200x more than if you would have had a tickspacing of 200 (like Uni3 )
The explanation of the decision to make tickSpacing high for volatile assets and low for stable coins is here https://docs.uniswap.org/concepts/protocol/concentrated-liquidity
#3 - gte620v
2023-01-13T16:34:26Z
The protocol, as you mentioned, will choose the lowest fee pool (the one the attacker created)
This part is not true. A user, including our frontend, can/will simulate the swap using a staticcall to understand both the price impact and the gas of a given swap across various pools. The user, or our frontend, can/will then pick the swap that has the best price net of fees and gas. As you point out, that will not necessarily be the lowest-fee pool.
The flip side of your argument makes the case for having this additional flexibility:
#4 - kirk-baird
2023-01-15T23:18:52Z
There are two issues here which I consider both of them two be Low severity.
High fees do provide a phishing attack vector and it is advisable to have reasonable bounds on the maximum fee e.g. 15%. However, users should specify the minimum amount to receive for the token which accounts for fees. Furthermore, front-ends or off-chain programs would determine the best pool based off net amount out (includes fees) rather than simply price. With that said there may still be user's who don't consider fees properly and may be exploited by picking a pool with the best price and thus this is a valid low severity issue.
Small tick spacing will significantly increase gas fees. For two stable coin assets it's expected that price changes will be minimal and thus small tick spacing is required. It is therefore a trade-off for users and liquidity providers on what see to set. User's aim to trade on the pool with lowest fees however liquidity providers will want to deposit their liquidity of pools with higher fees. Hence, there is genuine use cases for having low tick spacing and low fees.
The attack is a valid attack which would have significant gas costs for pools with price volatility. User's when signing and sending transactions are required to sign over the gas price and amount. All Web3 providers display the estimated gas usage and cost and thus user's will be aware of these costs before confirming their transaction. Thus, I consider this a low severity issue.
#5 - c4-judge
2023-01-15T23:18:58Z
kirk-baird changed the severity to QA (Quality Assurance)
#6 - c4-judge
2023-01-15T23:19:08Z
kirk-baird marked the issue as grade-a
#7 - c4-judge
2023-01-18T22:17:30Z
kirk-baird marked the issue as grade-b