Drips Protocol contest - 0xSmartContract's results

Summary

Low Risk Issues List

Total 11 issues

Non-Critical Issues List

Total 13 issues

Suggestions

[L-01] collect() function allows re-entrancy from hookable tokens

https://github.com/code-423n4/2023-01-drips/blob/main/src/AddressDriver.sol#L60-L63

Vulnerability details

Some token standards, such as ERC777, allow a callback to the source of the funds before the balances are updated in transfer(). This callback could be used to re-enter the protocol while already holding the minted tranche tokens and at a point where the system accounting reflects a receipt of funds that has not yet occurred.

Even if an attacker cannot interact with this function due to any restrictions, they may still interact with the rest of the protocol, so care should be taken with any externall calls that may be affected.

src/AddressDriver.sol:
  59      /// @return amt The collected amount
  60:     function collect(IERC20 erc20, address transferTo) public whenNotPaused returns (uint128 amt) {
  61:         amt = dripsHub.collect(callerUserId(), erc20);
  62:         erc20.safeTransfer(transferTo, amt);
  63:     }

Recommendation;

Add Re-Entrancy Guard to the function

[L-02] Danger "while" loop

Impact

Drips.sol#L716-L729 has a while loop on this line . When using while loops, the Solidity compiler is aware that a condition needs to be checked first, before executing the code inside the loop.

Firstly, the conditional part is evaluated

a) if the condition evaluates to true , the instructions inside the loop (defined inside the brackets { ... }) get executed.

b) once the code reaches the end of the loop body (= the closing curly brace } , it will jump back up to the conditional part).

c) the code inside the while loop body will keep executing over and over again until the conditional part turns false.

For loops are more associated with iterations. While loop instead are more associated with repetitive tasks. Such tasks could potentially be infinite if the while loop body never affects the condition initially checked. Therefore, they should be used with care.

Proof of Concept

At very small intervals, a large number of loops can be accessed by someone attempting a DOS attack.

src/Drips.sol:
  715  
  716:             while (true) {
  717:                 uint256 end = (enoughEnd + notEnoughEnd) / 2;
  718:                 if (end == enoughEnd) {
  719:                     return uint32(end);
  720:                 }
  721:                 if (_isBalanceEnough(balance, configs, configsLen, end)) {
  722:                     enoughEnd = end;
  723:                 } else {
  724:                     notEnoughEnd = end;
  725:                 }
  726:             }
  727:             // slither-disable-end incorrect-equality,timestamp
  728:         }
  729      }

Tools Used

Manual code review

Recommended Mitigation Steps

Set a reasonable while loop interval

[L-03] Missing Event for initialize

Context:

5 result

src/AddressDriver.sol:
  29      /// @param _driverId The driver ID to use when calling DripsHub.
  30:     constructor(DripsHub _dripsHub, address forwarder, uint32 _driverId)
  31:         ERC2771Context(forwarder)
  32:     {
  33:         dripsHub = _dripsHub;
  34:         driverId = _driverId;
  35:     }


src/Drips.sol:
  218      /// @param dripsStorageSlot The storage slot to holding a single `DripsStorage` structure.
  219:     constructor(uint32 cycleSecs, bytes32 dripsStorageSlot) {
  220:         require(cycleSecs > 1, "Cycle length too low");
  221:         _cycleSecs = cycleSecs;
  222:         _dripsStorageSlot = dripsStorageSlot;
  223:     }

src/NFTDriver.sol:
  31      /// @param _driverId The driver ID to use when calling DripsHub.
  32:     constructor(DripsHub _dripsHub, address forwarder, uint32 _driverId)
  33:         ERC2771Context(forwarder)
  34:         ERC721("DripsHub identity", "DHI")
  35:     {
  36:         dripsHub = _dripsHub;
  37:         driverId = _driverId;
  38:     }


src/Managed.sol:
  72      /// The contract instance can be used only as a call delegation target for a proxy.
  73:     constructor() {
  74:         _managedStorage().isPaused = true;
  75:     }

src/ImmutableSplitsDriver.sol:
  27      /// @param _driverId The driver ID to use when calling DripsHub.
  28:     constructor(DripsHub _dripsHub, uint32 _driverId) {
  29:         dripsHub = _dripsHub;
  30:         driverId = _driverId;
  31:         totalSplitsWeight = _dripsHub.TOTAL_SPLITS_WEIGHT();
  32:     }

Description: 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

Recommendation: Add Event-Emit

[L-04] Lack of control to assign 0 values in the value assignments of critical state variables in the constructor

src/Splits.sol:
  93      /// @param splitsStorageSlot The storage slot to holding a single `SplitsStorage` structure.
  94:     constructor(bytes32 splitsStorageSlot) {
  95:         _splitsStorageSlot = splitsStorageSlot;
  96:     }

[L-05] Project Upgrade and Stop Scenario should be

At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an " EMERGENCY STOP (CIRCUIT BREAKER) PATTERN ".

https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol

[L-06] Draft Openzeppelin Dependencies

The Caller.sol contract utilised draft-EIP712.sol, an OpenZeppelin contract. This contract is still a draft and are not considered ready for mainnet use. OpenZeppelin contracts may be considered draft contracts if they have not received adequate security auditing or are liable to change with future development.

src/Caller.sol:
  5: import {ECDSA, EIP712} from "openzeppelin-contracts/utils/cryptography/draft-EIP712.sol";

[L-07] Loss of precision due to rounding

src/Drips.sol:
  1119      /// @return cycle The cycle containing the timestamp.
  1120:     function _cycleOf(uint32 timestamp) private view returns (uint32 cycle) {
  1121:         unchecked {
  1122:             return timestamp / _cycleSecs + 1;
  1123:         }
  1124:     }

[L-08] Some events are missing msg.sender parameters

Only amounts are published in emits, whereas msg.sender must be specified in every transaction, a contract or web page listening to events cannot react to users, emit does not serve the purpose. Basically, this event cannot be used

src/Drips.sol:
  232      /// @return receivedAmt The received amount
  233:     function _receiveDrips(uint256 userId, uint256 assetId, uint32 maxCycles)
  234:         internal
  235:         returns (uint128 receivedAmt)
  236:     {
  237:         uint32 receivableCycles;
  238:         uint32 fromCycle;
  239:         uint32 toCycle;
  240:         int128 finalAmtPerCycle;
  241:         (receivedAmt, receivableCycles, fromCycle, toCycle, finalAmtPerCycle) =
  242:             _receiveDripsResult(userId, assetId, maxCycles);
  243:         if (fromCycle != toCycle) {
  244:             DripsState storage state = _dripsStorage().states[assetId][userId];
  245:             state.nextReceivableCycle = toCycle;
  246:             mapping(uint32 => AmtDelta) storage amtDeltas = state.amtDeltas;
  247:             for (uint32 cycle = fromCycle; cycle < toCycle; cycle++) {
  248:                 delete amtDeltas[cycle];
  249:             }
  250:             // The next cycle delta must be relative to the last received cycle, which got zeroed.
  251:             // In other words the next cycle delta must be an absolute value.
  252:             if (finalAmtPerCycle != 0) {
  253:                 amtDeltas[toCycle].thisCycle += finalAmtPerCycle;
  254:             }
- 255:         }  256:         emit ReceivedDrips(userId, assetId, receivedAmt, receivableCycles);
+ 255:         }  256:         emit ReceivedDrips(msg.sender, userId, assetId, receivedAmt, receivableCycles);

  257:     }

src/Drips.sol:
  341      /// @return amt The squeezed amount.
  342:     function _squeezeDrips(
  343:         uint256 userId,
  344:         uint256 assetId,
  345:         uint256 senderId,
  346:         bytes32 historyHash,
  347:         DripsHistory[] memory dripsHistory
  348:     ) internal returns (uint128 amt) {
  349:         uint256 squeezedNum;
  350:         uint256[] memory squeezedRevIdxs;
  351:         bytes32[] memory historyHashes;
  352:         uint256 currCycleConfigs;
  353:         (amt, squeezedNum, squeezedRevIdxs, historyHashes, currCycleConfigs) =
  354:             _squeezeDripsResult(userId, assetId, senderId, historyHash, dripsHistory);
  355:         bytes32[] memory squeezedHistoryHashes = new bytes32[](squeezedNum);
  356:         DripsState storage state = _dripsStorage().states[assetId][userId];
  357:         uint32[2 ** 32] storage nextSqueezed = state.nextSqueezed[senderId];
  358:         for (uint256 i = 0; i < squeezedNum; i++) {
  359:             // `squeezedRevIdxs` are sorted from the newest configuration to the oldest,
  360:             // but we need to consume them from the oldest to the newest.
  361:             uint256 revIdx = squeezedRevIdxs[squeezedNum - i - 1];
  362:             squeezedHistoryHashes[i] = historyHashes[historyHashes.length - revIdx];
  363:             nextSqueezed[currCycleConfigs - revIdx] = _currTimestamp();
  364:         }
  365:         uint32 cycleStart = _currCycleStart();
  366:         _addDeltaRange(state, cycleStart, cycleStart + 1, -int256(amt * _AMT_PER_SEC_MULTIPLIER));
- 367:         emit SqueezedDrips(userId, assetId, senderId, amt, squeezedHistoryHashes);
+ 367:         emit SqueezedDrips(msg.sender, userId, assetId, senderId, amt, squeezedHistoryHashes);
  368:     }
  369 

src/Drips.sol:
  609      /// @return realBalanceDelta The actually applied drips balance change.
  610:     function _setDrips(
                // codes...
- 666:                 emit DripsReceiverSeen(newDripsHash, receiver.userId, receiver.config);
+ 666:                  emit DripsReceiverSeen(msg.sender, newDripsHash, receiver.userId, receiver.config);
  667:             }
  668:         }

Recommended Mitigation Steps

add msg.sender parameter in event-emit

[L-09] Need Fuzzing test

Context:

6 results - 1 file

src/Drips.sol:
   685      ) private view returns (uint32 maxEnd) {
   686:         unchecked {
   687              (uint256[] memory configs, uint256 configsLen) = _buildConfigs(receivers);

   742      ) private view returns (bool isEnough) {
   743:         unchecked {
   744              uint256 spent = 0;

   773      {
   774:         unchecked {
   775              require(receivers.length <= _MAX_DRIPS_RECEIVERS, "Too many drips receivers");

  1040      ) private {
  1041:         unchecked {
  1042              // In order to set a delta on a specific timestamp it must be introduced in two cycles.

  1099          // per transaction and it needs to be optimized as much as possible.
  1100:         // As of Solidity 0.8.13, rewriting it in unchecked Solidity triples its gas cost.
  1101          uint256 cycleSecs = _cycleSecs;

  1120      function _cycleOf(uint32 timestamp) private view returns (uint32 cycle) {
  1121:         unchecked {
  1122              return timestamp / _cycleSecs + 1;

Description: In total 6 unchecked are used, the functions used are critical. For this reason, there must be fuzzing tests in the tests of the project. Not seen in tests.

Recommendation: 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.

https://medium.com/coinmonks/smart-contract-fuzzing-d9b88e0b0a05

[L-10] Using both mint and safeMint method at the same time is not the right way for security

For all battle-tested projects, either mint or safeMint patterns are preferred, this is a design decision; It is evaluated in terms of security concerns and gas optimization, but using both at the same time causes confusion, one of them should be chosen as best practice.

In case of using mint, if msg.sender is contract, a faulty contract may be sent without supporting NFT purchase.

src/NFTDriver.sol:
  62:     function mint(address to, UserMetadata[] calldata userMetadata)
  63:         public
  64:         whenNotPaused
  65:         returns (uint256 tokenId)
  66:     {
  67:         tokenId = _registerTokenId();
  68:         _mint(to, tokenId);
  69:         if (userMetadata.length > 0) dripsHub.emitUserMetadata(tokenId, userMetadata);
  70:     }
  71: 



  79:     function safeMint(address to, UserMetadata[] calldata userMetadata)
  80:         public
  81:         whenNotPaused
  82:         returns (uint256 tokenId)
  83:     {
  84:         tokenId = _registerTokenId();
  85:         _safeMint(to, tokenId);
  86:         if (userMetadata.length > 0) dripsHub.emitUserMetadata(tokenId, userMetadata);
  87:     }
  88 

Recommendation: Use only safeMint

[L-11] Cross-chain replay attacks are possible with  callSigned

https://github.com/code-423n4/2023-01-drips/blob/main/src/Caller.sol#L164-L183

If a user does a callSigned() using the wrong network, an attacker can replay the action on the correct chain, and steal the funds a-la the wintermute gnosis safe attack, where the attacker can create the same address that the user tried to, and steal the funds from there https://mirror.xyz/0xbuidlerdao.eth/lOE5VN-BHI0olGOXe27F0auviIuoSlnou_9t3XRJseY

There is no chain.id in the data

src/Caller.sol:
  174          uint256 currNonce = nonce[sender]++;
  175:         bytes32 executeHash = keccak256(
  176:             abi.encode(
  177:                 callSignedTypeHash, sender, to, keccak256(data), msg.value, currNonce, deadline
  178:             )
  179:         );

function callSigned(
        address sender,
        address to,
        bytes memory data,
        uint256 deadline,
        bytes32 r,
        bytes32 sv
    ) public payable returns (bytes memory returnData) {
        // slither-disable-next-line timestamp
        require(block.timestamp <= deadline, "Execution deadline expired");
        uint256 currNonce = nonce[sender]++;
        bytes32 executeHash = keccak256(
            abi.encode(
                callSignedTypeHash, sender, to, keccak256(data), msg.value, currNonce, deadline
            )
        );
        address signer = ECDSA.recover(_hashTypedDataV4(executeHash), r, sv);
        require(signer == sender, "Invalid signature");
        return _call(sender, to, data, msg.value);
    }

Similar issue from Harpieio; https://github.com/Harpieio/contracts/pull/4/commits/de24a50349ec014163180ba60b5305098f42eb14

p.s : I couldn't write a POC because I didn't have enough time, so I entred in as a QA

Recommended Mitigation Steps

Include the chain.id in keccak256

[N-01] Implement some type of version counter that will be incremented automatically for contract upgrades

As part of the upgradeability of Proxies , the contract can be upgraded multiple times, where it is a systematic approach to record the version of each upgrade.

src/Managed.sol:
  149  
  150:     /// @notice Authorizes the contract upgrade. See `UUPSUpgradeable` docs for more details.
  151:     function _authorizeUpgrade(address /* newImplementation */ ) internal view override onlyAdmin {
  152:         return;
  153:     }

I suggest implementing some kind of version counter that will be incremented automatically when you upgrade the contract.

Recommendation:

uint256 public authorizeUpgradeCounter;

function _authorizeUpgrade(address newImplementation) internal override onlyGuardian {
	authorizeUpgradeCounter+=1;
}


    }

[N-02] Insufficient coverage

Description: The test coverage rate of the project is ~80%. Testing all functions is best practice in terms of security criteria. Due to its capacity, test coverage is expected to be 100%.

| File                          | % Lines          | % Statements     | % Branches       | % Funcs          |
|-------------------------------|------------------|------------------|------------------|------------------|
| src/AddressDriver.sol         | 100.00% (16/16)  | 100.00% (16/16)  | 100.00% (6/6)    | 87.50% (7/8)     |
| src/Caller.sol                | 100.00% (22/22)  | 100.00% (30/30)  | 100.00% (10/10)  | 100.00% (8/8)    |
| src/Drips.sol                 | 89.01% (243/273) | 89.27% (283/317) | 66.98% (71/106)  | 83.33% (30/36)   |
| src/DripsHub.sol              | 100.00% (58/58)  | 100.00% (63/63)  | 100.00% (14/14)  | 100.00% (31/31)  |
| src/ImmutableSplitsDriver.sol | 100.00% (10/10)  | 100.00% (13/13)  | 75.00% (3/4)     | 100.00% (2/2)    |
| src/Managed.sol               | 94.12% (16/17)   | 94.12% (16/17)   | 100.00% (4/4)    | 100.00% (12/12)  |
| src/NFTDriver.sol             | 87.10% (27/31)   | 87.88% (29/33)   | 80.00% (8/10)    | 94.44% (17/18)   |
| src/Splits.sol                | 100.00% (64/64)  | 100.00% (71/71)  | 68.18% (15/22)   | 100.00% (13/13)  |
| Total                         | 81.58% (465/570) | 82.04% (530/646) | 65.20% (133/204) | 84.25% (123/146) |

[N-03] Function writing that does not comply with the Solidity Style Guide

Context: All Contracts

Description: Order of Functions; ordering 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.

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

Functions should be grouped according to their visibility and ordered:

constructor receive function (if exists) fallback function (if exists) external public internal private within a grouping, place the view and pure functions last

[N-04] Tokens accidentally sent to the contract cannot be recovered

Context; AddressDriver.sol

It can't be recovered if the tokens accidentally arrive at the contract address, which has happened to many popular projects, so I recommend adding a recovery code to your critical contracts.

Recommended Mitigation Steps

Add this code:

 /**
  * @notice Sends ERC20 tokens trapped in contract to external address
  * @dev Onlyowner is allowed to make this function call
  * @param account is the receiving address
  * @param externalToken is the token being sent
  * @param amount is the quantity being sent
  * @return boolean value indicating whether the operation succeeded.
  *
 */
  function rescueERC20(address account, address externalToken, uint256 amount) public onlyOwner returns (bool) {
    IERC20(externalToken).transfer(account, amount);
    return true;
  }
}

[N-05] Assembly Codes Specific – Should Have Comments

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.

12 results - 4 files

src/Drips.sol:
   820          uint256 val;
   821:         // slither-disable-next-line assembly
   822:         assembly ("memory-safe") {
   823              val := mload(add(32, add(configs, shl(5, idx))))

  1101          uint256 cycleSecs = _cycleSecs;
  1102:         // slither-disable-next-line assembly
  1103:         assembly {
  1104              let endedCycles := sub(div(end, cycleSecs), div(start, cycleSecs))

  1143          bytes32 slot = _dripsStorageSlot;
  1144:         // slither-disable-next-line assembly
  1145:         assembly {
  1146              dripsStorage.slot := slot

src/DripsHub.sol:
  622          bytes32 slot = _dripsHubStorageSlot;
  623:         // slither-disable-next-line assembly
  624:         assembly {
  625              storageRef.slot := slot

src/Managed.sol:
  143          bytes32 slot = _managedStorageSlot;
  144:         // slither-disable-next-line assembly
  145:         assembly {
  146              storageRef.slot := slot

src/Splits.sol:
  284          bytes32 slot = _splitsStorageSlot;
  285:         // slither-disable-next-line assembly
  286:         assembly {
  287              splitsStorage.slot := slot

[N-06] For functions, follow Solidity standard naming conventions (internal function style rule)

Context:

8 results
8 results

src/AddressDriver.sol:
  46:     function callerUserId() internal view returns (uint256 userId) {

src/Caller.sol:
  84:     bytes32 internal immutable callSignedTypeHash = keccak256(bytes(CALL_SIGNED_TYPE_NAME));

src/Drips.sol:
  60:     function create(uint32 dripId_, uint160 amtPerSec_, uint32 start_, uint32 duration_) internal
  72:     function dripId(DripsConfig config) internal pure returns (uint32) {
  77:     function amtPerSec(DripsConfig config) internal pure returns (uint160) {
  82:     function start(DripsConfig config) internal pure returns (uint32) {
  87:     function duration(DripsConfig config) internal pure returns (uint32) {
  93:     function lt(DripsConfig config, DripsConfig otherConfig) internal pure returns (bool) {

Description: The above codes don't follow Solidity's standard naming convention,

internal and private functions : the mixedCase format starting with an underscore (_mixedCase starting with an underscore)

[N-07] Floating pragma

Description: Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. https://swcregistry.io/docs/SWC-103

Floating Pragma List:

8 results - 8 files

src/AddressDriver.sol:
  1  // SPDX-License-Identifier: GPL-3.0-only
  2: pragma solidity ^0.8.17;
  3  

src/Caller.sol:
  1  // SPDX-License-Identifier: GPL-3.0-only
  2: pragma solidity ^0.8.17;
  3  

src/Drips.sol:
  1  // SPDX-License-Identifier: GPL-3.0-only
  2: pragma solidity ^0.8.17;
  3  

src/DripsHub.sol:
  1  // SPDX-License-Identifier: GPL-3.0-only
  2: pragma solidity ^0.8.17;
  3  

src/ImmutableSplitsDriver.sol:
  1  // SPDX-License-Identifier: GPL-3.0-only
  2: pragma solidity ^0.8.17;
  3  

src/Managed.sol:
  1  // SPDX-License-Identifier: GPL-3.0-only
  2: pragma solidity ^0.8.17;
  3  

src/NFTDriver.sol:
  1  // SPDX-License-Identifier: GPL-3.0-only
  2: pragma solidity ^0.8.17;
  3  

src/Splits.sol:
  1  // SPDX-License-Identifier: GPL-3.0-only
  2: pragma solidity ^0.8.17;

Recommendation: Lock the pragma version and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

[N-08] Use SMTChecker

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.

https://twitter.com/0xOwenThurm/status/1614359896350425088?t=dbG9gHFigBX85Rv29lOjIQ&s=19

[N-09]  Add NatSpec Mapping comment

Description: Add NatSpec comments describing mapping keys and values

Context:

16 results - 4 files

src/Caller.sol:
  88:     mapping(address => EnumerableSet.AddressSet) internal _authorized;
  90:     mapping(address => uint256) public nonce;
  91  

src/Drips.sol:
   175:   mapping(uint256 => mapping(uint256 => DripsState)) states;
   184:   mapping(uint256 => uint32[2 ** 32]) nextSqueezed;
   202:   mapping(uint32 => AmtDelta) amtDeltas;
   245:   mapping(uint32 => AmtDelta) storage amtDeltas = state.amtDeltas;
   872:   mapping(uint256 => DripsState) storage states,
  1026:   mapping(uint32 => AmtDelta) storage amtDeltas = state.amtDeltas;
  1036:   mapping(uint32 => AmtDelta) storage amtDeltas,

src/DripsHub.sol:
   99:    mapping(uint32 => address) driverAddresses;
  101:    mapping(IERC20 => uint256) totalBalances;
  630:    mapping(IERC20 => uint256) storage totalBalances = _dripsHubStorage().totalBalances;

src/Splits.sol:
   76:    mapping(uint256 => SplitsState) splitsStates;
   83:    mapping(uint256 => SplitsBalance) balances;
  148:    mapping(uint256 => SplitsState) storage splitsStates = _splitsStorage().splitsStates;

Recommendation:

 /// @dev    address(user) -> address(ERC0 Contract Address) -> uint256(allowance amount from user)
    mapping(address => mapping(address => uint256)) public allowance;

[N-10] Remove Unused Codes

return keyword is not necessary

src/DripsHub.sol:
  108      /// Must be higher than 1
  109:     constructor(uint32 cycleSecs_)
  110:         Drips(cycleSecs_, _erc1967Slot("eip1967.drips.storage"))
  111:         Splits(_erc1967Slot("eip1967.splits.storage"))
  112:     {
  113:         return;
  114:     }

[N-11] Highest risk must be specified in NatSpec comments and documentation

src/Managed.sol:
  149  
  150:     /// @notice Authorizes the contract upgrade. See `UUPSUpgradeable` docs for more details.
  151:     function _authorizeUpgrade(address /* newImplementation */ ) internal view override onlyAdmin {
  152:         return;
  153:     }

Description: Although the UUPS model has many advantages and the proposed proxy model is currently the UUPS model, there are some caveats we should be aware of before applying it to a real-world project.

One of the main attention: Since upgrades are done through the implementation contract with the help of the _authorizeUpgrade method, there is a high risk of new implementations such as excluding the _authorizeUpgrade method, which can permanently kill the smart contract's ability to upgrade. Also, this pattern is somewhat complicated to implement compared to other proxy patterns.

Recommendation: Therefore, this highest risk must be found in NatSpec comments and documents.

[N-12] Not using the type name in function specified in returns causes confusion

Drips._squeezed() function has uint128 type and squeezedAmtname returns argument , but this name never use in function , thats causes confusion

src/Drips.sol:
  469      /// @return squeezedAmt The squeezed amount.
  470:     function _squeezedAmt(
  471:         uint256 userId,
  472:         DripsHistory memory dripsHistory,
  473:         uint32 squeezeStartCap,
  474:         uint32 squeezeEndCap
- 475:     ) private view returns (uint128 squeezedAmt) {
+ 475:     ) private view returns (uint128) {

  476:         DripsReceiver[] memory receivers = dripsHistory.receivers;
  477:         // Binary search for the `idx` of the first occurrence of `userId`
  478:         uint256 idx = 0;
  479:         for (uint256 idxCap = receivers.length; idx < idxCap;) {
  480:             uint256 idxMid = (idx + idxCap) / 2;
  481:             if (receivers[idxMid].userId < userId) {
  482:                 idx = idxMid + 1;
  483:             } else {
  484:                 idxCap = idxMid;
  485:             }
  486:         }
  487:         uint32 updateTime = dripsHistory.updateTime;
  488:         uint32 maxEnd = dripsHistory.maxEnd;
  489:         uint256 amt = 0;
  490:         for (; idx < receivers.length; idx++) {
  491:             DripsReceiver memory receiver = receivers[idx];
  492:             if (receiver.userId != userId) break;
  493:             (uint32 start, uint32 end) =
  494:                 _dripsRange(receiver, updateTime, maxEnd, squeezeStartCap, squeezeEndCap);
  495:             amt += _drippedAmt(receiver.config.amtPerSec(), start, end);
  496:         }
  497:         return uint128(amt);
  498:     }

Recommendation: Consider adopting a consistent approach to return values throughout the codebase by removing all named return variables, explicitly declaring them as local variables, and adding the necessary return statements where appropriate. This would improve both the explicitness and readability of the code, and it may also help reduce regressions during future code refactors.

[N-13] Use a single file for all system-wide constants

There are many addresses and constants used in the system. It is recommended to put the most used ones in one file (for example constants.sol, use inheritance to access these values)

This will help with readability and easier maintenance for future changes.

constants.sol Use and import this file in contracts that require access to these values. This is just a suggestion, in some use cases this may result in higher gas usage in the distribution

16 results - 4 files

src/Caller.sol:
  81  contract Caller is EIP712("Caller", "1"), ERC2771Context(address(this)) {
  82:     string internal constant CALL_SIGNED_TYPE_NAME = "CallSigned("
  83          "address sender,address to,bytes data,uint256 value,uint256 nonce,uint256 deadline)";

src/Drips.sol:
  105      /// Limits cost of changes in drips configuration.
  106:     uint256 internal constant _MAX_DRIPS_RECEIVERS = 100;
  107      /// @notice The additional decimals for all amtPerSec values.
  108:     uint8 internal constant _AMT_PER_SEC_EXTRA_DECIMALS = 9;
  109      /// @notice The multiplier for all amtPerSec values. It's `10 ** _AMT_PER_SEC_EXTRA_DECIMALS`.
  110:     uint256 internal constant _AMT_PER_SEC_MULTIPLIER = 1_000_000_000;
  111      /// @notice The total amount the contract can keep track of each asset.
  112:     uint256 internal constant _MAX_TOTAL_DRIPS_BALANCE = uint128(type(int128).max);
  113      /// @notice On every timestamp `T`, which is a multiple of `cycleSecs`, the receivers

src/DripsHub.sol:
  34  /// so recently dripped funds may not be receivable immediately.
  35: /// `cycleSecs` is a constant configured when the drips hub is deployed.
  36  /// The receiver balance is independent from the drips balance,

  55      /// Limits cost of changes in drips configuration.
  56:     uint256 public constant MAX_DRIPS_RECEIVERS = _MAX_DRIPS_RECEIVERS;
  57      /// @notice The additional decimals for all amtPerSec values.
  58:     uint8 public constant AMT_PER_SEC_EXTRA_DECIMALS = _AMT_PER_SEC_EXTRA_DECIMALS;
  59      /// @notice The multiplier for all amtPerSec values.
  60:     uint256 public constant AMT_PER_SEC_MULTIPLIER = _AMT_PER_SEC_MULTIPLIER;
  61      /// @notice Maximum number of splits receivers of a single user. Limits the cost of splitting.
  62:     uint256 public constant MAX_SPLITS_RECEIVERS = _MAX_SPLITS_RECEIVERS;
  63      /// @notice The total splits weight of a user
  64:     uint32 public constant TOTAL_SPLITS_WEIGHT = _TOTAL_SPLITS_WEIGHT;
  65      /// @notice The offset of the controlling driver ID in the user ID.
  66      /// In other words the controlling driver ID is the highest 32 bits of the user ID.
  67:     uint256 public constant DRIVER_ID_OFFSET = 224;
  68      /// @notice The total amount the contract can store of each token.
  69      /// It's the minimum of _MAX_TOTAL_DRIPS_BALANCE and _MAX_TOTAL_SPLITS_BALANCE.
  70:     uint256 public constant MAX_TOTAL_BALANCE = _MAX_TOTAL_DRIPS_BALANCE;
  71      /// @notice The ERC-1967 storage slot holding a single `DripsHubStorage` structure.

src/Splits.sol:
  19      /// @notice Maximum number of splits receivers of a single user. Limits the cost of splitting.
  20:     uint256 internal constant _MAX_SPLITS_RECEIVERS = 200;
  21      /// @notice The total splits weight of a user
  22:     uint32 internal constant _TOTAL_SPLITS_WEIGHT = 1_000_000;
  23      /// @notice The total amount the contract can keep track of each asset.
  24      // slither-disable-next-line unused-state
  25:     uint256 internal constant _MAX_TOTAL_SPLITS_BALANCE = type(uint128).max;
  26      /// @notice The storage slot holding a single `SplitsStorage` structure.

[S-01] Generate perfect code headers every time

Description: I recommend using header for Solidity code layout and readability

https://github.com/transmissions11/headers

/*//////////////////////////////////////////////////////////////
                           TESTING 123
//////////////////////////////////////////////////////////////*/

Gas Optimizations List

Total 18 issues

[G-01] Starting from Solidity '0.8.13', typing 'unchecked' increases gas cost by 3x

10 results - 5 files:

src\Drips.sol:

  1103:    assembly {
  1104         let endedCycles := sub(div(end, cycleSecs), div(start, cycleSecs))

  1145:    assembly {
  1146         dripsStorage.slot := slot

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L1103

src\DripsHub.sol:

  624:     assembly {
  625          storageRef.slot := slot

https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L624

src\Managed.sol:

  145:     assembly {
  146          storageRef.slot := slot

https://github.com/code-423n4/2023-01-drips/blob/main/src/Managed.sol#L145

src\Splits.sol:

  286:     assembly {
  287          splitsStorage.slot := slot

https://github.com/code-423n4/2023-01-drips/blob/main/src/Splits.sol#L286

src\Drips.sol:

   686:    unchecked {
   687         (uint256[] memory configs, uint256 configsLen) = _buildConfigs(receivers);

   743:    unchecked {
   744         uint256 spent = 0;

   774:    unchecked {
   775         require(receivers.length <= _MAX_DRIPS_RECEIVERS, "Too many drips receivers");

  1041:    unchecked {

  1121:    unchecked {
  1122         return timestamp / _cycleSecs + 1;

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L686

[G-02] Use hardcode address instead address(this)

Instead of address(this), it is more gas-efficient to pre-calculate and use the address with a hardcode. Foundry's script.sol and solmate LibRlp.sol contracts can do this.

Reference: https://book.getfoundry.sh/reference/forge-std/compute-create-address https://twitter.com/transmissions11/status/1518507047943245824

7 results - 4 files:

src\AddressDriver.sol:

  171:    erc20.safeTransferFrom(_msgSender(), address(this), amt);

  173:    if (erc20.allowance(address(this), address(dripsHub)) == 0) {

https://github.com/code-423n4/2023-01-drips/blob/main/src/AddressDriver.sol#L171

src\Caller.sol:

  81:     contract Caller is EIP712("Caller", "1"), ERC2771Context(address(this)) {

https://github.com/code-423n4/2023-01-drips/blob/main/src/Caller.sol#L81

src\DripsHub.sol:

  416:    erc20.safeTransferFrom(msg.sender, address(this), amt);

  533:    erc20.safeTransferFrom(msg.sender, address(this), uint128(realBalanceDelta));

https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L416

src\NFTDriver.sol:

  286:    erc20.safeTransferFrom(_msgSender(), address(this), amt);

  288:    if (erc20.allowance(address(this), address(dripsHub)) == 0) {

https://github.com/code-423n4/2023-01-drips/blob/main/src/NFTDriver.sol#L286

[G-03] Using constants without caching saves gas

As seen in the code below, using constants without caching saves around 8k gas in deployment cost.

src\Drips.sol:
  1036:     function _addDelta(
  1037:         mapping(uint32 => AmtDelta) storage amtDeltas,
  1038:         uint256 timestamp,
  1039:         int256 amtPerSec
  1040:     ) private {
  1041:         unchecked {
  1042:             // In order to set a delta on a specific timestamp it must be introduced in two cycles.
  1043:             // These formulas follow the logic from `_drippedAmt`, see it for more details.
- 1044:             int256 amtPerSecMultiplier = int256(_AMT_PER_SEC_MULTIPLIER);
- 1045:             int256 fullCycle = (int256(uint256(_cycleSecs)) * amtPerSec) / amtPerSecMultiplier;
+ 1045:             int256 fullCycle = (int256(uint256(_cycleSecs)) * amtPerSec) / int256(_AMT_PER_SEC_MULTIPLIER);
  1046:             // slither-disable-next-line weak-prng
- 1047:             int256 nextCycle = (int256(timestamp % _cycleSecs) * amtPerSec) / amtPerSecMultiplier;
+ 1047:             int256 nextCycle = (int256(timestamp % _cycleSecs) * amtPerSec) / int256(_AMT_PER_SEC_MULTIPLIER);
  1048:             AmtDelta storage amtDelta = amtDeltas[_cycleOf(uint32(timestamp))];
  1049:             // Any over- or under-flows are fine, they're guaranteed to be fixed by a matching
  1050:             // under- or over-flow from the other call to `_addDelta` made by `_addDeltaRange`.
  1051:             // This is because the total balance of `Drips` can never exceed `type(int128).max`,
  1052:             // so in the end no amtDelta can have delta higher than `type(int128).max`.
  1053:             amtDelta.thisCycle += int128(fullCycle - nextCycle);
  1054:             amtDelta.nextCycle += int128(nextCycle);
  1055:         }
  1056:     }

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L1036-L1056

before;

| test/Drips.t.sol:DripsTest contract |                 |       |        |        |         |
|-------------------------------------|-----------------|-------|--------|--------|---------|
| Deployment Cost                     | Deployment Size |       |        |        |         |
| 9220438                             | 44913           |       |        |        |         |
| Function Name                       | min             | avg   | median | max    | # calls |
| balanceAtExternal                   | 1589            | 1892  | 1892   | 2195   | 2       |
| setDripsExternal                    | 2577            | 24085 | 6519   | 170082 | 9       |
| squeezeDripsExternal                | 2859            | 3358  | 3358   | 3857   | 2       |
| squeezeDripsResultExternal          | 2812            | 3311  | 3311   | 3810   | 2       |

after;

| test/Drips.t.sol:DripsTest contract |                 |       |        |        |         |
|-------------------------------------|-----------------|-------|--------|--------|---------|
| Deployment Cost                     | Deployment Size |       |        |        |         |
| 9212411                             | 44866           |       |        |        |         |
| Function Name                       | min             | avg   | median | max    | # calls |
| balanceAtExternal                   | 1589            | 1892  | 1892   | 2195   | 2       |
| setDripsExternal                    | 2577            | 24085 | 6519   | 170082 | 9       |
| squeezeDripsExternal                | 2859            | 3358  | 3358   | 3857   | 2       |
| squeezeDripsResultExternal          | 2812            | 3311  | 3311   | 3810   | 2       |

[G-04] Setting a loop interval instead of a "while" loop causes gas saved

Drips.sol#L716-L729 has a while loop on this line. When using while loops, the Solidity compiler is aware that a condition needs to be checked first, before executing the code inside the loop.

Firstly, the conditional part is evaluated:

a) if the condition evaluates to true , the instructions inside the loop (defined inside the brackets { ... }) get executed.

b) once the code reaches the end of the loop body (= the closing curly brace } , it will jump back up to the conditional part).

c) the code inside the while loop body will keep executing over and over again until the conditional part turns false.

For loops are more associated with iterations. While loop instead are more associated with repetitive tasks. Such tasks could potentially be infinite if the while loop body never affects the condition initially checked. Therefore, they should be used with care.

At very small intervals, a large number of loops can be accessed by someone attempting a DOS attack. This causes very high gas.

src/Drips.sol:
  716:             while (true) {
  717:                 uint256 end = (enoughEnd + notEnoughEnd) / 2;
  718:                 if (end == enoughEnd) {
  719:                     return uint32(end);
  720:                 }
  721:                 if (_isBalanceEnough(balance, configs, configsLen, end)) {
  722:                     enoughEnd = end;
  723:                 } else {
  724:                     notEnoughEnd = end;
  725:                 }
  726:             }
  727:             // slither-disable-end incorrect-equality,timestamp
  728:         }
  729      }

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L716-L729

Recommendation: Set a reasonable while loop interval

[G-05] Gas overflow during iteration (DoS)

Each iteration of the cycle requires a gas flow. A moment may come when more gas is required than it is allocated to record one block. In this case, all iterations of the loop will fail.

So set a reasonable max.length

src/Caller.sol:
  193:     function callBatched(Call[] memory calls) public payable returns (bytes[] memory returnData) {
  194:         returnData = new bytes[](calls.length);
  195:         address sender = _msgSender();
+                 require(calls.length < maxLengt, "max length");
  196:         for (uint256 i = 0; i < calls.length; i++) {
  197:             Call memory call = calls[i];
  198:             returnData[i] = _call(sender, call.to, call.data, call.value);
  199:         }
  200:     }

https://github.com/code-423n4/2023-01-drips/blob/main/src/Caller.sol#L193-L200

[G-06] Using both mint and safeMint method at the same time is gas wasted method

For all battle-tested projects, either mint or safeMint patterns are preferred, this is a design decision; It is evaluated in terms of security concerns and gas optimization, but using both at the same time causes confusion, one of them should be chosen as best practice.

In case of using mint, if msg.sender is contract, a faulty contract may be sent without supporting NFT purchase.

In the design, mint gas with gas optimization should be preferred.

solidity
src/NFTDriver.sol:
  62:     function mint(address to, UserMetadata[] calldata userMetadata)
  63:         public
  64:         whenNotPaused
  65:         returns (uint256 tokenId)
  66:     {
  67:         tokenId = _registerTokenId();
  68:         _mint(to, tokenId);
  69:         if (userMetadata.length > 0) dripsHub.emitUserMetadata(tokenId, userMetadata);
  70:     }

https://github.com/code-423n4/2023-01-drips/blob/main/src/NFTDriver.sol#L62-L70

src/NFTDriver.sol:
- 79:     function safeMint(address to, UserMetadata[] calldata userMetadata)
- 80:         public
- 81:         whenNotPaused
- 82:         returns (uint256 tokenId)
- 83:     {
- 84:         tokenId = _registerTokenId();
- 85:         _safeMint(to, tokenId);
- 86:         if (userMetadata.length > 0) dripsHub.emitUserMetadata(tokenId, userMetadata);
- 87:     }

https://github.com/code-423n4/2023-01-drips/blob/main/src/NFTDriver.sol#L79-L87

[G-07] Using storage instead of memory for structs/arrays saves gas

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct

5 results - 2 files:

src\Caller.sol:

  197:    Call memory call = calls[i];

https://github.com/code-423n4/2023-01-drips/blob/main/src/Caller.sol#L197

src\Drips.sol:

  451:    DripsHistory memory drips = dripsHistory[i];

  564:    DripsReceiver memory receiver = receivers[i];

  665:    DripsReceiver memory receiver = newReceivers[i];

  778:    DripsReceiver memory receiver = receivers[i];

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L451

[G-08] Remove unused codes

Unused code will increase deployment cost.

src/DripsHub.sol:

  109:     constructor(uint32 cycleSecs)
  110:         Drips(cycleSecs, _erc1967Slot("eip1967.drips.storage"))
  111:         Splits(_erc1967Slot("eip1967.splits.storage"))
  112:     {
- 113:         return;
  114:     }

https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L113

[G-09] Using delete instead of setting struct to 0 saves gas

2 results - 1 file:

src\Splits.sol:
  156:         balance.splittable = 0;

  188:         balance.collectable = 0;

https://github.com/code-423n4/2023-01-drips/blob/main/src/Splits.sol#L156 https://github.com/code-423n4/2023-01-drips/blob/main/src/Splits.sol#L188

Recommendation Code:

src\Splits.sol:

- 156:         balance.splittable = 0;
+ 156:         delete balance.splittable;

[G-10] Not using the named return variables when a function returns, wastes deployment gas

src\Drips.sol:
   985:     function _dripsRange(
   
-   991:     ) private pure returns (uint32 start, uint32 end_) {
+   991:     ) private pure returns (uint32, uint32) {
   
   1012:         return (start, uint32(end));

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L991

[G-11] Don't use _msgSender() if not supporting EIP-2771

Use msg.sender if the code does not implement EIP-2771 trusted forwarder support.

Reference: https://eips.ethereum.org/EIPS/eip-2771

10 results - 3 files:

src\AddressDriver.sol:

   47:    return calcUserId(_msgSender());

  171:    erc20.safeTransferFrom(_msgSender(), address(this), amt);

https://github.com/code-423n4/2023-01-drips/blob/main/src/AddressDriver.sol#L47

src\Caller.sol:

  107:    address sender = _msgSender();

  115:    address sender = _msgSender();

  149:    require(isAuthorized(sender, _msgSender()), "Not authorized");

  195:    address sender = _msgSender();

https://github.com/code-423n4/2023-01-drips/blob/main/src/Caller.sol#L107

src\NFTDriver.sol:

   42:    _isApprovedOrOwner(_msgSender(), tokenId),

  286:    erc20.safeTransferFrom(_msgSender(), address(this), amt);

  295:    return ERC2771Context._msgSender();

https://github.com/code-423n4/2023-01-drips/blob/main/src/NFTDriver.sol#L42

[G-12] Use nested if and, avoid multiple check combinations

Using nested is cheaper than using && multiple check combinations. There are more advantages, such as easier to read code and better coverage reports.

5 results - 1 file:

src\Drips.sol:
  700:    if (hint1 > enoughEnd && hint1 < notEnoughEnd) {

  708:    if (hint2 > enoughEnd && hint2 < notEnoughEnd) {
  
  898     if (
  899:         pickCurr && pickNew
  900:             && (
  901                 currRecv.userId != newRecv.userId
  902                     || currRecv.config.amtPerSec() != newRecv.config.amtPerSec()
  903             )

  909:    if (pickCurr && pickNew) {

  929:    if (currStartCycle > newStartCycle && state.nextReceivableCycle > newStartCycle) {

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L652

Recomendation Code:

src\Drips.sol:
- 700:    if (hint1 > enoughEnd && hint1 < notEnoughEnd) {
+     if (hint1 > enoughEnd) {
+         if (hint1 < notEnoughEnd) {
+               }                                   
+           }                                     

[G-13] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contracts gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html

Use a larger size then downcast where needed.

29 results - 5 fles:

src\Drips.sol:

  /// @audit uint32 receivableCycles
  283:    receivableCycles = toCycle - fromCycle - maxCycles;

  /// @audit uint32 toCycle
  284:    toCycle -= receivableCycles;

  /// @audit uint128 amtPerCycle
  288:    amtPerCycle += state.amtDeltas[cycle].thisCycle;

  /// @audit uint128 receivedAmt
  289:    receivedAmt += uint128(amtPerCycle);

  /// @audit uint128 amtPerCycle
  290:    amtPerCycle += state.amtDeltas[cycle].nextCycle;

  /// @audit uint32 fromCycle, uint32 toCycle
  306:    return toCycle - fromCycle;

  /// @audit uint32 fromCycle
  319:    fromCycle = _dripsStorage().states[assetId][userId].nextReceivableCycle;

  /// @audit uint32 toCycle
  320:    toCycle = _cycleOf(_currTimestamp());

  /// @audit uint128 amt
  429:    amt += _squeezedAmt(userId, drips, squeezeStartCap, squeezeEndCap);

  /// @audit uint32 squeezeEndCap
  432:    squeezeEndCap = drips.updateTime;

  /// @audit uint128 balance
  572:    balance -= uint128(_drippedAmt(receiver.config.amtPerSec(), start, end));

  /// @audit uint128 mewbalance
  636:    newBalance = uint128(currBalance + realBalanceDelta);

  /// @audit uint32 newMaxEnd 
  637:    newMaxEnd = _calcMaxEnd(newBalance, newReceivers, maxEndHint1, maxEndHint2);

  /// @audit uint32 start
  992:    start = receiver.config.start();

  /// @audit uint32 timestamp
  1122:   return timestamp / _cycleSecs + 1;

  /// @audit uint32 currTimestamp
  1137:   return currTimestamp - (currTimestamp % _cycleSecs);

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L283-L284

src\DripsHub.sol:

  /// @audit uint32 driverId
  136:    driverId = dripsHubStorage.nextDriverId++;

  /// @audit uint128 receivedAmt
  244:    receivedAmt = Drips._receiveDrips(userId, assetId, maxCycles);

  /// @audit uint128 amt
  278:    amt = Drips._squeezeDrips(userId, assetId, senderId, historyHash, dripsHistory);

  /// @audit uint128 amt
  392:    amt = Splits._collect(userId, _assetId(erc20));

https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L136

src\Splits.sol:

  /// @audit uint32 splitsWeight
  128:    splitsWeight += currReceivers[i].weight;

  /// @audit uint128 splitAmt
  130:    splitAmt = uint128((uint160(amount) * splitsWeight) / _TOTAL_SPLITS_WEIGHT);

  /// @audit uint128 collectableAmt
  131:    collectableAmt = amount - splitAmt;

  /// @audit uint32 splitsWeight
  159:    splitsWeight += currReceivers[i].weight;

  /// @audit uint128 splitAmt
  162:    splitAmt += currSplitAmt;

  /// @audit uint128 collectableAmt
  167:    collectableAmt -= splitAmt;

  /// @audit uint64 totalWeight
  235:    totalWeight += weight;

https://github.com/code-423n4/2023-01-drips/blob/main/src/Splits.sol#L128

src\AddressDriver.sol:

  /// @audit uint128 amt
  61:     amt = dripsHub.collect(callerUserId(), erc20);

https://github.com/code-423n4/2023-01-drips/blob/main/src/AddressDriver.sol#L61

src\NFTDriver.sol:

  /// @audit uint128 amt
  115:    amt = dripsHub.collect(tokenId, erc20);

https://github.com/code-423n4/2023-01-drips/blob/main/src/NFTDriver.sol#L115

[G-14] Sort Solidity operations using short-circuit mode

Short-circuiting is a solidity contract development model that uses OR/AND logic to sequence different cost operations. It puts low gas cost operations in the front and high gas cost operations in the back, so that if the front is low If the cost operation is feasible, you can skip (short-circuit) the subsequent high-cost Ethereum virtual machine operation.

//f(x) is a low gas cost operation 
//g(y) is a high gas cost operation 

//Sort operations with different gas costs as follows 
f(x) || g(y) 
f(x) && g(y)

5 results - 2 files:

src\Drips.sol:
  322:    if (fromCycle == 0 || toCycle < fromCycle) {

  691:    if (configsLen == 0 || balance == 0) {
  
  898:    if (
  899       pickCurr && pickNew
  900            && (
  901                currRecv.userId != newRecv.userId
  902                    || currRecv.config.amtPerSec() != newRecv.config.amtPerSec()

  951:    if (state.nextReceivableCycle == 0 || state.nextReceivableCycle > startCycle) {

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L322

src\Managed.sol:
  53          require(
  54:             admin() == msg.sender || isPauser(msg.sender), "Caller is not the admin or a pauser«

https://github.com/code-423n4/2023-01-drips/blob/main/src/Managed.sol#L53-L54

[G-15] Setting the constructor to payable

You can cut out 10 opcodes in the creation-time EVM bytecode if you declare a constructor payable. Making the constructor payable eliminates the need for an initial check of msg.value == 0 and saves 13 gas on deployment with no security risks.

8 results - 7 files:

src\AddressDriver.sol:

  30:     constructor(DripsHub _dripsHub, address forwarder, uint32 _driverId)

https://github.com/code-423n4/2023-01-drips/blob/main/src/AddressDriver.sol#L30

src\Drips.sol:

  219:     constructor(uint32 cycleSecs, bytes32 dripsStorageSlot) {

https://github.com/code-423n4/2023-01-drips/blob/main/src/Drips.sol#L219

src\DripsHub.sol:

  109:     constructor(uint32 cycleSecs_)

https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L109

src\ImmutableSplitsDriver.sol:

  28:     constructor(DripsHub _dripsHub, uint32 _driverId) {

https://github.com/code-423n4/2023-01-drips/blob/main/src/ImmutableSplitsDriver.sol#L28

src\Managed.sol:

   73:     constructor() {

  158:     constructor(Managed logic, address admin) ERC1967Proxy(address(logic), new bytes(0)) {

https://github.com/code-423n4/2023-01-drips/blob/main/src/Managed.sol#L73

src\NFTDriver.sol:

  32:     constructor(DripsHub _dripsHub, address forwarder, uint32 _driverId)

https://github.com/code-423n4/2023-01-drips/blob/main/src/NFTDriver.sol#L32

src\Splits.sol:

  94:     constructor(bytes32 splitsStorageSlot) {

https://github.com/code-423n4/2023-01-drips/blob/main/src/Splits.sol#L94

Recommendation: Set the constructor to payable

[G-16] Optimize names to save gas

Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions. 

Context:  All Contracts

Recommendation:  Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas For example, the function IDs in the Drips.sol contract will be the most used; A lower method ID may be given.

Proof of Consept: https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Drips.sol function names can be named and sorted according to METHOD ID

Sighash   |   Function Signature
========================
796321aa  =>  create(uint32,uint160,uint32,uint32)
3a859e28  =>  dripId(DripsConfig)
36574f3d  =>  amtPerSec(DripsConfig)
27e07d3c  =>  start(DripsConfig)
4319a265  =>  duration(DripsConfig)
93c5ef21  =>  lt(DripsConfig,DripsConfig)
5025ce09  =>  _receiveDrips(uint256,uint256,uint32)
208536b3  =>  _receiveDripsResult(uint256,uint256,uint32)
f094c2e2  =>  _receivableDripsCycles(uint256,uint256)
65bcd9c1  =>  _receivableDripsCyclesRange(uint256,uint256)
2007ed7a  =>  _squeezeDrips(uint256,uint256,uint256,bytes32,DripsHistory[])
3a028b2f  =>  _squeezeDripsResult(uint256,uint256,uint256,bytes32,DripsHistory[])
8df20ef7  =>  _verifyDripsHistory(bytes32,DripsHistory[],bytes32)
cde22657  =>  _squeezedAmt(uint256,DripsHistory,uint32,uint32)
c1c00721  =>  _dripsState(uint256,uint256)
490762d9  =>  _balanceAt(uint256,uint256,DripsReceiver[],uint32)
5d544393  =>  _balanceAt(uint128,uint32,uint32,DripsReceiver[],uint32)
5ddcd5ab  =>  _setDrips(uint256,uint256,DripsReceiver[],int128,DripsReceiver[],uint32,uint32)
3030aefd  =>  _calcMaxEnd(uint128,DripsReceiver[],uint32,uint32)
fa0fae1f  =>  _isBalanceEnough(uint256,uint256[],uint256,uint256)
2968a833  =>  _buildConfigs(DripsReceiver[])
f41962fa  =>  _addConfig(uint256[],uint256,DripsReceiver)
a665230e  =>  _getConfig(uint256[],uint256)
846d07be  =>  _hashDrips(DripsReceiver[])
ded0b165  =>  _hashDripsHistory(bytes32,bytes32,uint32,uint32)
c4410956  =>  _updateReceiverStates(mapping(uint256)
2b9a67af  =>  _dripsRangeInFuture(DripsReceiver,uint32,uint32)
4d49c72f  =>  _dripsRange(DripsReceiver,uint32,uint32,uint32,uint32)
f2763cf7  =>  _addDeltaRange(DripsState,uint32,uint32,int256)
c5fda91e  =>  _addDelta(mapping(uint32)
abbbd536  =>  _isOrdered(DripsReceiver,DripsReceiver)
058efd18  =>  _drippedAmt(uint256,uint256,uint256)
bee3d77b  =>  _cycleOf(uint32)
25f58251  =>  _currTimestamp()
8e001409  =>  _currCycleStart()
9ecda414  =>  _dripsStorage()

[G-17] Use Minimalist and gas efficient standard ERC721 implementation

Project Use OpenZeppelin ERC721 nft contract.

I recommend using the minimalist and gas-efficient standard ERC1155 implementation.

Reference: https://github.com/transmissions11/solmate/blob/main/src/tokens/ERC721.sol

[G-18] Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants.

3 results 1 file:

src\DripsHub.sol:

   56:     uint256 public constant MAX_DRIPS_RECEIVERS = _MAX_DRIPS_RECEIVERS;

   67:    uint256 public constant DRIVER_ID_OFFSET = 224;

   70:    uint256 public constant MAX_TOTAL_BALANCE = _MAX_TOTAL_DRIPS_BALANCE;

https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L56