fix(ctp): Drippie Spearbit audit fixes (#3280)

* fix(ctp): Drippie Spearbit issue 45

Fixes Spearbit issue 45, saves gas by using calldata parameters instead
of memory parameters.

* fix(ctp): Drippie Spearbit issue 44

Fixes Spearbit issue 44, documents the count variable and increments
count before external calls.

* fix(ctp): Drippie Spearbit issue 42

Fixes Spearbit issue 42, saves gas by removing extra SLOADs.

* fix(ctp): Drippie Spearbit issue 35

Fixes Spearbit issue 35, corrects contract layout ordering.

* fix(ctp): Drippie Spearbit issue 34

Fixes Spearbit issue 34, adds natspec where incomplete.

* fix(ctp): Drippie Spearbit issue 32 and 33

Fixes Spearbit issues 32 and 33, clarifies the behavior of the
executable function to revert instead of returning false, and removes an
unnecessary check as a result.

* fix(ctp): Drippie Spearbit issue 31

Fixes Spearbit issue 31, requires explicit opt-in for reentrant drip
execution.

* fix(ctp): Drippie Spearbit issue 28

Fixes Spearbit issue 28, better documentation of the behavior of
execution checks in the drip function.

* fix(ctp): Drippie Spearbit issue 21

Fixes Spearbit issue 21, use MIT licensed version of Solmate.

* fix(ctp): Drippie Spearbit issue 25

Fixes Spearbit issue 25, reorders DripStatus enum for clarity.

* fix(ctp): Drippie Spearbit issue 24

Fixes Spearbit issue 24, use call with value over transfer to avoid
future gas issues.

* fix(ctp): Drippie Spearbit issue 22

Fixes Spearbit issue 22, removes unnecessary gas parameter.

* fix(ctp): Drippie Spearbit issue 39

Fixes Spearbit issue 39, updates Solidity to latest version.

Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>

.changeset/chilled-bananas-count.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Drippie Spearbit audit fix for issues #32 and #33, clarify behavior of executable function

.changeset/clean-eels-pretend.md

@@ -0,0 +1,6 @@
+---
+'@eth-optimism/contracts-periphery': patch
+'@eth-optimism/drippie-mon': patch
+---
+
+Drippie Spearbit audit fix for issue #25, reorder DripStatus enum for clarity

.changeset/cyan-grapes-fry.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Drippie Spearbit audit fix for issue #44, document drip count and increment before external calls

.changeset/friendly-beds-tell.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Drippie Spearbit audit fix for issue 24, use call over transfer for withdrawETH

.changeset/gold-dots-provide.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Drippie Spearbit audit fix for issue 22, remove unnecessary gas parameter

.changeset/khaki-walls-worry.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Drippie Spearbit audit fix for issue #34, missing natspec

.changeset/nasty-games-jog.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Drippie Spearbit audit fix for issue #28, document dripcheck behavior in drip function

.changeset/poor-ducks-itch.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Drippie Spearbit audit fix #42, remove unnecessary SLOADs in the status function

.changeset/poor-years-smell.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Drippie Spearbit audit fix for issue #39, update to latest version of Solidity

.changeset/selfish-timers-wave.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Drippie Spearbit audit fix for issue #21, use correct version of Solmate

.changeset/stale-crabs-divide.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Drippie Spearbit audit fix for issue #31, require explicit opt-in for reentrant drips

.changeset/tricky-ghosts-provide.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Drippie Spearbit audit fix for issue #45, calldata over memory to save gas

.changeset/wet-humans-divide.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Drippie Spearbit audit fix for issue #35, correct contract layout ordering

packages/contracts-periphery/contracts/universal/AssetReceiver.sol

@@ -14,7 +14,8 @@ contract AssetReceiver is Transactor {
     /**
      * @notice Emitted when ETH is received by this address.
      *
-     * @param from Address that sent ETH to this contract.
+     * @param from   Address that sent ETH to this contract.
+     * @param amount Amount of ETH received.
      */
     event ReceivedETH(address indexed from, uint256 amount);
 
@@ -86,7 +87,7 @@ contract AssetReceiver is Transactor {
      */
     function withdrawETH(address payable _to, uint256 _amount) public onlyOwner {
         // slither-disable-next-line reentrancy-unlimited-gas
-        _to.transfer(_amount);
+        (bool success, ) = _to.call{ value: _amount }("");
         emit WithdrewETH(msg.sender, _to, _amount);
     }
 

packages/contracts-periphery/contracts/universal/Transactor.sol

@@ -18,7 +18,6 @@ contract Transactor is Owned {
      *
      * @param _target Address to call.
      * @param _data   Data to send with the call.
-     * @param _gas    Amount of gas to send with the call.
      * @param _value  ETH value to send with the call.
      *
      * @return Boolean success value.
@@ -27,28 +26,27 @@ contract Transactor is Owned {
     function CALL(
         address _target,
         bytes memory _data,
-        uint256 _gas,
         uint256 _value
     ) external payable onlyOwner returns (bool, bytes memory) {
-        return _target.call{ gas: _gas, value: _value }(_data);
+        return _target.call{ value: _value }(_data);
     }
 
     /**
      * Sends a DELEGATECALL to a target address.
      *
      * @param _target Address to call.
      * @param _data   Data to send with the call.
-     * @param _gas    Amount of gas to send with the call.
      *
      * @return Boolean success value.
      * @return Bytes data returned by the call.
      */
-    function DELEGATECALL(
-        address _target,
-        bytes memory _data,
-        uint256 _gas
-    ) external payable onlyOwner returns (bool, bytes memory) {
+    function DELEGATECALL(address _target, bytes memory _data)
+        external
+        payable
+        onlyOwner
+        returns (bool, bytes memory)
+    {
         // slither-disable-next-line controlled-delegatecall
-        return _target.delegatecall{ gas: _gas }(_data);
+        return _target.delegatecall(_data);
     }
 }

packages/contracts-periphery/contracts/universal/drippie/Drippie.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-pragma solidity 0.8.15;
+pragma solidity 0.8.16;
 
 import { AssetReceiver } from "../AssetReceiver.sol";
 import { IDripCheck } from "./IDripCheck.sol";
@@ -21,14 +21,14 @@ contract Drippie is AssetReceiver {
      * @notice Enum representing different status options for a given drip.
      *
      * @custom:value NONE     Drip does not exist.
-     * @custom:value ACTIVE   Drip is active and can be executed.
      * @custom:value PAUSED   Drip is paused and cannot be executed until reactivated.
+     * @custom:value ACTIVE   Drip is active and can be executed.
      * @custom:value ARCHIVED Drip is archived and can no longer be executed or reactivated.
      */
     enum DripStatus {
         NONE,
-        ACTIVE,
         PAUSED,
+        ACTIVE,
         ARCHIVED
     }
 
@@ -45,6 +45,7 @@ contract Drippie is AssetReceiver {
      * @notice Represents the configuration for a given drip.
      */
     struct DripConfig {
+        bool reentrant;
         uint256 interval;
         IDripCheck dripcheck;
         bytes checkparams;
@@ -123,7 +124,7 @@ contract Drippie is AssetReceiver {
      * @param _name   Name of the drip.
      * @param _config Configuration for the drip.
      */
-    function create(string memory _name, DripConfig memory _config) external onlyOwner {
+    function create(string calldata _name, DripConfig calldata _config) external onlyOwner {
         // Make sure this drip doesn't already exist. We *must* guarantee that no other function
         // will ever set the status of a drip back to NONE after it's been created. This is why
         // archival is a separate status.
@@ -132,9 +133,25 @@ contract Drippie is AssetReceiver {
             "Drippie: drip with that name already exists"
         );
 
+        // Validate the drip interval, only allowing an interval of zero if the drip has explicitly
+        // been marked as reentrant. Prevents client-side bugs making a drip infinitely executable
+        // within the same block (of course, restricted by gas limits).
+        if (_config.reentrant) {
+            require(
+                _config.interval == 0,
+                "Drippie: if allowing reentrant drip, must set interval to zero"
+            );
+        } else {
+            require(
+                _config.interval > 0,
+                "Drippie: interval must be greater than zero if drip is not reentrant"
+            );
+        }
+
         // We initialize this way because Solidity won't let us copy arrays into storage yet.
         DripState storage state = drips[_name];
         state.status = DripStatus.PAUSED;
+        state.config.reentrant = _config.reentrant;
         state.config.interval = _config.interval;
         state.config.dripcheck = _config.dripcheck;
         state.config.checkparams = _config.checkparams;
@@ -157,60 +174,63 @@ contract Drippie is AssetReceiver {
      * @param _name   Name of the drip to update.
      * @param _status New drip status.
      */
-    function status(string memory _name, DripStatus _status) external onlyOwner {
+    function status(string calldata _name, DripStatus _status) external onlyOwner {
         // Make sure we can never set drip status back to NONE. A simple security measure to
         // prevent accidental overwrites if this code is ever updated down the line.
         require(
             _status != DripStatus.NONE,
             "Drippie: drip status can never be set back to NONE after creation"
         );
 
+        // Load the drip status once to avoid unnecessary SLOADs.
+        DripStatus curr = drips[_name].status;
+
         // Make sure the drip in question actually exists. Not strictly necessary but there doesn't
         // seem to be any clear reason why you would want to do this, and it may save some gas in
         // the case of a front-end bug.
         require(
-            drips[_name].status != DripStatus.NONE,
-            "Drippie: drip with that name does not exist"
+            curr != DripStatus.NONE,
+            "Drippie: drip with that name does not exist and cannot be updated"
         );
 
         // Once a drip has been archived, it cannot be un-archived. This is, after all, the entire
         // point of archiving a drip.
         require(
-            drips[_name].status != DripStatus.ARCHIVED,
-            "Drippie: drip with that name has been archived"
+            curr != DripStatus.ARCHIVED,
+            "Drippie: drip with that name has been archived and cannot be updated"
         );
 
         // Although not strictly necessary, we make sure that the status here is actually changing.
         // This may save the client some gas if there's a front-end bug and the user accidentally
         // tries to "change" the status to the same value as before.
         require(
-            drips[_name].status != _status,
-            "Drippie: cannot set drip status to same status as before"
+            curr != _status,
+            "Drippie: cannot set drip status to the same status as its current status"
         );
 
         // If the user is trying to archive this drip, make sure the drip has been paused. We do
         // not allow users to archive active drips so that the effects of this action are more
         // abundantly clear.
         if (_status == DripStatus.ARCHIVED) {
             require(
-                drips[_name].status == DripStatus.PAUSED,
-                "Drippie: drip must be paused to be archived"
+                curr == DripStatus.PAUSED,
+                "Drippie: drip must first be paused before being archived"
             );
         }
 
         // If we made it here then we can safely update the status.
         drips[_name].status = _status;
-        emit DripStatusUpdated(_name, _name, drips[_name].status);
+        emit DripStatusUpdated(_name, _name, _status);
     }
 
     /**
      * @notice Checks if a given drip is executable.
      *
      * @param _name Drip to check.
      *
-     * @return True if the drip is executable, false otherwise.
+     * @return True if the drip is executable, reverts otherwise.
      */
-    function executable(string memory _name) public view returns (bool) {
+    function executable(string calldata _name) public view returns (bool) {
         DripState storage state = drips[_name];
 
         // Only allow active drips to be executed, an obvious security measure.
@@ -245,18 +265,18 @@ contract Drippie is AssetReceiver {
      *         signal that the drip should be executable according to the drip parameters, drip
      *         check, and drip interval. Note that drip parameters are read entirely from the state
      *         and are not supplied as user input, so there should not be any way for a
-     *         non-authorized user to influence the behavior of the drip.
+     *         non-authorized user to influence the behavior of the drip. Note that the drip check
+     *         is executed only **once** at the beginning of the call to the drip function and will
+     *         not be executed again between the drip actions within this call.
      *
      * @param _name Name of the drip to trigger.
      */
-    function drip(string memory _name) external {
+    function drip(string calldata _name) external {
         DripState storage state = drips[_name];
 
-        // Make sure the drip can be executed.
-        require(
-            executable(_name) == true,
-            "Drippie: drip cannot be executed at this time, try again later"
-        );
+        // Make sure the drip can be executed. Since executable reverts if the drip is not ready to
+        // be executed, we don't need to do an assertion that the returned value is true.
+        executable(_name);
 
         // Update the last execution time for this drip before the call. Note that it's entirely
         // possible for a drip to be executed multiple times per block or even multiple times
@@ -265,6 +285,11 @@ contract Drippie is AssetReceiver {
         // block (since this will then prevent re-entrancy).
         state.last = block.timestamp;
 
+        // Update the number of times this drip has been executed. Although this increases the cost
+        // of using Drippie, it slightly simplifies the client-side by not having to worry about
+        // counting drips via events. Useful for monitoring the rate of drip execution.
+        state.count++;
+
         // Execute each action in the drip. We allow drips to have multiple actions because there
         // are scenarios in which a contract must do multiple things atomically. For example, the
         // contract may need to withdraw ETH from one account and then deposit that ETH into
@@ -297,7 +322,6 @@ contract Drippie is AssetReceiver {
             );
         }
 
-        state.count++;
         emit DripExecuted(_name, _name, msg.sender, block.timestamp);
     }
 }

packages/contracts-periphery/contracts/universal/drippie/IDripCheck.sol

@@ -7,5 +7,12 @@ interface IDripCheck {
     // possible to easily encode parameters on the client side. Solidity does not support generics
     // so it's not possible to do this with explicit typing.
 
+    /**
+     * @notice Checks whether a drip should be executable.
+     *
+     * @param _params Encoded parameters for the drip check.
+     *
+     * @return Whether the drip should be executed.
+     */
     function check(bytes memory _params) external view returns (bool);
 }

packages/contracts-periphery/contracts/universal/drippie/dripchecks/CheckBalanceHigh.sol

@@ -8,12 +8,21 @@ import { IDripCheck } from "../IDripCheck.sol";
  * @notice DripCheck for checking if an account's balance is above a given threshold.
  */
 contract CheckBalanceHigh is IDripCheck {
-    event _EventToExposeStructInABI__Params(Params params);
     struct Params {
         address target;
         uint256 threshold;
     }
 
+    /**
+     * @notice External event used to help client-side tooling encode parameters.
+     *
+     * @param params Parameters to encode.
+     */
+    event _EventToExposeStructInABI__Params(Params params);
+
+    /**
+     * @inheritdoc IDripCheck
+     */
     function check(bytes memory _params) external view returns (bool) {
         Params memory params = abi.decode(_params, (Params));
 

packages/contracts-periphery/contracts/universal/drippie/dripchecks/CheckBalanceLow.sol

@@ -8,12 +8,21 @@ import { IDripCheck } from "../IDripCheck.sol";
  * @notice DripCheck for checking if an account's balance is below a given threshold.
  */
 contract CheckBalanceLow is IDripCheck {
-    event _EventToExposeStructInABI__Params(Params params);
     struct Params {
         address target;
         uint256 threshold;
     }
 
+    /**
+     * @notice External event used to help client-side tooling encode parameters.
+     *
+     * @param params Parameters to encode.
+     */
+    event _EventToExposeStructInABI__Params(Params params);
+
+    /**
+     * @inheritdoc IDripCheck
+     */
     function check(bytes memory _params) external view returns (bool) {
         Params memory params = abi.decode(_params, (Params));