Drips Protocol contest - 0xA5DF's results

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Caller.sol#L104-L119

Vulnerability details

Impact

The Caller contract enables users to authorize other users to execute tx on their behalf. This option enables the authorized/delegated user to add more users to the authorized users list. In case the original user is trying to remove an authorized user (i.e. run unauthorize()), the delegated user can simply front run that tx to add another user, then after unauthorized() is executed the delegated can use the added user to gain his authority back.

This would allow the malicious delegated user to keep executing txs on behalf of the original user and cause them a loss of funds (e.g. collecting funds on their behalf and sending it to the attacker's address).

Proof of Concept

The test below demonstrates such a scenario:

• Alice authorizes Bob
• Bob becomes malicious and Alice now wants to remove him
• Bob noticed the tx to unauthorize him and front runs it by authorizing Eve
• Alice unauthorize() tx is executed
• Bob now authorizes himself back again via Eve's account

Front running can be done either by sending a tx with a higher gas price (usually tx are ordered in a block by the gas price / total fee), or by paying additional fee to the validator if they manage to run their tx without reverting (i.e. by sending additional ETH to block.coinbase, hoping validator will notice it). It's true that Alice can run unauthorize() again and again and needs to succeed only once, but:

• Bob can keep adding other users and Alice would have to keep removing them all
• This is an ongoing battle that can last forever, and Alice might not have enough knowledge, resources and time to deal with it right away. This might take hours or days, and in the meanwhile Alice might be receiving a significant amount of drips that would be stolen by Bob.

diff --git a/test/Caller.t.sol b/test/Caller.t.sol
index 861b351..3e4be22 100644
--- a/test/Caller.t.sol
+++ b/test/Caller.t.sol
@@ -125,6 +125,24 @@ contract CallerTest is Test {
         vm.expectRevert(ERROR_UNAUTHORIZED);
         caller.callAs(sender, address(target), data);
     }
+    
+    function testFrontRunUnauthorize() public {
+        bytes memory data = abi.encodeWithSelector(target.run.selector, 1);
+        address bob = address(0xbab);
+        address eve = address(0xefe);
+        // Bob is authorized by Alice
+        authorize(sender, bob);
+
+        // Bob became malicious and Alice now wants to remove him
+        // Bob sees the `unauthorize()` call and front runs it with authorizing Eve
+        authorizeCallAs(sender, bob, eve);
+
+        unauthorize(sender, bob);
+
+        // Eve can now give Bob his authority back
+        authorizeCallAs(sender, eve, bob);
+
+    }
 
     function testAuthorizingAuthorizedReverts() public {
         authorize(sender, address(this));
@@ -257,6 +275,13 @@ contract CallerTest is Test {
         assertTrue(caller.isAuthorized(authorizing, authorized), "Authorization failed");
     }
 
+    function authorizeCallAs(address originalUser,address delegated, address authorized) internal {
+        bytes memory data = abi.encodeWithSelector(Caller.authorize.selector, authorized);
+        vm.prank(delegated);
+        caller.callAs(originalUser, address(caller),  data);
+        assertTrue(caller.isAuthorized(originalUser, authorized), "Authorization failed");
+    }
+
     function unauthorize(address authorizing, address unauthorized) internal {
         vm.prank(authorizing);
         caller.unauthorize(unauthorized);

Recommended Mitigation Steps

Don't allow authorized users to call authorize() on behalf of the original user. This can be done by replacing _msgSender() with msg.sender at authorize(), if the devs want to enable authorizing by signing I think the best way would be to add a dedicated function for that (other ways to prevent calling authorize() from callAs() can increase gas cost for normal calls, esp. since we'll need to cover all edge cases of recursive calls with callBatched()).

User might loose ETH in callBatched() if they send more than needed

callBatched() doesn't verify that the amount of value the user sent matches the amount of value. In case a user sent more ETH than spent in the calls it can be stolen by another user. Consider the following scenario:

• Bob wants to drip WETH to some receivers, so he sends some ETH to convert it to WETH
• Bob intended to convert 0.1 ETH, but set the msg.value to 1 ETH
• Alice operates a bot that monitors the Caller address, and right away executes via callBatched() a call that would simply transfer the remaining 0.9 ETH to her As a result, Bob lost 0.9 ETH do to a typo

Mitigation

Verify that msg.value is equal to the total value used in the batched calls. You can also add an unsafe version callBatchedUnsafe() for users who want to save that extra gas, or for users who are worried that an insignificant difference would cause a revert.

FoT and rebase tokens

It should be noted that those kind of tokens can cause issues, since the contract might end up having less balance than it assumes it has, and the last users to collect their funds wouldn't be able to do it.

Using uint32 for timestamp might cause issues at 2106

This has been raised in past contest and isn't considered a significant issue (since projects usually don't care about 85 years from now), but it's worth mentioning it. (this would cause issues mostly about redeeming old drips after that timestamp has past)

At ImmutableSplitsDriver if user sends to himself the funds will be stuck forever

Not sure to what extent the protocol needs to protect users from dumb mistakes, but it might be worth adding a check at ImmutableSplitsDriver.createSplits() that the user isn't sending to himself, since in that case there will always be some funds that will be stuck in the user's splittable balance.

NFTDriver.burn() might cause the user to loose access to funds

The burn() function here doesn't seem to be very useful, the only case it can be useful is to make a permanent splittable receivers, and for that we have already ImmutableSplitsDriver. Consider removing it, or adding checks that the user splits config sends it all to other users.

Use a 'bitmap' to mark which amtDeltas entries have non-zero values

This one is significant and can save millions of gas units, esp. if the protocol uses a short cycle. The only downside here is that it will increase the gas cost of setting drips.

The idea is taken from Uniswap V3. The idea is to use a uint256 variable to mark which amtDelta entries are 'full' (i.e. have non-zero values). This way, if we call receiveDrips() over 1000 cycles and only 8 of them have deltas - instead of reading 1000 slots we need to read only 12 slots (8 deltas + 4 bitmap entries). In other words 25.2K gas instead of 2.1M (!). (To be more precise the deletion of the entries gives us some gas refund, so we'll actually by paying ~2.4K instead of ~2.9M)

From sender's point the gas will increase by the following (per amtDelta written):

• If the amtDelta entries are already non-zero - than it'd cost only 100 gas units (there's no need to read the bitmap entry, just to read the amtDelta entry before writing to it)
• If the amtDelta entry is zero:
  • If the bitmap entry is non-zero it'll cost 5K
    • Unless that entry was already modified in the current tx, in that case it'll be only 100
  • If the bitmap entry is zero it'll cost 22.1K

Since each receiver adds 1 delta range that modifies 2 amtDelta entries that means 200 to 44.2K gas units per receiver. On the average case though it'd probably be 200-10K. The more activity the receiving user will have the more it'll decrease the cost for the senders (senders will probably also converge around the cycles of other users in order to save gas for them).

On the other hand, for receivers with less activity it might be costly for the senders but will also save much more for receivers (since they have more amtDelta empty entries).

If a cycle of a week will be chosen it probably wouldn't be that relevant, but in case of shorter cycles (1 hour or 1 day) it can save lots of gas.

Don't delete empty amtDelta entries

Gas saved: 100 units times the amount of empty entries

(this is in case the above suggestion isn't accepted) At _receiveDrips() all old entries are deleted, even the empty ones. This imposes an additional 100 gas units cost per empty entry. I think the simplest solution might be to implement a non-view version of _receiveDripsResult() that will delete non-empty entries.

Read amtDelta to memory

Gas saved: ~100 units per cycle

At _receiveDripsResult() each amtDelta entry is simply a single slot, reading the different fields of this struct separately takes an additional sload instruction that costs about 100 gas units (plus some additional cost to calculate the slot value). Reading the whole struct to memory and reading from there can save that gas.

         for (uint32 cycle = fromCycle; cycle < toCycle; cycle++) {
-            amtPerCycle += state.amtDeltas[cycle].thisCycle;
+            AmtDelta memory amtDelta = state.amtDeltas[cycle];
+            amtPerCycle += amtDelta.thisCycle;
             receivedAmt += uint128(amtPerCycle);
-            amtPerCycle += state.amtDeltas[cycle].nextCycle;
+            amtPerCycle += amtDelta.nextCycle;
         }

Note: this should also be relevant to the _addDelta(), but for some reason the test results didn't show a decrease in gas cost. I didn't have the time to get the bottom of it and it might be worth looking into it.

Cache state.nextReceivableCycle

state.nextReceivableCycle can be read twice here. It should be cached to memory first in order to save ~100 gas units.

Have a built-in address driver

Using any driver costs an additional sload per tx - since it requires the DripsHub contract to check the driverAddresses map + the cost of calling an additional contract (calling a cold address - 2.1K units). It may be worth to embed the functionality of common drivers like the address driver (reserve a driver ID to it, and at _assertCallerIsDriver() send it to the embedded functionality), or alternately register the driver's address in an immutable variable. This might cost a few extra units (probably 10-20, just an additional condition check) for other drivers, but might save a significant amount of gas for common drivers.