Platform: Code4rena
Start Date: 26/01/2023
Pot Size: $60,500 USDC
Total HM: 7
Participants: 31
Period: 6 days
Judge: berndartmueller
Total Solo HM: 3
Id: 207
League: ETH
Rank: 11/31
Findings: 1
Award: $997.11
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: CodingNameKiki
Also found by: 0xAgro, 0xSmartContract, IllIllI, Rolezn, SleepingBugs, btk, chrisdior4, matrix_0wl
997.1116 USDC - $997.11
| Total Low issues |
|---|
| Risk | Issues Details | Number |
|---|---|---|
| [L-01] | Solmate's SafeTransferLib.sol doesn't check whether the ERC20 contract exists | 4 |
| [L-02] | Loss of precision due to rounding | 6 |
| [L-03] | Missing Event for initialize | 5 |
| Total Non-Critical issues |
|---|
| Risk | Issues Details | Number |
|---|---|---|
| [NC-01] | Lock pragmas to specific compiler version | All Contracts |
| [NC-02] | Use a more recent version of solidity | All Contracts |
| [NC-03] | Constants in comparisons should appear on the left side | 27 |
| [NC-04] | Use bytes.concat() and string.concat() | 3 |
| [NC-05] | Include @return parameters in NatSpec comments | 29 |
| [NC-06] | Function writing does not comply with the Solidity Style Guide | 3 Contracts |
| [NC-07] | NatSpec comments should be increased in contracts | All Contracts |
| [NC-08] | Contracts should have full test coverage | All Contracts |
| [NC-09] | Add NatSpec comment to mapping | 4 |
| [NC-10] | Remove Unused Codes | 1 |
| [NC-11] | For functions, follow Solidity standard naming conventions | All Contracts |
| [NC-12] | Not using the named return variables anywhere in the function is confusing | 3 |
| [NC-13] | Constants should be defined rather than using magic numbers | 4 |
| [NC-14] | Need Fuzzing test | All Contracts |
SafeTransferLib.sol doesn't check whether the ERC20 contract existsSolmate's SafeTransferLib.sol, which is often used to interact with non-compliant/unsafe ERC20 tokens, does not check whether the ERC20 contract exists. The following code will not revert in case the token doesn't exist.
/// @dev Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.
Add a contract exist check to your SafeTransferLib.sol library.
Loss of precision due to the nature of arithmetics and rounding errors.
return FullMath.mulDiv(liquidity, 2 * upperBound, 1e18) / token1Scale;
Events help non-contract tools to track changes, and events prevent users from being surprised by changes Issuing event-emit during initialization is a detail that many projects skip.
Add Event-Emit.
Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile locally.
Ethereum Smart Contract Best Practices: Lock pragmas to specific compiler version.
For security, it is best practice to use the latest Solidity version.
pragma solidity ^0.8.4;
Old version of Solidity is used (^0.8.4), newer version can be used (0.8.17).
Constants in comparisons should appear on the left side, doing so will prevent typo bug.
if (totalLiquidity == 0) return 0;
if (0 == totalLiquidity) return 0;
bytes.concat() and string.concat()bytes.concat() vs abi.encodePacked(<bytes>,<bytes>)string.concat() vs abi.encodePacked(<string>,<string>)Use bytes.concat() and string.concat() instead.
@return parameters in NatSpec commentsIf Return parameters are declared, you must prefix them with @return. Some code analysis programs do analysis by reading NatSpec details, if they can't see the @return tag, they do incomplete analysis.
Include the @return argument in the NatSpec comments.
Solidity Style GuideOrdering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.
Functions should be grouped according to their visibility and ordered:
constructor()receive()fallback()external / public / internal / privateview / pureFollow Solidity Style Guide.
It is recommended that Solidity contracts are fully annotated using NatSpec, it is clearly stated in the Solidity official documentation.
In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.
Some code analysis programs do analysis by reading NatSpec details, if they can't see the tags (@param, @dev, @return), they do incomplete analysis.
Include NatSpec comments in the codebase.
While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
- What is the overall line coverage percentage provided by your tests?: 0
Line coverage percentage should be 100%.
mappingAdd NatSpec comments describing mapping keys and values.
/// @dev uint(timestamp) => uint(token Id) mapping(uint => uint) public timestampForTokenId;
The below error is unused code:
error FailedApprove();
I recommend removing unused codes from the codebase.
The protocol don't follow solidity standard naming convention.
Ref: https://docs.soliditylang.org/en/v0.8.17/style-guide.html#naming-conventions
Follow solidity standard naming convention.
Not using the named return variables anywhere in the function is confusing.
function utilizationRate(uint256 borrowedLiquidity, uint256 totalLiquidity) private pure returns (uint256 rate) { if (totalLiquidity == 0) return 0; return (borrowedLiquidity * 1e18) / totalLiquidity; }
Consider changing the variable to be an unnamed one.
function utilizationRate(uint256 borrowedLiquidity, uint256 totalLiquidity) private pure returns (uint256) { if (totalLiquidity == 0) return 0; return (borrowedLiquidity * 1e18) / totalLiquidity; }
uint256 amountInWithFee = amountIn * 997;
uint256 denominator = (reserveIn * 1000) + amountInWithFee;
uint256 numerator = reserveIn * amountOut * 1000;
uint256 denominator = (reserveOut - amountOut) * 997;
As Alberto Cuesta Canada said: Fuzzing is not easy, the tools are rough, and the math is hard, but it is worth it. Fuzzing gives me a level of confidence in my smart contracts that I didn’t have before. Relying just on unit testing anymore and poking around in a testnet seems reckless now.
Ref: https://medium.com/coinmonks/smart-contract-fuzzing-d9b88e0b0a05
Use fuzzing test like Echidna.
#0 - c4-judge
2023-02-16T11:47:16Z
berndartmueller marked the issue as grade-a