# Internal Security Review: Drips Contract v2.0
Author: `Manuel Polzhofer`
Date: `November 2022`

### Scope
The following security review focuses on potential bugs and security issues in the new features and changes to the **Drips Solidity Contracts** since July 2022.

In addition, the review aims to identify features that have introduced complexity and discuss their design and underlying requirements.

The Drips SDK library is not within the scope of this document.

### Drips Contracts Repository
Repository: https://github.com/radicle-dev/drips-contracts
Commit Hash: [835656a99015e3cc28ee1003924654e5071f3d00](https://github.com/radicle-dev/drips-contracts/commit/835656a99015e3cc28ee1003924654e5071f3d00)

## Minor

### M.1. Caller batchTransaction doesn't verify msg.value sum

The caller contract allows to batch multiple different transaction together. Each of them can include a `msg.value` to send ETH to a contract.

returnData[i] = _call(sender, call.to, call.data, call.value);

The ETH value of a indiviual batch call is passed as a parameter with `call.value`. However, the sum of all `call.value` is not verfied against the initial `msg.value` sent to the contract itself.

In case the caller contract holds any ETH a user could send less ETH to the contract and use the ETH locked in the caller contract for a transaction.

#### Recommendation
Verify that the sum of the individual `call.value` equals `msg.value`. For a clean implementation fo the batch transaction pattern.

As an alternative we could check the pre-state eth balance of the contract and compare it with the post-state ETH balance.

### M.2. Caller callAs authorize pattern gives permission to add other authorized addresses
A user can give another user the permission to act in behalf of them. However, this user also gets the permission to authorize other user again.

## Informal

### I.1. _squeezedAmt receiver binary search doesn't need be performed on-chain

In the squeezing algorithm as sender configuration is passed to drips contract in `calldata`. A sender can stream to multiple different userId's and one or multiple of them stream to receiver which should get squeezed.

Currently to find the squeeze-receiver a binary search is applied on the sorted sender-configuration.

        // binary search in squeezed
        DripsReceiver[] memory receivers = dripsHistory.receivers;
        // Binary search for the `idx` of the first occurrence of `userId`
        uint256 idx = 0;
        for (uint256 idxCap = receivers.length; idx < idxCap;) {
            uint256 idxMid = (idx + idxCap) / 2;
            if (receivers[idxMid].userId < userId) {
                idx = idxMid + 1;
            } else {
                idxCap = idxMid;
            }
        }

#### Recommendation
The `indexes` of the receiver in the sender array could be passed as a parameter to safe gas.

### I.2. Simplification of squeezeDrips and squeezeDripsResult

The function `squeezeDripsResult` performs the squeezing algorithm without modifying the state. This function is used by `squeezeDrips` which modifies the state afterwards.

However, it results that an array with the `squeezedIdxs` needs to be generated and passed arround. The length of the array can be more than needed.

#### Recommendation
One function which peforms the `squeezeDrips` algorithm and modifies the state to avoid passing an array around. A seperate `view` function not used by `squeezeDrips` could be added.
This would reduce the gas cost and reduce the complexity of the code which acutally modifies the state.

### I.3. ERC20 Rebase-Token break Drips logic

It is important to point out that the current Drips implementation doesn't support any rebase-token. A rebase token is a token in which the balance of a user can increase or decrease over time.

**Example**
1. A user deposits a 100 tokens into Drips
2. The token balance increases to actual 120 tokens
3. The user can only withdraw 100 tokens instead of 120

For most common rebase token a wrapped version with a fixed balance of tokens exists.

The fixed balance represents a share in the underlying rebase token.

**Recommendation**
Add rebase limitation to documentation and code comments. Like in description of the `erc20` parameter addresses.

### I.4. Unused imports in the source code

In `ImmutableDrips`

import {
    ERC721,
    ERC721Burnable
} from "openzeppelin-contracts/token/ERC721/extensions/ERC721Burnable.sol";

### I.5. Remove Upgradeablity from ImmutableSplitsDriver
The ImmutableSplitsDriver only has some immutable variables to point to other drips contracts and no additional state. The upgradablity feature could be removed and the contract could be just redeployed
in case the address of the drips contract changes.

It would also add more trust to the `immutable` feature, since an upgrade could potentially allow the change the splits configurations again.

### Gas Improvements
#### G.1. use custom error types instead of revert messages
Usage of custom Solidity error types instead of string revert message can reduce gas costs.
#### G.2. allow passing lower and upper hint block.timestamps for binary search to find `maxEnd`
Currently, finding the `maxEnd` needs to be performed on-chain because the block.timestamp of the transaction is unkown in advance.

However, it would be possible to pass a lower and upper value for the search, which could be the exact `maxEnd` in some cases.

In other cases a mint time assumption could be made. Only if the `maxEnd` is not inbetween lower and upper limit.

#### G.3. nextSqueezing using a mapping instead of an array in Drips.sol

A common pattern in Solidity is to use a mapping instead of a storage array.

An array over the entire storage space is also a potential `security risk`.
If the user could choose the `currCycleConfigs` as a parameter we would give them full storage write access to write a block.timestamp.

Currently, it is not such a problem because `currCycleConfigs` is autoincrement and the next interesting storage slot should be too far away.

The gas-cost are too high to perform such an attack with different cycleConfigs.

However, with a mapping which results in a hashing it is too computation intensive to find a index for an interesting slot.

**Currently**

mapping(uint256 => uint32[2 ** 32]) nextSqueezed;

**Recommentation**

 mapping(uint256 => mapping(uint256 => uint32)) nextSqueezed;

**Array**
[PASS] testSqueezeDrips() (gas: 257887)

**Mapping**
[PASS] testSqueezeDrips() (gas: 257810)

I tested the change and all tests are passing.

### Architecture Discussion

#### 1. Caller Pattern with self-calling trustedForwarder
For enabling meta-transactions in `drips` a ERC712 scheme has been implemented in a `Caller` contract.

**Meta Transaction**

This feature allows any address to act in behalf of any user if a signed message by the user is provided. After a signature verification for a concrete action the call will be forwarded. Each interaction can only happens once afterwards the signature for a specific action is invalid.

A sender signs a message with indicates which function should be called in behalf of them. The `caller` contracts verifies the signature and if correct attaches the message signer address in `calldata`.
Other contracts like the `AddressDriver` or `NFTDriver` use the `Caller` contract as so called `TrustedForwarder`.
The address passed by the `Caller` will be treated like a `msg.sender` from other calls.

**callAs Pattern**
However, the `Caller` contract itself treates itself as a `TrustedForwarder` for recursive self-calls.
This happens because the `Caller` contract manages some additional state for so called `callAs` pattern.

In the `callAs` pattern a user can give the permission to an Ethereum address to act in behalf of him. For the `callAs` pattern user gives the full power to one specific address.

An user give permission by calling the `Caller` directly or by using the meta transaction feature.

**Batch Transaction**
TODO

**Complex Scenario**
The `callAs`, `MetaTransaction` and `BatchTransaction` used together allow to generate complex secenarios.

Consider the following:

Alice gave Bob a `callAs` permission to act in behalf of her. Bob wants to make a batch transaction as meta transaction.
Therefore he signs a message to **callAs** which should call `callBatched**

**Call-Stack**
1. **Charly** calls **Caller.callSigned** (`_msgSender() == Charly`)
2. **Caller.callSigned** calls **Caller.calledAs** (`_msgSender()==Bob`)
3. **Caller.calledAs** calls **Caller.callBatched** (`_msgSender()==Alice`)
4. **Caller.callBatched** calls **AddressDriver.setDrips**(`_msgSender() == Alice`)

**Discussion**
- Considering if the `calledAs` feature is really needed or it can be removed to reduce the complexity.
- A `callAs` pattern could be always later implemented on top by other parties if really needed.
- In many cases a multi-sig with one signature approval might used instead.

#### 2. Squeezing Complexity / Drips Complexity
The supported features in Drips allow an immense varity of different cases in the contracts.
I want to provide some examples to ilustrate complexity of and permutation of different scenarios of squeezing the contractsneed to handle accordingly.

- squeeze a drips which **started in the the current cycle**
- squeeze a drips which **starts in the future**
- squeeze a drips which **ended in the the current cycle**
- squeeze a drips which **will end in a future cycle**
- multiple **different sender configs in one cycle**
- multiple **different squeeze combinations of one cycle**
- regular drip **ends before squeeze happens**
- **overlapping drip from previous cycle**
- drips end because **maxEnd reached** (duration=0 case)
- drip started with **current block.timestamp** (start=0)
- **maxEnd** in curr Cycle **before squeezing**
- **maxEnd** in currCycle **after squeezing**
- **not using all drips from dripsHistory** in squeezing
- **holes in squeezing** (drips end in curr cycle with maxEnd and normal end) and start a new one after some time in same cycle
- **multiple squeezes** after more time has passed
- scheduled**drips in the future** and remove it again
- calling squeeze in edgeCases like **beginning of a cycle**
- calling squeezing with a sender which **doesn't include the receiver**
- **same receiver multiple times** in the sender config (with all combinations, start=0, duration=0, etc)

# Unstructed Notes

### Questions
Why do we allow multiple entries of the same stream in one receiver config?
- What is the point of it, is it not additional code complexity?
-`_verifyDripsHistory` iterates the drips once to verify and then again for the squeeze algorithm
- squeeze algorithm starts from the end to pass the previous updateTime. It could start from the beginning and look one element ahead to get the updateTime
- squeezeDripsResult view function, can it be simplified. Removal of passing around the array with the squeezeIdx's.
- What are good on-chain invariant which could be added as requires in the contracts to enfore correctness?
- Ideas: `history.length < senderDripsConfigs + 1` => previous configs which have no impact on the current cycle
- inconsistence with return values in different function.