Platform: Code4rena
Start Date: 25/01/2023
Pot Size: $90,500 USDC
Total HM: 3
Participants: 26
Period: 9 days
Judge: GalloDaSballo
Id: 209
League: ETH
Rank: 24/26
Findings: 1
Award: $122.82
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: berndartmueller
Also found by: 0xA5DF, 0xSmartContract, HollaDieWaldfee, IllIllI, SleepingBugs, btk, chaduke, fs0c, hansfriese, nalus, rbserver, zzzitron
122.8177 USDC - $122.82
| Total Low issues |
|---|
| Risk | Issues Details | Number |
|---|---|---|
| [L-01] | The protocol is using draft-EIP712.sol which is deprecated | 1 |
| [L-02] | Integer overflow by unsafe casting | 11 |
| [L-03] | Use safeMint instead of mint for ERC721 | 1 |
| [L-04] | Array lengths not checked | 4 |
| [L-05] | Critical changes should use two-step procedure | 1 |
| [L-06] | Loss of precision due to rounding | 4 |
| [L-07] | No storage gap for upgradeable contracts | 1 |
| [L-08] | Unhandled return values of transferFrom | 1 |
| [L-09] | Missing Event for initialize | 8 |
| [L-10] | admin can renounce while system is paused | 1 |
| Total Non-Critical issues |
|---|
| Risk | Issues Details | Number |
|---|---|---|
| [NC-01] | Lock pragmas to specific compiler version | All Contracts |
| [NC-02] | Contracts should have full test coverage | All Contracts |
| [NC-03] | Constants in comparisons should appear on the left side | 12 |
| [NC-04] | Address(0) checks | 4 |
| [NC-05] | Include @return parameters in NatSpec comments | 3 Contracts |
| [NC-06] | Function writing does not comply with the Solidity Style Guide | All Contracts |
| [NC-07] | Use bytes.concat() and string.concat() | 1 |
| [NC-08] | For functions, follow Solidity standard naming conventions | All Contracts |
| [NC-09] | Generate perfect code headers every time | All Contracts |
| [NC-10] | There is no need to cast a variable that is already an address, such as address(x) | 2 |
| [NC-11] | Consider using delete rather than assigning zero to clear values | 2 |
| [NC-12] | Constants should be defined rather than using magic numbers | 2 |
| [NC-13] | Need Fuzzing test | 1 Contracts |
| [NC-14] | Assembly Codes Specific – Should Have Comments | 6 |
| [NC-15] | Use SMTChecker | |
| [NC-16] | Add NatSpec comment to mapping | 9 |
| [NC-17] | Not using the named return variables anywhere in the function is confusing | All Contracts |
draft-EIP712.sol which is deprecatedWhile OpenZeppelin draft contracts are safe to use and have been audited, their 'draft' status means that the EIPs they're based on are not finalized, and thus there may be breaking changes in even minor releases. If a bug is found in this version of OpenZeppelin, and the version that you're forced to upgrade to has breaking changes in the new version, you'll encounter unnecessary delays in porting and testing replacement contracts. Ensure that you have extensive test coverage of this area so that differences can be automatically detected, and have a plan in place for how you would fully test a new version of these contracts if they do indeed change unexpectedly.
// EIP-712 is Final as of 2022-08-11. This file is deprecated.
Consider creating a forked version of the file rather than importing it from the package, and manually patch your fork as changes are made.
Keep in mind that the version of solidity used, despite being greater than 0.8, does not prevent integer overflows during casting, it only does so in mathematical operations.
It is necessary to safely convert between the different numeric types.
splitAmt = uint128((uint160(amount) * splitsWeight) / _TOTAL_SPLITS_WEIGHT);
Use openzeppelin safeCast.sol library to prevent unexpected overflows when casting from uint256.
safeMint instead of mint for ERC721Users could lost their NFTs if msg.sender is a contract address that does not support ERC721, the NFT can be frozen in the contract forever.
As per the documentation of EIP-721:
A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.
As per the documentation of ERC721.sol by Openzeppelin:
function mint(address to, UserMetadata[] calldata userMetadata) public whenNotPaused returns (uint256 tokenId) { tokenId = _registerTokenId(); _mint(to, tokenId); if (userMetadata.length > 0) dripsHub.emitUserMetadata(tokenId, userMetadata); }
Use _safeMint instead of mint to check received address support for ERC721 implementation.
If the length of the arrays are not required to be of the same length, user operations may not be fully executed.
function setDrips( IERC20 erc20, DripsReceiver[] calldata currReceivers, int128 balanceDelta, DripsReceiver[] calldata newReceivers, // slither-disable-next-line similar-names uint32 maxEndHint1, uint32 maxEndHint2, address transferTo ) public whenNotPaused returns (int128 realBalanceDelta) { // @audit array length if (balanceDelta > 0) { _transferFromCaller(erc20, uint128(balanceDelta)); } realBalanceDelta = dripsHub.setDrips( callerUserId(), erc20, currReceivers, balanceDelta, newReceivers, maxEndHint1, maxEndHint2 ); if (realBalanceDelta < 0) { // @audit check if transferTo is a valid address erc20.safeTransfer(transferTo, uint128(-realBalanceDelta)); } }
if (currReceivers != newReceivers) revert("Array length mismatch");
The protocol inherit openzeppelin ERC1967Upgrade.sol which does not use two-step procedure when changing the admin, and since the protocol rely heavily on the onlyAdmin() modifier (8 results in 1 files), Thus using two-step procedure is a best practice
in case of transferring the admin role to an invalid address.
function changeAdmin(address newAdmin) public onlyAdmin { _changeAdmin(newAdmin); }
Consider adding two step procedure on the critical functions where the first is announcing a pendingAdmin and the new address should then claim the admin role.
Loss of precision due to the nature of arithmetics and rounding errors.
splitAmt = uint128((uint160(amount) * splitsWeight) / _TOTAL_SPLITS_WEIGHT);
For upgradeable contracts, inheriting contracts may introduce new variables. In order to be able to add new variables to the upgradeable contract without causing storage collisions, a storage gap should be added to the upgradeable contract.
See this for a description of this storage variable.
Consider adding a storage gap at the end of the upgradeable contract:
/** * @dev This empty reserved space is put in place to allow future versions to add new * variables without shifting down storage in the inheritance chain. * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps */ uint256[50] private __gap;
transferFromERC20 implementations are not always consistent. Some implementations of transferFrom could return false on failure instead of reverting. It is safer to check the return value and revert on false to prevent these failures.
This is a known issue with solmate and Opanzeppelin ERC20 libraries, For example, Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens, hence Tether (USDT) transferFrom() function do not return boolean as the specification requires, and instead have no return value.
function transferFrom(address from, address to, uint256 tokenId) public override whenNotPaused { super.transferFrom(from, to, tokenId); }
Check the return value and revert() on false.
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
admin can renounce while system is pausedThe contract admin is not prevented from renouncing the role/ownership while the contract is paused, which would cause any user assets stored in the protocol, to be locked indefinitely.
function pause() public onlyAdminOrPauser whenNotPaused { _managedStorage().isPaused = true; emit Paused(msg.sender); }
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.
pragma solidity ^0.8.17;
Ethereum Smart Contract Best Practices: Lock pragmas to specific compiler version.
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?: 90
Line coverage percentage should be 100%.
Constants in comparisons should appear on the left side, doing so will prevent typo bug.
if (start == 0) { start = updateTime; }
if (0 == start) { start = updateTime; }
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(DripsHub _dripsHub, address forwarder, uint32 _driverId) ERC2771Context(forwarder) { dripsHub = _dripsHub; driverId = _driverId; }
Add a check for address(0).
@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.
bytes.concat() and string.concat()bytes.concat() vs abi.encodePacked(<bytes>,<bytes>)string.concat() vs abi.encodePacked(<string>,<string>)return Address.functionCallWithValue(to, abi.encodePacked(data, sender), value);
Use bytes.concat() and string.concat() instead.
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.
I recommend using header for Solidity code layout and readability.
/*////////////////////////////////////////////////////////////// TESTING 123 //////////////////////////////////////////////////////////////*/
address(x)There is no need to cast a variable that is already an address, such as address(x), x is also address.
if (erc20.allowance(address(this), address(dripsHub)) == 0) { erc20.safeApprove(address(dripsHub), type(uint256).max); }
if (erc20.allowance(address(this), dripsHub) == 0) { erc20.safeApprove(dripsHub, type(uint256).max); }
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.
balance.splittable = 0;
delete balance.splittable;
uint256 idxMid = (idx + idxCap) / 2;
uint256 end = (enoughEnd + notEnoughEnd) / 2;
uint8 private constant TWO = 2;
In total 1 contract, 6 unchecked{} are used, the functions used are critical. For this reason, there must be fuzzing tests in the tests of the contract. Not seen in tests.
Use should fuzzing test like Echidna. 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
Since this is a low level language that is more difficult to parse by readers, include extensive documentation, comments on the rationale behind its use, clearly explaining what each assembly instruction does.
This will make it easier for users to trust the code, for reviewers to validate the code, and for developers to build on or update the code.
Note that using Aseembly removes several important security features of Solidity, which can make the code more insecure and more error-prone.
Include NatSpec in assembly codes.
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
mappingAdd NatSpec comments describing mapping keys and values.
mapping(address => uint256) public nonce;
/// @dev address(user) => uint256(nonce) mapping(address => uint256) public nonce;
Not using the named return variables anywhere in the function is confusing.
function isAuthorized(address sender, address user) public view returns (bool authorized) { return _authorized[sender].contains(user); }
Consider changing the variable to be an unnamed one.
function isAuthorized(address sender, address user) public view returns (bool) { return _authorized[sender].contains(user); }
#0 - GalloDaSballo
2023-02-14T20:23:25Z
[L-01] The protocol is using draft-EIP712.sol which is deprecated 1 R
[L-02] Integer overflow by unsafe casting 11 Disputing for lack of proof
[L-03] Use safeMint instead of mint for ERC721 1 L
[L-04] Array lengths not checked 4 R
[L-05] Critical changes should use two-step procedure 1 NC
[L-06] Loss of precision due to rounding 4 Disputing due to lack of proof
[L-07] No storage gap for upgradeable contracts 1 R
[L-08] Unhandled return values of transferFrom 1 INVALID - 3
[L-09] Missing Event for initialize 8 NC
[L-10] admin can renounce while system is paused 1 NC
[NC-01] Lock pragmas to specific compiler version All Contracts NC
[NC-02] Contracts should have full test coverage All Contracts Disputing
[NC-03] Constants in comparisons should appear on the left side 12 NC
[NC-04] Address(0) checks 4 L
[NC-05] Include @return parameters in NatSpec comments 3 Contracts NC
[NC-06] Function writing does not comply with the Solidity Style Guide All Contracts NC
[NC-07] Use bytes.concat() and string.concat() 1 NC
[NC-08] For functions, follow Solidity standard naming conventions All Contracts NC
[NC-09] Generate perfect code headers every time All Contracts Ignoring
[NC-10] There is no need to cast a variable that is already an address, such as address(x) 2 Invalid
[NC-11] Consider using delete rather than assigning zero to clear values 2 Invalid
[NC-12] Constants should be defined rather than using magic numbers 2 R
[NC-13] Need Fuzzing test 1 Contracts Disputing
[NC-14] Assembly Codes Specific – Should Have Comments 6 Disputing
[NC-15] Use SMTChecker Same
[NC-16] Add NatSpec comment to mapping 9 NC
[NC-17] Not using the named return variables anywhere in the function is confusing R
#1 - GalloDaSballo
2023-02-23T16:16:52Z
2L 5R 10NC - 3
#2 - c4-judge
2023-02-24T10:55:36Z
GalloDaSballo marked the issue as grade-b