Platform: Code4rena
Start Date: 07/09/2022
Pot Size: $20,000 CANTO
Total HM: 7
Participants: 65
Period: 1 day
Judge: 0xean
Total Solo HM: 3
Id: 159
League: ETH
Rank: 19/65
Findings: 2
Award: $146.62
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: hickuphh3
Also found by: 0xNazgul, 0xSky, CertoraInc, Deivitto, Jeiwan, SinceJuly, hansfriese, linmiaomiao, rbserver
664.9949 CANTO - $107.40
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L582 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L549-L593
Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision.
In general, this is a problem due to precision. In this case, it also affects money/price/tokens, what makes me suggest medium severity.
Slither + manual analysis
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L582
token0TVL = assetReserves[i] * (prices[i] / decimals)
LpPrice = LpPricesCumulative / 8 LpPrice * getPriceNote(address(wcanto),false) / 1e18
Reorder the operations. For more info: https://github.com/crytic/slither/wiki/Detector-Documentation#divide-before-multiply
#0 - nivasan1
2022-09-10T19:05:26Z
duplicate #41
🌟 Selected for report: lukris02
Also found by: 0x040, 0x1f8b, 0x52, 0xA5DF, 0xNazgul, 0xSky, Bnke0x0, Bronicle, CertoraInc, Chom, CodingNameKiki, Deivitto, Diraco, Dravee, EthLedger, IgnacioB, JC, JansenC, Jeiwan, R2, RaymondFam, ReyAdmirado, Rolezn, SinceJuly, TomJ, Tomo, Yiko, a12jmx, ajtra, ak1, codexploder, cryptphi, csanuragjain, erictee, fatherOfBlocks, gogo, hake, hansfriese, hickuphh3, ignacio, ontofractal, oyc_109, p_crypt0, pashov, peritoflores, rajatbeladiya, rbserver, rokinot, rvierdiiev, tnevler
242.8216 CANTO - $39.22
Division by 0 will revert code, however on some locations this isn’t checked.
Navigate to the following contracts,
granularity parameter is used with no validation of being > 0, if the variable is equal to zero, div by zero will occur.
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-core.sol#L234
return (reserveAverageCumulative0 / granularity, reserveAverageCumulative1 / granularity);
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-core.sol#L268 return (totalSupplyCumulativeAvg / granularity);
Different calls to erc20(address).decimals() are used with no validation of being > 0, therefore, in the edge case of an erc20 that overrides decimals() to be 0, decimals variable will be equal to zero therefore div by zero will occur.
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L533 return price * 1e18 / decimals; //return the scaled price
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L545 return price * 1e18 / decimals; // divide by decimals now to maintain precision
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L582 uint token0TVL = assetReserves[i] * (prices[i] / decimals);
supply[i] is equal to 0, div by zero will occur.
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L584
LpPricesCumulative += (token0TVL + token1TVL) * 1e18 / supply[i];
Recommend making sure division by 0 won’t occur by checking the variables beforehand and handling this edge cases.
block.timestamp used as time proxyRisk of using block.timestamp for time should be considered.
block.timestamp is not an ideal proxy for time because of issues with synchronization, miner manipulation and changing block times.
This kind of issue may affect the code allowing or reverting the code before the expected deadline, modifying the normal functioning or reverting sometimes.
SWC ID: 116
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-core.sol#L138 uint blockTimestamp = block.timestamp;
Consider using an oracle for precision Consider the risk of using block.timestamp as time proxy and evaluate if block numbers can be used as an approximation for the application logic. Both have risks that need to be factored in.
Events are important for critical parameter change and some paths of the code where ether/tokens are involved.
No events are emitted in BaseV1-periphery.sol, however, functions with ether involved change of permissions (for example: admin)
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol
Add events for better monitoring of the code
Magic numbers are hardcoded numbers used in the code which are ambiguous to their intended purpose. These should be replaced with constants to make code more readable and maintainable.
Values are hardcoded and would be more readable and maintainable if declared as a constant
In several locations in the code numbers like 1e18, 8, 500, 1e18 are used. It quite easy to make a mistake somewhere, also when comparing values or missing a 0 what can lead to some bad behaviors.]
1e187 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L499-L508 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L545 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L584 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L592
8 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L532 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L544 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L550 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L558-L577
Replace magic hardcoded numbers with declared constants. Define constants for the numbers used throughout the code. Comment what these numbers are intended for
Missing Natspec and regular comments affect readability and maintainability of a codebase.
Contracts has a few inline comments but mostly it has lack of comments and full lack of any natspec property
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-core.sol
Add @param descriptors Add @return descriptors Add Natspec comments. Add inline comments. Add comments for what the contract does
Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file
totalSupplyCumulativeLast += _totalSupply * timeElapsed; //update totalSupply after each change in LP Token supply
But following the style of the other comments would be:
totalSupplyCumulativeLast += _totalSupply * timeElapsed; // update totalSupply after each change in LP Token supply
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-core.sol#L143 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-core.sol#L230 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-core.sol#L265 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L486 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L489 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L497 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L503 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L507 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L524 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L533 https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-periphery.sol#L559
Some lines has the start of a comment but without any kind of comment
https://github.com/code-423n4/2022-09-canto/blob/a6406d21b5c4359ec3fd2139a2959d3ecbf35a54/src/Swap/BaseV1-core.sol#L231 reserveAverageCumulative1 += _reserves1[i]; //
Add the expected comment or remove in case there is nothing to say.
Long lines should be wrapped to conform with Solidity Style guidelines.
Lines that exceed the 79 (or 99) character length suggested by the Solidity Style guidelines. Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
Reduce line length to less than 99 at least to improve maintainability and readability of the code