VNST - audit

In the contract `VNSTProxy`, the role `_owner` has authority over the functions shown in the diagram below. Any compromise to the `_owner` account may allow the hacker to take advantage of this authority and add a moderator role, remove a moderator, pause contract functions, authorize contract upgrade, unpause the contract.  ----- In the contract `VNSTProtocol`, the role `MODERATOR_ROLE` has authority over the functions shown in the diagram below. Any compromise to the `MODERATOR_ROLE` account may allow the hacker to take advantage of this authority and add verified users, remove verified users, set redeem and mint covered amounts, update market and pool variables with validation, set the mint fee, set the redeem fee, set the redeem status, set max mint and redeem limits for verified users, and change mint status.  In the contract `VNSTProtocol`, the role `_owner` has authority over the functions shown in the diagram below. Any compromise to the `_owner` account may allow the hacker to take advantage of this authority and withdraw all USDT funds, withdraw operation pool as fees, set the maximum redeem limit, and set the maximum mint limit. 

In the contract `VNSTProxy`, the role `owner` has the authority to update the implementation contract behind the proxy contract. Any compromise to the `owner` account may allow a hacker to take advantage of this authority and change the implementation contract which is pointed by proxy and therefore execute potential malicious functionality in the implementation contract.

There is confusion due to the name of the `VNSTProxy` contract. Despite its name, it actually functions as the implementation contract rather than the proxy contract. The test file uses the `upgrades.deployProxy()` function, which deploys a real proxy contract and a `VNSTProxy` contract, setting the deployed `VNSTProxy` as the proxy’s implementation contract. In this setup, `VNSTProxy` contract is used as implementation contracts. However, the `VNSTProxy` contract lacks sufficient logic for the entire project and primarily serves as an ERC20 token, so it should not be set as an implementation contract of the proxy contract. ```ts vnstProxy = await hre.upgrades.deployProxy(VNSTProxy, [usdt.target], { kind: "uups", }) ```

The whitepaper states: "For instance, in cases where the difference between the USDT/VNST exchange rate and the market rate exceeds the covered price (0.5%), the VMM will autonomously balance by increasing or decreasing circulating supply immediately to maintain stability and equilibrium in the VNST/USDT exchange rate." The selection of the 0.5% threshold for triggering the Virtual Market Maker (VMM) balancing mechanism lacks a clear rationale. This threshold is crucial because significant fluctuations in the VNST/USDT exchange rate can create arbitrage opportunities across other centralized and decentralized exchanges (CEXs and DEXs). An inadequately justified threshold may lead to unintended market dynamics and impact the token's stability.

When updating upgradeable smart contracts for new features or bug fixes, keeping the state variables' declaration order unchanged is essential to avoid storage layout issues. A practical solution is to include unused state variables or explicitly named storage gaps (like `__gap`) in the base contracts. This foresight allows reserved slots for future use, ensuring that any additions to the contract's state won't disrupt the storage pattern of derived contracts or the compatibility with previously deployed versions. After `ERC-7201`, it is also possible to place all storage variables of a contract into one or more structs like `Namespaced Storage Layout`. The problem of "Lack of Storage Gap Or NameSpaced Storage Layout in Upgradeable Contract" occurs when **these storage gaps are not incorporated into the base contract's logic nor the base contract defines the namespace storage layout**. As a result, if new state variables are added to the base contract, they might overwrite existing variables in the child contracts due to storage slot collisions. **In the current contract, the contract allows for future upgrades and is also inherited by other contracts. However, the storage gap is missing for the the current contract, nor is the `namespaced storage layout` used.** For detailed guidelines and best practices, refer to the following OpenZeppelin documentation: - https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps - https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#storage-gaps

The direct usage of `grantRole` and `revokeRole` functions from the `AccessControlEnumerableUpgradeable` contract in functions with additional access control logic can lead to conflicts or redundant permissions. This is due to the requirement that the caller must have the admin role for the corresponding role to invoke these functions successfully. If the caller's role permissions are not properly set and synchronized with the role administration logic, it can result in inconsistencies and unintended access rights.

Contract `VNSTProtocol` extends `VNSTProxy`, but the extended contract is not initialized by the current contract. Generally, the initializer function of a contract should always call all the initializer functions of the contracts that it extends.

The VNST token contract allows any user to mint VNST tokens by providing USDT. This unrestricted minting poses a significant risk: if not properly managed, the total supply of VNST tokens could reach the maximum `uint256` value. Once this limit is reached, further minting would be impossible, potentially causing significant disruptions to the token's functionality and economy.