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: 6/7
Findings: 7
Award: $6,904.79
🌟 Selected for report: 6
🚀 Solo Findings: 0
49.5711 SUSHI - $619.64
pauliax
Should be incentiveId, not positionId here: Incentive memory incentive = incentives[pool][positionId];
Incentive memory incentive = incentives[pool][incentiveId];
#0 - sarangparikh22
2021-10-12T10:06:20Z
Duplicate of #39
49.5711 SUSHI - $619.64
pauliax
function reclaimIncentive can be invoked more than once for the same incentiveId. If there were multiple incentives for the same token it would allow to drain these tokens by repeatedly calling reclaimIncentive.
Simple mitigation is to simply add the amount to the rewardsUnclaimed or add an extra boolean flag 'reclaimed' to the Incentive struct but that would incur more gas.
#0 - sarangparikh22
2021-10-11T14:01:41Z
Duplicate of #37
68.8487 SUSHI - $860.61
pauliax
function burn should subtract amount0 and amount1, not only fees from reserve0 and reserve1 here as whole amounts are withdrawn: reserve0 -= uint128(amount0fees); reserve1 -= uint128(amount1fees);
reserve0 -= uint128(amount0); reserve1 -= uint128(amount1);
#0 - sarangparikh22
2021-10-12T10:25:45Z
Duplicate of #51
50.999 SUSHI - $637.49
pauliax
function addIncentive does not verify secondsClaimed so any arbitrary value can be set initially by the creator and it may break calculations.
Consider either requiring that incentive.secondsClaimed is 0 or manually resetting it upon creation.
#0 - sarangparikh22
2021-10-12T10:10:33Z
Duplicate of #42, Disagree with severity, it should be 1.
50.999 SUSHI - $637.49
pauliax
When the signature is not valid, ecrecover returns empty (0x0) address. There is a potential check against that: require( (recoveredAddress != address(0) && recoveredAddress == owner) || isApprovedForAll[owner][recoveredAddress], "INVALID_PERMIT_SIGNATURE" ); However, the parentheses are not in place here, as an empty address check does not impact the OR condition. To bypass this check an attacker only needs the owner to have his isApprovedForAll set to 0x0. Then he can maliciously pass an invalid signature and this OR branch will result in 'true' allowing this bad actor to set any address as the new operator: isApprovedForAll[owner][operator] = true;
Recommended solution: require( recoveredAddress != address(0) && (recoveredAddress == owner || isApprovedForAll[owner][recoveredAddress], "INVALID_PERMIT_SIGNATURE" ); Overall, I would like to recommend using a battle-tested ECDSA library for signature verifications: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol
#0 - sarangparikh22
2021-10-13T23:27:45Z
Duplicate of #44
#1 - alcueca
2021-11-12T10:29:30Z
Severity 2 since for the loss of assets the asset owner need to execute an unrelated, but uncommon, operation.
🌟 Selected for report: pauliax
37.7771 SUSHI - $472.21
pauliax
Consider introducing reasonable limits for the endTime and expiry of the incentives. If a user (accidentally) sets these values to a very high number, it may become impractical or even impossible to accrue rewards or reclaim this incentive.
I strongly advise adding upper limits for these values, e.g. endTime max 3 years in the future or something like that, and respectively for the expiry.
🌟 Selected for report: pauliax
37.7771 SUSHI - $472.21
pauliax
ConcentratedLiquidityPoolManager may not work with weird ERC20s, e.g. deflationary tokens that could break the calculations if you do not verify how many tokens were actually transferred (the actual amount transferred is balanceOf this token before and after).
Consider supporting deflationary / rebasing / etc tokens by extra checking the balances before/after or strictly inform your users not to use such tokens if they don't want to lose them.
#0 - alcueca
2021-11-12T11:10:01Z
Issue is valid.
#1 - sarangparikh22
2021-11-16T21:52:46Z
Yes, the issue is valid, we will more clearly describe what tokens we support. I mistakenly marked it as sponsor disputed, my apologies. @alcueca
🌟 Selected for report: pauliax
37.7771 SUSHI - $472.21
pauliax
I think the conditions should be inclusive '<=' here: require(position.liquidity < MAX_TICK_LIQUIDITY, "MAX_TICK_LIQUIDITY"); require(incentive.endTime + 5 weeks < incentive.expiry, "END_PAST_BUFFER"); and here the check should not be inclusive as the incentive ids start from 0 and thus incentiveCount is always bigger by 1: require(incentiveId <= incentiveCount[pool], "NOT_INCENTIVE");
Review and update these conditions and be careful with such boundary checks (Robert Leshner cries in the corner).
#0 - sarangparikh22
2021-10-12T10:19:32Z
Haha! Robert lol, however, the above checks doesn't break anything, the incentive is protected by the time check below it. It is non-critical issues.
#1 - alcueca
2021-11-12T11:11:46Z
Let's be considerate of Robert's plight, and keep it as Low Risk knowing what happened.
#2 - sarangparikh22
2021-11-16T22:55:26Z
@alcueca haha, agreed!
16.9997 SUSHI - $212.50
pauliax
function subscribe does not verify the owner of the position so anyone can re-stake any positions to different incentives. This doesn't create any harm for the user as they can stake one position into as many incentives as they want but still I think this should be left for the user's responsibility.
Consider adding auth to function subscribe to verify the sender.
#0 - alcueca
2021-11-12T10:20:56Z
Chose #77 as best description
🌟 Selected for report: pauliax
37.7771 SUSHI - $472.21
pauliax
function _burn in TridentNFT does not decrement totalSupply. Function _mint increases it upon minting: uint256 tokenId = totalSupply++; so I think it makes sense to decrease it upon burning.
totalSupply--;
#0 - sarangparikh22
2021-10-13T20:51:14Z
We don't burn the NFT and decrease the totalSupply as that could lead to problems where two positions get the same ID as we derive it from the totalSupply. To keep the totalSupply ever increasing, we don't decrease the totalSupply when burning.
#1 - alcueca
2021-11-12T11:13:32Z
Function (totalSupply) incorrect as to spec. Are you guys sure you want to do this? Why not letting totalSupply do its job and having a variable for the next id?
#2 - sarangparikh22
2021-11-16T21:56:27Z
@alcueca We have previously discussed this internally and decided upon that we would proceed in this direction, as it saves tracking and update for one more extra variable. Though, I agree it deviates from the spec, and we would better document it.
🌟 Selected for report: pauliax
57.1429 SUSHI - $714.29
pauliax
contract ConcentratedLiquidityPosition has a state variable 'wETH' but it is not being used in any meaningful way. So you can remove it to save some gas.
Remove useless state variables.
🌟 Selected for report: pauliax
57.1429 SUSHI - $714.29
pauliax
There is an unused import: import "../../interfaces/ITridentRouter.sol"; in ConcentratedLiquidityPosition. It will increase the size of deployment with no real benefit.
Consider removing this unused import to save some gas.