audit: EnsoVestingFactory and EnsoVestingWallet

Author: vnavascues

src/EnsoVestingFactory.sol

@@ -1,23 +1,32 @@
 // SPDX-License-Identifier: GPL-3.0-only
 pragma solidity ^0.8.20;
 
-import { SafeERC20, IERC20 } from "openzeppelin-contracts/token/ERC20/utils/SafeERC20.sol";
-import { Ownable } from "openzeppelin-contracts/access/Ownable.sol";
 import { EnsoVestingWallet } from "./EnsoVestingWallet.sol";
+import { Ownable } from "openzeppelin-contracts/access/Ownable.sol";
+import { IERC20, SafeERC20 } from "openzeppelin-contracts/token/ERC20/utils/SafeERC20.sol";
 
+// @audit-info consider making this contract a clone factory for gas efficiency
+// @audit-info 2-step ownership?
 contract EnsoVestingFactory is Ownable {
     using SafeERC20 for IERC20;
 
     IERC20 private immutable _token;
 
     struct VestingOptions {
+        // @audit-info a single uint64 below (and up to 3 x uint48) can be packed with address in the same slot
+        // However, the struct members are never stored on-chain on storage, but as immutables in the created
+        // EnsoVestingWallet
         address beneficiary;
         uint256 amount;
+        // @audit-info consider using `uint48` (safe for timestamps), which will pack 3 timestamps in 12 bytes
+        // However, the struct members are never stored on-chain on storage, but as immutables in the created
+        // EnsoVestingWallet
         uint64 startTimestamp;
         uint64 durationSeconds;
         uint64 cliffSeconds;
     }
 
+    // @audit-info no indexed parameters? Possibly index `wallet` and `beneficiary`
     event VestingWalletCreated(
         address wallet,
         address beneficiary,
@@ -64,4 +73,4 @@ contract EnsoVestingFactory is Ownable {
             options.cliffSeconds
         );
     }
-}
+}

src/EnsoVestingWallet.sol

@@ -7,6 +7,12 @@ pragma solidity ^0.8.20;
 import { Ownable } from "openzeppelin-contracts/access/Ownable.sol";
 import { IERC20, SafeERC20 } from "openzeppelin-contracts/token/ERC20/utils/SafeERC20.sol";
 
+// @audit-info consider making this contract a clone for gas efficiency
+// @audit-info 2-step ownership?
+// @audit-info missing interface with errors and events?
+// @audit-info "total vested Amount" is not stored. Any concern regarding this design choice and that
+// `token.balanceOf(address(this))` can be increased anytime (which modifies `_vestingSchedule` return value)? All OK by
+// me btw!
 contract EnsoVestingWallet is Ownable {
     using SafeERC20 for IERC20;
 
@@ -15,10 +21,13 @@ contract EnsoVestingWallet is Ownable {
 
     uint256 public released;
     bool public revoked;
+    // @audit-info `revoker == address(0)` means non-revocable forever.
     bool public immutable revocable;
 
     IERC20 private immutable _token;
+    // @audit-info revoker immutable? risks?
     address private immutable _revoker;
+    // @audit-info consider using `uint48` (safe for timestamps), which will pack 3 timestamps in 12 bytes
     uint64 private immutable _start;
     uint64 private immutable _duration;
     uint64 private immutable _cliff;
@@ -45,6 +54,7 @@ contract EnsoVestingWallet is Ownable {
         _duration = durationSeconds;
         _cliff = startTimestamp + cliffSeconds;
         _token = token;
+        // @audit-info simplify to `_revoker = revoker` if `address(0)` means non-revocable forever
         if (revoker != address(0)) {
             _revoker = revoker;
             revocable = true;
@@ -69,7 +79,6 @@ contract EnsoVestingWallet is Ownable {
      * @dev Getter for the end timestamp.
      */
     function end() public view returns (uint256) {
-        // @audit return _start + _duration?
         return start() + duration();
     }
 
@@ -98,7 +107,6 @@ contract EnsoVestingWallet is Ownable {
      * @dev Getter for the amount of releasable tokens.
      */
     function releasable() public view returns (uint256) {
-        // @audit return `_vestingSchedule(...)`?
         return vestedAmount(uint64(block.timestamp)) - released;
     }
 
@@ -116,7 +124,9 @@ contract EnsoVestingWallet is Ownable {
      */
     function release() public {
         if (revoked) revert Revoked();
+        // @audit-info call `_vestingSchedule(...)`?
         uint256 amount = releasable();
+        // @audit-info skip checking amount for gas efficiency (and allow 0 transfers)
         if (amount > 0) {
             released += amount;
             emit TokenReleased(amount);
@@ -145,6 +155,10 @@ contract EnsoVestingWallet is Ownable {
      * an asset given its total historical allocation. Returns 0 if the {cliff} timestamp is not met.
      */
     function _vestingSchedule(uint256 totalAllocation, uint64 timestamp) internal view returns (uint256) {
+        // @audit-info if revoked revert with Revoked?
+        // @audit-info if `timestamp < _cliff` revert with BlockTimestampLtCliff(uint256 blockTimestamp, uint256 cliff)
+        // or `CliffNotReached(uint256 blockTimestamp, uint256 cliff)`?
+        // @audit-info no need for `if..else if..else`
         if (revoked || timestamp < cliff()) {
             return 0;
         } else if (timestamp >= end()) {