Valinity Contract Audit

In the contract `RegistrarMigrator` the role `_adminAgents` has authority over the functions shown in the diagram below. Any compromise to the `_adminAgents` account may allow the hacker to take advantage of this authority and start migrating tokens.  ----- In the contract `VETHReverseStakingTreasury` the role `_adminGovAgents` has authority over the function shown in the diagram below. Any compromise to the `_adminGovAgents` account may allow the hacker to take advantage of this authority and migrate tokens to another address.  In the contract `VETHReverseStakingTreasury` the role `_backendAdminAgents` has authority over the functions shown in the diagram below. Any compromise to the `_backendAdminAgents` account may allow the hacker to take advantage of this authority and set the initial reverse stake term and the maximum reverse stakes for an address.  In the contract `VETHReverseStakingTreasury` the role `_backendAgents` has authority over the functions shown in the diagram below. Any compromise to the `_backendAgents` account may allow the hacker to take advantage of this authority and close offers, burn accrued VY tokens, and migrate reverse stakes.  In the contract `VETHReverseStakingTreasury` the role `_governance` has authority over the functions shown in the diagram below. Any compromise to the `_governance` account may allow the hacker to take advantage of this authority and set the migration address and create a new reverse stake term.  In the contract `VETHReverseStakingTreasury` the role `_registrar` has authority over the function shown in the diagram below. Any compromise to the `_registrar` account may allow the hacker to take advantage of this authority and update critical addresses.  ----- In the contract `BackendAgent` the role `_backendAdminAgents` has authority over the functions shown in the diagram below. Any compromise to the `_backendAdminAgents` account may allow the hacker to take advantage of this authority and grant and revoke the `_backendAgents` role.  ----- In the contract `VETHP2P` the role `_backendAgents` has authority over the function shown in the diagram below. Any compromise to the `_backendAgents` account may allow the hacker to take advantage of this authority and close offers.  In the contract `VETHP2P` the role `_registrar` has authority over the functions shown in the diagram below. Any compromise to the `_registrar` account may allow the hacker to take advantage of this authority and update the `_vethRevenueCycleTreasury` and initiate the trading pairs.  ----- In the contract `VETHRevenueCycleTreasury` the role `_adminGovAgents` has authority over the function shown in the diagram below. Any compromise to the `_adminGovAgents` account may allow the hacker to take advantage of this authority and migrate tokens to another address.  In the contract `VETHRevenueCycleTreasury` the role `_backendAgents` has authority over the functions shown in the diagram below. Any compromise to the `_backendAgents` account may allow the hacker to take advantage of this authority and create and close offers. ![](https://accelerator-tasks-prod.acc.corp.certik.com/11ee-6134-c9136830-a27f-212746946fd3/diagrams/centralization_VETHRevenueCycleT

In the contract `Router`, 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.

When transferring deflationary ERC20 tokens, the input amount may not be equal to the received amount due to the charged transaction fee. For example, if a user sends 100 deflationary tokens (with a 10% transaction fee), only 90 tokens actually arrived to the contract. However, a failure to discount such fees may allow the same user to withdraw 100 tokens from the contract, which causes the contract to lose 10 tokens in such a transaction. With the `VY Token`, if neither the sender nor the receiver is designated as an "agent", the token applies its fee. Within the `VETHP2P` contract, the `createOffer()` function triggers a token transfer, potentially leading to the contract receiving less tokens than expected. If these deductions persist, the contract may lack the necessary balance for functions such as `tradeOffer()`. Reference: https://thoreum-finance.medium.com/what-exploit-happened-today-for-gocerberus-and-garuda-also-for-lokum-ybear-piggy-caramelswap-3943ee23a39f ```solidity=126 _createOffer(tradingPair, quantity, price); ``` - Transferring tokens by `quantity`. - This function call executes the following operation. ```solidity=125 makerAsset.permit(_msgSender(), address(this), quantity, v, r, s); ``` ```solidity=145 if (tradingPair == TradingPairs.VY_ETH) { ERC20 token = _getSpendingTokenAndCheck(_tradingPairs[tradingPair].makerAssetAddress, quantity); token.transferFrom(_msgSender(), address(this), quantity); } ``` - The `quantity` appears to be used for bookkeeping purposes without compensating the potential transfer fees. The same issue also applies to `VETHRevenueCycleTreasury` and `VETHReverseStakingTreasury`.

A denial-of-service (DoS) attack overwhelms a system, rendering it inaccessible to intended users. In the context of smart contracts, DoS exploits vulnerabilities in the contract's logic or design, hindering its functions, potentially locking up funds or disrupting its normal operations. The `VETHGovernance` contract governs essential operations related to address updates and asset migration. An "_adminGovAgents" account can start a proposal, which is subsequently subjected to a voting process where token holders cast their votes. When a proposal expires and the half or more votes are 'yes', any of the "_adminGovAgents" can execute the proposal. A feature of the design is that the execution function can only execute the current proposal (for instance, when the proposal nonce is at 1). This leaves a vulnerability that makes it susceptible to DoS attacks. If an "_adminGovAgents" account is compromised, an attacker could exploit this by deliberately submitting a new proposal (with a higher nonce) before a previously approved proposal (with a lower nonce) gets executed. This vulnerability can lead to a denial of service attack on the governance system, effectively freezing any proposal that the attacker does not want to be executed. This malicious activity can cause delays in essential governance actions, creating system-wide inefficiencies, and undermining the credibility of the governance mechanism.

The transaction may be charged fees, so the max holding of the receiver should be the sum of the held tokens and the amount received from the transfer. The fees should not be included in the max holding. ```solidity=121 uint256 fee = _calculateTransferFee(sender, recipient, amount); ``` - Receiving amount is adjusted. ```solidity=234 _balances[to] += amount; ``` - Balance is increased by the adjusted amount. --- ```solidity=352 require(_vyToken.balanceOf(address(this)) + stakedVY <= circulation, "Total staked VY cannot exceed circulation"); ``` - Max holding is checked. ```solidity=127 super.transferFrom(sender, recipient, amount - fee); // transfers amount - fee to recipient ``` - Receiving amount is adjusted. ```solidity=234 _balances[to] += amount; ``` - Balance is increased by the adjusted amount.

The `VYToken` contract allows for the setting of the transfer fee rate without an upper limit. This design flaw enables setting the transfer fee to any arbitrary value, up to 100%, which exposes the token holders to severe financial risk and makes the token susceptible to becoming a honeypot. ```solidity=139 function setTransferFee(uint256 fee) external onlyAdminAgents { _transferFee = fee; } ```

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. In the `_tradeOffer()` function, once an offer's ETH quantity falls below the threshold defined by `MINIMUM_AUTOCLOSE_IN_ETH`, it should ideally no longer be available for trading. However, the possibility of reentrancy emerges due to the external calls made by the `_transfer()` function on lines 229 and 230. This call will trigger the `receive()` function within the caller's address, in which a malicious caller could make a recursive call back to `tradeOffer()` before the state change of contract has been finalized. This vulnerability might also result in events being emitted out of sequence, with subsequent trades broadcasting events prior to initial trades. ```solidity=191 function _tradeOffer(uint256 id, address seller, uint256 quantity) private returns (TradeOfferCalcInfo memory) { TradingPair memory tradingPair = _tradingPairs[_offers[seller][id].tradingPair]; uint256 maxInput = _offers[seller][id].quantity * _offers[seller][id].price / MULTIPLIER; require(quantity <= maxInput, "Not enough to sell"); /// @dev returns maker quantity fulfilled by this trade TradeOfferCalcInfo memory calc = _calcTradeOffer(tradingPair, quantity, _offers[seller][id].price); // Update offer quantity require(_offers[seller][id].quantity >= calc.amountOut, "Bad calculations"); _offers[seller][id].quantity -= calc.amountOut; // VY_ETH trade if (tradingPair.takerAssetAddress == address(0)) { ERC20 makerAsset = ERC20(tradingPair.makerAssetAddress); // Transfer taker ETH _transfer(seller, calc.makerReceives); _ethComptroller.route{ value: calc.makerFee }(); // Transfer maker VY makerAsset.transfer(_msgSender(), calc.takerReceives); makerAsset.transfer(tradingPair.makerTreasuryAddress, calc.takerFee); } else { // ETH_VY trade ERC20 takerAsset = _getSpendingTokenAndCheck(tradingPair.takerAssetAddress, quantity); /** * Transfer taker VY * * @dev the code below transfers makerReceives from taker to contract, then from contract to maker * instead of transferring makerReceives directly from taker to maker, is to avoid user transfer fee * being applied to (See ticket-296 for more info) */ takerAsset.transferFrom(_msgSender(), address(this), calc.makerReceives); takerAsset.transfer(seller, calc.makerReceives); takerAsset.transferFrom(_msgSender(), tradingPair.takerTreasuryAddress, calc.makerFee); // Transfer maker ETH _transfer(_msgSender(), calc.takerReceives); _ethComptroller.route{ value: calc.takerFee }(); } // ETH_VY market - selling amount in ETH < MINIMUM_AUTOCLOSE_IN_ETH bool makerCloseout = (tradingPair.makerAssetAddress == address(0) && _offers[seller][id].quantity < MINIMUM_AUTOCLOSE_IN_ETH); // VY_ETH market - converted selling amount in VY to ETH < MINIMUM_AUTOCLOSE_IN_ETH bool takerCloseout = (tradingPair.takerAssetAddress == address(0) && _offers[seller][id].quantity * _offers[seller][id].price / MULTIPLIER < MINIMUM_AUTOCLOSE_IN_ETH); if (makerCloseout || takerCloseout) { _closeOffer(id, seller); // Auto-close when selling amount in ETH < MINIMUM_AUTOCLOSE_IN_ETH } emit TradeOffer(id, _msgSender(), calc.amountOut, quantity, _offers[seller][id].quantity, block.timestamp); return calc; } ```

In the current contract logic, when doing the `VY_ETH` trade, the `primaryStakeholder` address (defined in `Router.sol`) need to receive native tokens as fees. If this address is malicious contract or have incorrectly implemented the payment receive feature, it can cause the contracts "VETHP2P" ,"VETHReverseStakingTreasury", and "VETHRevenueCycleTreasury" to malfunction (e.g. TradeOffer transaction may revert). The implementation of all the "primaryStakeholder" instances is out of the scope of the audit.