Report for issue #116 updated by RaymondFam

data/RaymondFam-G.md

@@ -39,7 +39,9 @@ Here are some of the instances entailed:
     require(s.allowList[msg.sender] && receiver == owner());
 ```
 ## Unneeded function parameter
-In `deposit()` of Vault.sol, the input parameter of `receiver` is only used for comparing with `owner()` and no state change is associated with it. Consider having the code block refactored as follows to save gas both on contract deployment and function calls:
+In `deposit()` of Vault.sol, the input parameter of `receiver` is only used for comparing with `owner()` and no state change is associated with it. Additionally, with the owner the only depositor allowed when having a new vault deployed via [`newVault()`](https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L527-L528) in AstariaRouter.sol, the second condition in the require statement may be omitted to save even more gas on contract deployment and function calls.
+
+Consider having the code block refactored as follows :
 
 [File: Vault.sol#L59-L68](https://github.com/code-423n4/2023-01-astaria/blob/main/src/Vault.sol#L59-L68)
 
@@ -52,14 +54,86 @@ In `deposit()` of Vault.sol, the input parameter of `receiver` is only used for
   {
     VIData storage s = _loadVISlot();
 -    require(s.allowList[msg.sender] && receiver == owner());
-+    require(s.allowList[msg.sender] || msg.sender == owner());
++    require(s.allowList[msg.sender];
     ERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);
     return amount;
   }
 ```
-Note: The second condition in the require statement may be omitted to save even more gas if the owner gets himself enabled via 
-[`modifyAllowList()`](https://github.com/code-423n4/2023-01-astaria/blob/main/src/VaultImplementation.sol#L95-L99).
+## `newVault() and its associated logic could be trimmed
+In `newVault()` of AstariaRouter.sol, since `owner()` is the only depositor allowed in `allowList`, it does not make much sense caching a 1 element array to update the mapping `allowlist`. 
+
+Consider having the unnecessary code lines removed to save gas both on contract deployment and function calls:
+
+[AstariaRouter.sol#L522-L542](https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L522-L542) 
 
+```diff
+  function newVault(address delegate, address underlying)
+    external
+    whenNotPaused
+    returns (address)
+  {
+-    address[] memory allowList = new address[](1);
+-    allowList[0] = msg.sender;
+    RouterStorage storage s = _loadRouterSlot();
+
+    return
+      _newVault(
+        s,
+        underlying,
+        uint256(0),
+        delegate,
+        uint256(0),
+-        true,
++        false,
+-        allowList,
++        /* emptyList */,
+        uint256(0)
+      );
+  }
+```
+With the above trim measure in place, the second if block along with its nested for loop in the `init()` instance of VaultImplementation.sol will be skipped when externally invoked via `IVaultImplementation(vaultAddr).init(IVaultImplementation.InitParams()`, saving even more gas at deployment:
+
+[File: VaultImplementation.sol#L190-L208](https://github.com/code-423n4/2023-01-astaria/blob/main/src/VaultImplementation.sol#L190-L208)
+
+```solidity
+  function init(InitParams calldata params) external virtual {
+    require(msg.sender == address(ROUTER()));
+    VIData storage s = _loadVISlot();
+
+    if (params.delegate != address(0)) {
+      s.delegate = params.delegate;
+    }
+    s.depositCap = params.depositCap.safeCastTo88();
+    if (params.allowListEnabled) {
+      s.allowListEnabled = true;
+      uint256 i;
+      for (; i < params.allowList.length; ) {
+        s.allowList[params.allowList[i]] = true;
+        unchecked {
+          ++i;
+        }
+      }
+    }
+  }
+```
+Lastly, `deposit()` in Vault.sol will need to be refactored as follows to accommodate the trimming changes above:
+
+[File: Vault.sol#L59-L68](https://github.com/code-423n4/2023-01-astaria/blob/main/src/Vault.sol#L59-L68) 
+
+```diff
+-  function deposit(uint256 amount, address receiver)
++  function deposit(uint256 amount)
+    public
+    virtual
+    returns (uint256)
+  {
+    VIData storage s = _loadVISlot();
+-    require(s.allowList[msg.sender] && receiver == owner());
++    require(msg.sender == owner());
+    ERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);
+    return amount;
+  }
+```
 ## `||` costs less gas than its equivalent `&&`
 Rule of thumb: `(x && y)` is `(!(!x || !y))`
 
@@ -102,4 +176,20 @@ For instance, the modifier instance below may be refactored as follows:
 +        _whenNotPaused();
         _;
     }
-```
+```
+## Unneeded event emission
+`setDelegate()` has the event `AllowListUpdated()` emitted without having `delegate_` enabled in the mapping `allowList`.
+
+Consider removing this irrelevant emission to save gas both on contract deployment and function calls:
+
+[File: VaultImplementation.sol#L210-L216](https://github.com/code-423n4/2023-01-astaria/blob/main/src/VaultImplementation.sol#L210-L216)
+
+```diff
+  function setDelegate(address delegate_) external {
+    require(msg.sender == owner()); //owner is "strategist"
+    VIData storage s = _loadVISlot();
+    s.delegate = delegate_;
+    emit DelegateUpdated(delegate_);
+-    emit AllowListUpdated(delegate_, true);
+  }
+```