Platform: Code4rena
Start Date: 30/09/2021
Pot Size: $100,000 SUSHI
Total HM: 24
Participants: 7
Period: 7 days
Judge: alcueca
Total Solo HM: 16
Id: 35
League: ETH
Rank: 7/7
Findings: 5
Award: $2,805.43
🌟 Selected for report: 3
🚀 Solo Findings: 0
49.5711 SUSHI - $619.64
0xsanson
In multiple functions in ConcentratedLiquidityPoolManager, the index positionId is used instead of the correct incentiveId when dealing with the incentives mapping.
Of course the issue is that incentives cannot be used, or in some cases only by lucky positions, making the contract unusable.
https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol#L68 https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol#L87 https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol#L105
editor
Change the second index to incentiveId.
#0 - sarangparikh22
2021-10-12T09:41:47Z
Duplicate of #39 Disagree with severity, It should be 2.
68.8487 SUSHI - $860.61
0xsanson
When burning a liquidity position the reserves should be decreased by the tokens' amount that leaves the contract. However in ConcentratedLiquidityPool's burn they are decreased only by the fees.
editor
Consider subtracting amount0 and amount1 to the reserves, instead of amount0fees and amount1fees.
#0 - sarangparikh22
2021-10-11T13:59:48Z
Duplicate of #51
14.8713 SUSHI - $185.89
0xsanson
In ConcentratedLiquidityPoolManager, an user can claimReward of a subscribed position.
In order to compute the correct amount, secondsUnclaimed needs to be calculated, but it's implemented incorrectly:
uint256 secondsUnclaimed = (maxTime - incentive.startTime) << (128 - incentive.secondsClaimed);
This line should be: uint256 secondsUnclaimed = ((maxTime - incentive.startTime) << 128) - incentive.secondsClaimed;.
https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol#L93 https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol#L110
editor
Correct the computations.
#0 - sarangparikh22
2021-10-11T14:00:39Z
Duplicate of #41 Disagree with severity, should be 2.
#1 - alcueca
2021-11-12T10:38:49Z
Functionality is unavailable, but assets are not lost. Severity 2.
16.9997 SUSHI - $212.50
0xsanson
In ConcentratedLiquidityPoolManager, function addIncentive may need the following checks to avoid users' mistakes:
require(incentive.owner != address(0));, otherwise passing a zero address would make reclaiming the incentive impossible.
require(incentive.secondsClaimed == 0);, it wouldn't make sense to start otherwise.
editor
Consider adding the aforementioned requirements.
#0 - sarangparikh22
2021-10-12T10:05:17Z
Duplicate of #4
#1 - alcueca
2021-11-12T10:49:41Z
Choosing this as the main, as it includes a wider recommendation that doesn't significantly impact code clarity or gas consumption.
16.9997 SUSHI - $212.50
0xsanson
Function subscribe in ConcentratedLiquidityPoolManager lets anyone call it for another user's position.
This is probably unintentional, since in claimReward the check require(ownerOf[positionId] == msg.sender, "OWNER") is present.
The issue is that an user gets subscribed to multiple spam incentives that, when listed on UI, become annoying.
editor
Consider adding a require(ownerOf[positionId] == msg.sender, "OWNER").
#0 - sarangparikh22
2021-10-12T10:01:34Z
Duplicate of #69 It's an non-critical issue, as it doesn't effect the pool in any way. Disagree with severity, it should be 0.
#1 - alcueca
2021-11-12T10:20:22Z
Assets not at risk, but function incorrect as to spec. You can't argue that this is an intended behaviour. Severity 1 is correct.
#2 - sarangparikh22
2021-11-16T21:43:28Z
@alcueca During the time of the audit, we were still finishing the spec, at that time we had this subscribe function open on a recommendation by another team member, however after internal discussion we decided it to make it only callable by the respective owner of the position only. This makes this issue valid.
🌟 Selected for report: 0xsanson
57.1429 SUSHI - $714.29
0xsanson
Function addIncentive and reclaimIncentive in ConcentratedLiquidityPoolManager can be external instead of public to save gas.
https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol#L36 https://github.com/sushiswap/trident/blob/c405f3402a1ed336244053f8186742d2da5975e9/contracts/pool/concentrated/ConcentratedLiquidityPoolManager.sol#L49
editor
#0 - sarangparikh22
2021-10-12T10:24:35Z
The above changes should have no effect on gas in this case, can you give me exact gas saving.
#1 - alcueca
2021-11-12T11:09:21Z
@sarangparikh22, why would there be no gas savings?
These are public functions, transactional so they will always use gas. As far as I know the parameters will be copied from calldata to memory, costing gas.
#2 - sarangparikh22
2021-11-17T00:09:23Z
@alcueca The functioning of the current function has changed in our current code, I ran my test with those updated code, which uses the parameter and updates it, which is not possible to do with calldata. Since, this is for an older commit, it's a valid gas saving.
#3 - alcueca
2021-11-18T06:54:54Z
Thank you, the wardens can only work on the submitted commit, and the issues are judged there. The issue is therefore valid.