Vector Reserve

The Vector Reserve protocol shows high centralization and roles with powerful privileges. While manual deposit management is needed for some protocol features by design, users should be aware of these privileges. The protocol owners have full access to deposited funds, so off-chain security is also crucial. The Vector Reserve team should follow good practices for key management of privileged addresses as well as extensive testing before executing transactions modifying critical parameters of the protocol. A feature that requires special mention is the set of rates between deposited LSTs/LRTs and minted vETH. Protocol administrators should have alerts or automatic systems to react in the event of depegs or other price changes to avoid large arbitrage operations when redemptions are enabled. We have identified some potential issues if the privileged "owner" addresses get compromised or used maliciously: 1. The `VectorETH` contract allows deposits of liquid staking tokens (LSTs) as long as they are whitelisted. The state variable `vETHPerRestakedLST` keeps track of the exchange rate used to mint and burn vETH. If there is a depeg from the ETH value on any of the whitelisted LSTs, an attacker could take advantage of an outdated rate and get part of the contract funds. 1. The `VectorETH` contract allows deposits of liquid staking tokens (LSTs) as long as they are whitelisted. The state variable `vETHPerRestakedLST` keeps track of the exchange rate used to mint and burn vETH. According to the team, the rate should normally be 1:1 unless the corresponding LST is generating some yield. However, that variable can be updated by the owner to any arbitrary value which means there is no guarantee that users will get their initial deposit back since the rate could remain outdated if the owner does not update it regularly (or set it to zero). Additionally, a compromised owner could drain the contract completely. 1. The function `removeRestakedLST()` allows the admin to remove the `vETHPerRestakedLST` amount, even if the contract still holds balance for the given LST. This will prevent users from being able to redeem their LST. 1. The function `manageRestakedLST()` allows managers to transfer any staked token to an arbitrary address. Instead of allowing a manager to manage all the deposited tokens, the owner should have the option to assign a manager to manage only specific tokens to preset addresses.

The `VectorETH` contract does not track the rewards each user should get based on the deposited time. This could lead to an attack when updating the variable `vETHPerRestakedLST`. If it is larger than the previous one, the attacker can profit from it by sandwiching the update transaction. For example: let’s say we have two users that deposited when the rate was 1:1, [100 LST => 100vETH, 200 LST => 200 vETH]. Now the team wants to update the rate to be `1 vETH: 1:15 LST`, because of the yield accrued or a depeg, meaning the users should be able to claim [115 LST, 230 LST]. However, an attacker can sandwich attack the update transaction, depositing 300 LST tokens just before the update, and claiming right after returning the 345 LST tokens in the contract, completely draining it.

"Dead" code refers to code which is never executed and hence makes no impact on the final result of running a program. Dead code raises a concern, since either the code is unnecessary or the necessary code's results were ignored. Some examples in the codebase are: <br> 1. `EigenLayerInterface.sol` is empty. Also, `interfaces/eigenlayer/*` files are not used. 2. `Counters` is not used. 3. The import `IERC20Metadata` is not used in the `VECStaking` contract. 4. The import `IStaking` is not used in the `sVEC` contract.

The `sVEC` token implements ERC-20 approval logic, allowing approved users to transfer a certain amount of tokens on behalf of the owner. However, the implementation from OpenZeppelin was overridden: infinite allowance is not supported in the `sVEC` token (i.e. allowance value not decreasing if set to `type(uint256).max`).

The clone-and-own approach involves copying and adjusting open-source code at one's discretion. From the development perspective, it is initially beneficial as it reduces the amount of effort. However, from the security perspective, it involves some risks as the code may not follow the best practices, may contain a security vulnerability, or may include intentionally or unintentionally modified upstream libraries.

The owner of the contracts can call `transferOwnership()` to transfer the ownership to a new address. If an uncontrollable address is accidentally provided as the new owner address then the contract will no longer have an active owner, and functions with the `onlyOwner` modifier can no longer be executed.

The contracts are not designed to accommodate deflationary tokens when accepting token transfers. If a user transfers funds that incur a fee on transfer during order submission, the contract will store that the user has deposited more funds than they sent. This can cause issues during withdrawal or vault deposit. For example, accounting can be wrong for `restakedLSTManaged` or `totalRestakedLSTDeposited`. Similar issues can arise when interacting with ERC-777 tokens, which can include pre and post-transfer hooks, making the attack surface bigger (i.e. wrong accounting through reentrancy attacks).

LSTs and LRTs deposited in the `VectorETH` contract can be withdrawn by approved managers. The withdrawn amounts are tracked in `restakedLSTManaged` mapping. In the event of a user redemption, `redeem()` just validates the amount in `totalRestakedLSTDeposited`, which can be greater than the actual balance of the LST in the contract. This can cause unexpected reverts when trying to transfer the tokens to the user - the contract could be insolvent.

It is important to validate inputs, even if they only come from trusted addresses, to avoid human error. This is a non-exhaustive list showing some of the missing validations found in the codebase: <br> 1. `VectorBonding.constructor()`should validate `_treasury`, `_vETH`, and `_principalToken`. 2. `VectorBonding.setFeeAndFeeTo()` should validate the address `_feeTo` against `address(0)`. 3. `RewardDistributor.constructor()` should validate `_treasury`, `_VEC`, `_vETH`, `_svETH`, and `_vecStaking`. 1. `RewardDistributor.setsvETHReward()`should validate `_newReward`. 2. `RewardDistributor.withdrawToken()` should validate `_to` and `amount`. 4. `StakedVectorETH.constructor()` should validate `_vETH`. 5. `StakedVectorETH.unstake()` should validate `share` to not be zero. 6. `StakedVectorETH.stake()` should validate `amount` to not be zero. 5. `VectorTreasury.constructor()` should validate `_VEC`and `_vETH`. 1. `VectorETH.addRestakedLST()`should validate `_vETHPerLST`.

Contracts that inherit `Ownable` contain the function `renounceOwnership()` that allows the `owner` address to set ownership to `address(0)`. This will leave functions with the `onlyOwner` modifier without an address that can call them.