Platform: Code4rena
Start Date: 31/01/2023
Pot Size: $36,500 CANTO
Total HM: 5
Participants: 38
Period: 3 days
Judge: berndartmueller
Total Solo HM: 2
Id: 212
League: ETH
Rank: 31/38
Findings: 1
Award: $44.97
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: HardlyDifficult
Also found by: 0xAgro, 0xSmartContract, Aymen0909, DevABDee, JC, Matin, Rolezn, SleepingBugs, adriro, brevis, btk, chaduke, d3e4, enckrish, hihen, joestakey, libratus, merlin, nicobevi, rotcivegaf, shark, sorrynotsorry
149.2473 CANTO - $44.97
| Total Low issues |
|---|
| Risk | Issues Details | Number |
|---|---|---|
| [L-01] | Solmate's SafeTransferLib doesn't check whether the ERC20 contract exists | 2 |
| [L-02] | Loss of precision due to rounding | 1 |
| [L-03] | Address(0) checks | 2 |
| [L-04] | Using >/>= without specifying an upper bound is unsafe | 3 |
| Total Non-Critical issues |
|---|
| Risk | Issues Details | Number |
|---|---|---|
| [NC-01] | Use a more recent version of solidity | All Contracts |
| [NC-02] | Constants in comparisons should appear on the left side | 6 |
| [NC-03] | Using vulnerable dependency of solmate | 1 |
| [NC-04] | Use bytes.concat() and string.concat() | 2 |
| [NC-05] | Non-usage of specific imports | 8 |
| [NC-06] | Include @return parameters in NatSpec comments | 8 |
| [NC-07] | Function writing does not comply with the Solidity Style Guide | All Contracts |
| [NC-08] | Lines are too long | 5 |
| [NC-09] | Follow Solidity standard naming conventions | All Contracts |
| [NC-10] | Add NatSpec comment to mapping | 4 |
| [NC-11] | Consider using delete rather than assigning zero to clear values | 1 |
| [NC-12] | Use SMTChecker | |
| [NC-13] | Use scientific notation rather than exponentiation | 1 |
Solmate'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.
Ref: https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.sol#L9
/// @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 in functions or use Openzeppelin safeERC20 instead.
Loss of precision due to the nature of arithmetics and rounding errors.
uint256 cidFee = (subprotocolFee * CID_FEE_BPS) / 10_000;
Add scalars so roundings are negligible.
Address(0) checksCheck of address(0) to protect the code from (0x0000000000000000000000000000000000000000) address problem just in case. This is best practice or instead of suggesting that they verify _address != address(0), you could add some good NatSpec comments explaining what is valid and what is invalid and what are the implications of accidentally using an invalid address.
constructor( string memory _name, string memory _symbol, string memory _baseURI, address _cidFeeWallet, address _noteContract, address _subprotocolRegistry ) ERC721(_name, _symbol) { baseURI = _baseURI; cidFeeWallet = _cidFeeWallet; note = ERC20(_noteContract); subprotocolRegistry = SubprotocolRegistry(_subprotocolRegistry); }
Add checks for address(0) when assigning values to address state variables.
>/>= without specifying an upper bound is unsafeThere will be breaking changes in future versions of solidity, and at that point your code will no longer be compatable. While you may have the specific version to use in a configuration file, others that include your source files may not.
pragma solidity >=0.8.0;
Use a fixed version of solidity.
For security, it is best practice to use the latest Solidity version.
pragma solidity >=0.8.0;
Old version of Solidity is used (0.8.0), newer version can be used (0.8.17).
Constants in comparisons should appear on the left side, doing so will prevent typo bug.
if (_nftIDToAdd == 0) revert NFTIDZeroDisallowedForSubprotocols();
if (0 == _nftIDToAdd) revert NFTIDZeroDisallowedForSubprotocols();
The package.json configuration file says that the project is using 6.2.0 of solmate which has a not last update version.
{ "name": "@rari-capital/solmate", "license": "AGPL-3.0-only", "version": "6.2.0", "description": "Modern, opinionated and gas optimized building blocks for smart contract development.", "files": [ "src/**/*.sol" ],
Use solmate latest version 6.7.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 (note that you must use a solidity version of at least 0.8.12).
Using import declarations of the form import {<identifier_name>} from "some/file.sol" avoids polluting the symbol namespace making flattened files smaller, and speeds up compilation.
The Solidity docs recommend specifying imported symbols explicitly.
Ref: https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files
Use specific imports syntax per solidity docs recommendation.
@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.
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length.
Ref: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
Split the long lines when they reach the max length.
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.
mappingAdd NatSpec comments describing mapping keys and values.
mapping(address => uint256) private cidNFTs;
/// @dev address(owner) => uint256(token Id) mapping(address => uint256) private cidNFTs;
delete rather than assigning zero to clear valuesThe delete keyword more closely matches the semantics of what is being done, and draws more attention to the changing of state, which may lead to a more thorough audit of its associated logic.
activeData.positions[_nftIDToRemove] = 0;
delete activeData.positions[_nftIDToRemove];
The highest tier of smart contract behavior assurance is formal mathematical verification. All assertions that are made are guaranteed to be true across all inputs → The quality of your asserts is the quality of your verification.
Ref: https://twitter.com/0xOwenThurm/status/1614359896350425088?t=dbG9gHFigBX85Rv29lOjIQ&s=19
Use SMTChecker.
While the compiler knows to optimize away the exponentiation, it's still better coding practice to use idioms that do not require compiler optimization, if they exist.
uint256 public constant REGISTER_FEE = 100 * 10**18;
Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18).
#0 - c4-judge
2023-02-18T13:10:43Z
berndartmueller marked the issue as grade-b