Drips Protocol contest - SleepingBugs's results

Low

Missing checks for address(0x0) when assigning values to address state or immutable variables

NOTE: None of these findings where found by 4naly3er output - NC

Summary

Zero address should be checked for state variables, immutable variables. A zero address can lead into problems.

Code Snippet

• [AddressDriver.sol#L31](https://github.com/code-423n4/2023-01-drips/blob/a271fcc95ed1f2953bfef2345c86c0035a1dffbf/src/AddressDriver.sol#L31

• [ImmutableSplitsDriver.sol#L29](https://github.com/code-423n4/2023-01-drips/blob/1e5917b4e61b6a52a296a857ebbc523b22a95689/src/ImmutableSplitsDriver.sol#L29

• [Managed.sol#L158](https://github.com/code-423n4/2023-01-drips/blob/a271fcc95ed1f2953bfef2345c86c0035a1dffbf/src/Managed.sol#L158

• NFTDriver.sol#L33

• NFTDriver.sol#L36

Some constructor uses some OZ constructors, at them there is no check as can be seen at:

• ERC2771Context.sol#L17

• [ERC1967Upgrade.sol#L123](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/ERC1967/ERC1967Upgrade.sol#L123

Recommendation

Check zero address before assigning or using it

block.timestamp used as time proxy

Summary

Risk of using block.timestamp for time should be considered.

Details

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.

References

SWC ID: 116

Code Snippet

• Drips.sol#L1129 return uint32(block.timestamp);

• Caller.sol#L173 require(block.timestamp <= deadline, "Execution deadline expired");

Recommendation

• 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.
• Consider using an oracle for precision

Destination recipient for assets may be address(0)

• AddressDriver.sol#L62

• AddressDriver.sol#L145

• NFTDriver.sol#L116

• NFTDriver.sol#L204

Description

The recipient of a transfer may be address(0), leading to lost assets.

Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

Impact

For upgradeable contracts, there must be storage gap to "allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments" (quote OpenZeppelin). Otherwise it may be very difficult to write new implementation code. Without storage gap, the variable in child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences to the child contracts, potentially causing loss of user fund or cause the contract to malfunction completely. See.

For a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

Vulnerability Details

In the following context of the upgradeable contracts they are expected to use gaps for avoiding collision:

• UUPS

However, none of these contracts contain storage gap. The storage gap is essential for upgradeable contract because "It allows us to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Refer to the bottom part of this article.

If a contract inheriting from a base contract contains additional variable, then the base contract cannot be upgraded to include any additional variable, because it would overwrite the variable declared in its child contract. This greatly limits contract upgradeability.

Code Snippet

• Managed.sol#L18

Tools Used

Manual analysis

Recommendation

Recommend adding appropriate storage gap at the end of upgradeable contracts such as the below.

uint256[50] private __gap;

Reference OpenZeppelin upgradeable contract templates.

_safeMint() should be used rather than _mint() wherever possible

Impact

In NFTDriver.sol, eventually it is called ERC721 _mint(). Calling _mint() this way does not ensure that the receiver of the NFT is able to accept them, making possible to lose them.

_safeMint() should be used with as it checks to see if a user can properly accept an NFT and reverts otherwise.

There is no check of the address provided by the mint NFT that it implements ERC721Receiver.

Details

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver.

Both open OpenZeppelin and solmate have versions of this function so that NFTs aren’t lost if they’re minted to contracts that cannot transfer them back out.

References

Code Snippet

• NFTDriver.sol#L68 _mint(to, tokenId);

Recommendation

Use _safeMint() as suggested by OpenZeppelin or include the check before minting.

Informational

Use of deprecated draft file

There is a released file that is no longer a draft, it should be used rather than draft files

• Caller.sol#L5 import {ECDSA, EIP712} from "openzeppelin-contracts/utils/cryptography/draft-EIP712.sol";

Constants should be defined rather than using magic numbers

NOTE: None of these findings where found by 4naly3er output - NC

• Drips.sol#L66 config = (config << 160) | amtPerSec_;

• Drips.sol#L67 config = (config << 32) | start_;

• Drips.sol#L68 config = (config << 32) | duration_;

• Drips.sol#L74 return uint32(DripsConfig.unwrap(config) >> 224);

• Drips.sol#L79 return uint160(DripsConfig.unwrap(config) >> 64);

• Drips.sol#L84 return uint32(DripsConfig.unwrap(config) >> 32);

• Drips.sol#L185 mapping(uint256 => uint32[2 ** 32]) nextSqueezed;

• Drips.sol#L357 uint32[2 ** 32] storage nextSqueezed = state.nextSqueezed[senderId];

• Drips.sol#L419 uint32[2 ** 32] storage nextSqueezed =

• Drips.sol#L805 configs[configsLen] = (amtPerSec << 64) | (start << 32) | end;

• Drips.sol#L825 return (val >> 64, uint32(val >> 32), uint32(val));

• Drips.sol#L805 configs[configsLen] = (amtPerSec << 64) | (start << 32) | end;

• Drips.sol#L825 return (val >> 64, uint32(val >> 32), uint32(val));

• NFTDriver.sol#L51 return (uint256(driverId) << 224) + StorageSlot.getUint256Slot(_mintedTokensSlot).value;

• AddressDriver.sol#L41 return (uint256(driverId) << 224) | uint160(userAddr);

• ImmutableSplitsDriver.sol#L37 return (uint256(driverId) << 224) + StorageSlot.getUint256Slot(_counterSlot).value;

Max value can be used rather than 2 ** n

Rather than using 2 ** 256 or other numbers (128, 64, 32, 16, 8...), it can be used type(uint256).max (depending on the exponentiation number).

• Drips.sol#L185 mapping(uint256 => uint32[2 ** 32]) nextSqueezed;

• Drips.sol#L357 uint32[2 ** 32] storage nextSqueezed = state.nextSqueezed[senderId];

• Drips.sol#L419 uint32[2 ** 32] storage nextSqueezed =

Omissions in events

The events can include the new value and old value and even be refactored in just one event

_managedStorage().isPaused

• Managed.sol#L124 emit Paused(msg.sender);

• Managed.sol#L130 emit Unpaused(msg.sender);

Missing initializer modifier on constructor

OpenZeppelin recommends that the initializer modifier be applied to constructors in order to avoid potential griefs, social engineering, or exploits.

• Managed.sol#L73

No need to return void in a constructor

• DripsHub.sol#L113