Haven1 security record

In the contract `UniswapV3Factory` the role owner has authority over the following functions: - function `setOwner()` - function `setRouter` - function `enableFeeAmount()` Any compromise to the owner account may allow a hacker to take advantage of this authority and modify default fee settings for swap pools or modify the whitelisted router address list. ----------------------------------------------------------------------- In the contract `UniswapV3Pool` the role `FactoryOwner` has authority over the following functions: - function `setFeeProtocol()` - function `collectProtocol()` Any compromise to the factory owner account may allow a hacker to take advantage of this authority and modify protocol fee or collect protocol fee. ----------------------------------------------------------------------- In the contract `H1NativeApplication` the role `association` has authority over the following functions: - function `_authorizeFeeContractAddressUpdate()` Any compromise to the `association` account may allow a hacker to take advantage of this authority and modify fee contract addresses through the function `updateFeeContractAddress`. ----------------------------------------------------------------------- In the contract `H1NativeApplication` the role `association` has authority over the following functions: - function `updateSwapRouter()` - function `updateQuoter()` - function `addBaseToken()` - function `removeBaseToken()` Any compromise to the `association` account may allow a hacker to take advantage of this authority and change Router/Quoter address and modify predefined basetoken lists when performing the on-chain swap routing.

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. The contract pays fees to `_feeContract` via a low-level call `(call{value: amount})` before finalizing state updates. Since the `_feeContract` can be modified by the `association` account, if the attacker can control the `_feeContract` address, a malicious fallback function or reentry call could invoke the original contract logic again. This allows the attacker to trigger functions and manipulate state that would otherwise only run once. ```solidity=100 modifier applicationFee(bool payableFunction, bool refundRemainingBalance) { _updateFee(); uint256 fee = _feeContract().getFeeForContract( address(this), msg.sender, msg.sig ); if (msg.value < fee || (address(this).balance < fee)) { revert H1NativeBase__InsufficientFunds(address(this).balance, fee); } H1NativeBaseStorage storage $ = _getH1NativeStorage(); if (payableFunction) $.msgValueAfterFee = (msg.value - fee); _payFee(fee); _; if (refundRemainingBalance && address(this).balance > 0) { _safeTransfer(msg.sender, address(this).balance); } delete $.msgValueAfterFee; } ```

The functions marked as `view` or `pure` are unnecessarily restricted by the `onlyImplementation` modifier. These functions are designed to be read-only, meaning they do not modify the state of the blockchain. However, they are restricted so only a specific address can call them. It's important to note that even private state variables can be read off-chain, rendering the access restriction on these functions ineffective.

The issue arises because the votingStatus mapping within the OrgVoterDetails struct is defined as `mapping(uint256 => mapping(address => bool))`, but everywhere in the code it is accessed using `orgVoterList[id].votingStatus[id]`. This effectively reduces the `mapping` to `mapping(address => bool)` for a single `id`, making the outer `uint256` key redundant. As a result, the current structure wastes storage and adds unnecessary complexity, since the outer mapping serves no functional purpose when it is always accessed with the same key.

Using an outdated or incorrect version of the Solidity compiler can expose the contract to security vulnerabilities and compatibility issues. For example, specifying a version like `pragma solidity ^0.5.3`; restricts the contract to Solidity versions `0.5.3` or higher, but it does not account for important improvements, bug fixes, or security enhancements introduced in later versions. Moreover, this approach may prevent the contract from leveraging safer math operations, which were only introduced in Solidity `0.8.x` to prevent overflows and underflows. It is essential to specify a compiler version that is compatible with the latest stable releases to ensure the contract benefits from the latest features, security checks, and optimizations.

Uncontrolled growth of a list (e.g., dynamic arrays or mappings) without implementing limits or clean-up, and you have a separate function that enumerates or processes all elements in the list, you may encounter a "gas limit exceeded" issue. This occurs because the gas required to iterate through the list grows proportionally with its size, eventually surpassing the block gas limit and making the enumeration function unusable as the list becomes too large.

Defining an internal structure with fields that are never used leads to unnecessary complexity and wasted storage or memory allocation during contract execution. Unused fields in a struct increase the size of the struct and can result in inefficient use of resources, potentially increasing gas costs for operations involving the struct. Moreover, such fields can make the code harder to understand and maintain, as they may confuse developers or imply functionality that does not exist. This issue highlights the importance of reviewing and optimizing struct definitions to include only relevant fields.

In Solidity, using magic numbers (hardcoded numeric values) instead of defined constants reduces code readability and maintainability. Magic numbers do not convey their purpose or meaning, making it difficult for developers to understand the intent of the code. Additionally, if the same value needs to be updated in multiple places, it increases the risk of errors and inconsistencies. Replacing magic numbers with properly named constants improves clarity, ensures consistency, and makes the code easier to maintain and modify.

The `positions()` function is intended to expose details about a specific position. While the function successfully returns most of the details, it omits the `collectBlockNumber` field that is available in the Position struct. ```solidity=185 function positions( uint256 tokenId ) external view override returns ( uint96 nonce, address operator, address token0, address token1, uint24 fee, int24 tickLower, int24 tickUpper, uint128 liquidity, uint256 feeGrowthInside0LastX128, uint256 feeGrowthInside1LastX128, uint128 tokensOwed0, uint128 tokensOwed1 ) ```

`safeApprove()` has been [Deprecated](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bfff03c0d2a59bcd8e2ead1da9aed9edf0080d05/contracts/token/ERC20/utils/SafeERC20.sol#L38-L45) in favor of `safeIncreaseAllowance()` and `safeDecreaseAllowance()`. If only setting the initial allowance to the value that means infinite, `safeIncreaseAllowance()` can be used instead. The function may currently work, but if a bug is found in this version of OpenZeppelin, and the version that you're forced to upgrade to no longer has this function, you'll encounter unnecessary delays in porting and testing replacement contracts.