Author: Manuel Polzhofer
Date: November 2022
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.
Repository: https://github.com/radicle-dev/drips-contracts
Commit Hash: 835656a99015e3cc28ee1003924654e5071f3d00
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.
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.
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.
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;
}
}
The indexes of the receiver in the sender array could be passed as a parameter to safe gas.
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.
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.
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
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.
In ImmutableDrips
import {
ERC721,
ERC721Burnable
} from "openzeppelin-contracts/token/ERC721/extensions/ERC721Burnable.sol";
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.
Usage of custom Solidity error types instead of string revert message can reduce gas costs.
maxEndCurrently, 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.
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.
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
_msgSender() == Charly)_msgSender()==Bob)_msgSender()==Alice)_msgSender() == Alice)Discussion
calledAs feature is really needed or it can be removed to reduce the complexity.callAs pattern could be always later implemented on top by other parties if really needed.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.
Why do we allow multiple entries of the same stream in one receiver config?
_verifyDripsHistory iterates the drips once to verify and then again for the squeeze algorithm
history.length < senderDripsConfigs + 1 => previous configs which have no impact on the current cycle
-
Any changes
Be notified of any changes
-
Mention me
Be notified of mention me
-
Unsubscribe
Subscribe