Potential issue with `Vault.depost` function

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/Vault.sol#L59-L68 https://github.com/code-423n4/2023-01-astaria/blob/main/src/AstariaRouter.sol#L522-L542

Vulnerability details

Impact

Contract Vault is a private vault which only allows the owner (also the strategist) to deposit. However, Vault.deposit uses an unnecessary complicated logic require(s.allowList[msg.sender] && receiver == owner()); to allow only owner to deposit (actually the require statement can be simplified by require(msg.sender == owner); which will do the trick). This may have potential issue in the future. Because the allowList for a private vault is actually meaningless since all allowList-related functions are disabled on private vaults. Currently, the owner address is set in the allowlist when a private vault is created via AstariaRouter.newVault (i.e. allowList[0] = msg.sender;). However, this allowlist setting might be removed in the future as it's meaningless for a private vault. When it's removed, Vault.deposit won't work any more due to require(s.allowList[msg.sender]).

Proof of Concept

In the orginal Vault.deposit function, require(s.allowList[msg.sender] && receiver == owner()); is equivalent to require(msg.sender == owner);. This is because:

1. s.allowList[msg.sender] == true is valid only when msg.sender == owner() since when the private vault is created, only the msg.sender (i.e. the owner of the vault) is set in the allowList. Please refer to this code line for details. The allowList of a private vault has only one element which is the owner.
2. receiver == owner() just requires that the input argument receiver must be set to the owner. Since receiver is not actually used elsewhere in the function body, receiver == owner() can be omitted. This is because s.allowList[msg.sender] == true has already guaranteed the msg.sender must be the owner.

The allowList for private vaults is only used at two places which are the above mentioned two functions: Vault.depost and AstariaRouter.newVault. So the proposed optimizations in the below are safe to apply.

Tools Used

Manual review

Recommended Mitigation Steps

Step A: To optimize the Vault.deposit function as in the below

File: main/src/Vault.sol

59    function deposit(uint256 amount, address /* receiver */)  //@audit: Comment `receiver` as it is not used in the function
60      public
61      virtual
62      returns (uint256)
63    {
64      //@audit: This line is removed
65      require (msg.sender == owner(), "Only Owner Can Deposit");  //@audit: can use costom error to further save gas
66      ERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);
67      return amount;
68:   }

The benefits of the above optimization:

1. Improved readability;
2. Simplified logic makes it easier for maintenance;
3. Saved much gas, and can save more if custom error is used;
4. More robust, it won't be affected by the allowList future change.

Step B: To optimize the AstariaRouter.newVault function

Link to orginal code

Since Vault.deposit has been optimized as above, the allowList for private vaults can be safely "removed" since it's never used anywhere for private vaults.

The optimized code (to create private vault):

File: main/src/AstariaRouter.sol

522   function newVault(address delegate, address underlying)
        external
        whenNotPaused
        returns (address)
      {
527     address[] memory allowList = new address[](0);  //@audit: Set array length to zero
528     // allowList[0] = msg.sender;  //@audit: This line is removed
        RouterStorage storage s = _loadRouterSlot();

        return
          _newVault(
            s,
            underlying,
            uint256(0),
            delegate,
            uint256(0),
            true,
            allowList,
            uint256(0)
          );
542   }

The optimization saves much gas.

[c4-judge]

Picodes marked the issue as unsatisfactory: Overinflated severity

[Picodes]

Should be in QA or Gas as it's only a matter of optimization