The SHToken contract’s _updateUserBalance() and deleteUserFromArray() functions use an array, users, to keep track of addresses with non-zero balances. This design presents a critical Denial of Service (DoS) vulnerability that can be exploited by an attacker. Specifically, an attacker could transfer a minimal amount of tokens (e.g., 1 unit) to thousands of unique addresses, causing the users array to grow substantially in size. As the users array expands, the gas cost for iterating through it in the deleteUserFromArray() function and related functions becomes increasingly prohibitive. This could lead to failures in essential operations such as mint(), burn(), and transfer() due to gas limitations.

See the deleteUserFromArray() function, as it iterates through the users array to find and remove the specified user:

function deleteUserFromArray(address user) internal {
    for (uint256 i = 0; i < users.length; i++) {
        if (users[i] == user) {
            users[i] = users[users.length - 1];
            users.pop();
            break;
        }
    }
}

Because the users array can grow indefinitely, the gas cost of this loop could exceed the block gas limit when the array reaches a significant size, resulting in transaction failures and making the contract's core functions unusable.

The following Foundry test demonstrates this vulnerability:

function testFail_ArrayOverflowDoS() external {
    // Check 0 users using SHToken
    assertEq(shToken.getUsers().length, 0);

    // Step 1: Perform an initial mint of 1 unit to ALICE
    vm.prank(tokenOwner); // Simulate the context of the token owner.
    shToken.mint(ALICE, 1e6);

    // Step 2: Alice, as an attacker transfers 1 unit of tokens to 14_600 unique addresses.
    for (uint256 i = 0; i < 14_600; i++) {
        address recipient = address(uint160(i + 1)); // Create unique addresses starting from address 1.
        vm.prank(ALICE);
        shToken.transfer(recipient, 1);
    }

    // Verifies 14_600 addresses hold SHTokens
    assertEq(shToken.getUsers().length, 14_600);

    // Step 3: The token owner tries to mint some tokens to legit users and it gets OutOfGas errors
    for (uint256 i = 0; i < 100; i++) {
        address recipient = address(uint160(i + 100_000)); // Create unique addresses starting from address 100_000.
        vm.prank(tokenOwner);
        shToken.mint(recipient, 1);
    }
}

Run it with the following command and observe the OutOfGas error:

forge test --via-ir -vvvv --mt testFail_ArrayOverflowDoS

Redesign the user-tracking mechanism to avoid direct iteration over a potentially large array. Consider using a more efficient data structure, such as a mapping or linked list, to manage user balances without needing to iterate through the entire list. This would allow for constant-time operations when adding or removing users, preventing gas issues and enhancing the contract’s scalability and resilience to DoS attacks.

Additionally, consider adding mechanisms to limit or manage the size of the users array to prevent unbounded growth and ensure that all core functions remain operational under reasonable gas limits.

SOLVED: The SuperHedge team solved this finding in commit d3494d1 by removing the vulnerable functionality.

The SHToken contract suffers from a severe flaw in its accounting logic due to the use of two separate data structures: userBalances (custom) and _balances (inherited from OpenZeppelin's ERC20). This dual-mapping approach results in significant inconsistencies in user balance tracking, especially when functions like transferFrom() are used, which bypass custom tracking logic. As a result, users holding tokens may not appear in the users array, leading to inaccurate user reporting from the getUsers() function.

This issue can lead to incorrect user data, erroneous contract behavior, and potential vulnerabilities that undermine the reliability of user tracking and accounting.

The following Foundry test demonstrates this vulnerability:

function testFail_untracked_transferFrom() external {
    // Check 0 users using SHToken
    assertEq(shToken.getUsers().length, 0);

    // Step 1: Perform an initial mint of 1 unit to ALICE
    vm.prank(tokenOwner); // Simulate the context of the token owner.
    shToken.mint(ALICE, 1e6);

    // Check 1 user using SHToken
    assertEq(shToken.getUsers().length, 1);

    // Step 2: Alice transfers 1 token to BOB
    vm.startPrank(ALICE);
    shToken.transfer(BOB, 1);

    // Check 2 users using SHToken
    assertEq(shToken.getUsers().length, 2);

    // Step 3: Alice transfers 1 token to EVE
    shToken.approve(ALICE, 1e6);
    shToken.transferFrom(ALICE, EVE, 1);

    // Check 3 users using SHToken
    assertEq(shToken.getUsers().length, 3, "EVE has not been properly tracked");
}

Run with the following command and observe how using transferFrom() breaks the accounting and list of users:

forge test --via-ir -vvvv --mt testFail_untracked_transferFrom

To address this issue, unify the balance-tracking logic by removing duplicate structures or ensuring all relevant functions update both userBalances and the users array consistently. Additionally, review and extend custom logic to functions such as transferFrom() to maintain accurate user tracking and reporting across all operations.

SOLVED: The SuperHedge team solved this finding in commit d3494d1 by removing the vulnerable functionality.

The SHFactory contract features a setProductName() function designed to update the name of a product by invoking updateName() on ISHProduct:

/**
* @notice set new product name
*/
function setProductName(string memory _name, address _product) external onlyOwner {
  require(getProduct[_name] == address(0), "Product already exists");
  require(isProduct[_product], "Invalid product address");
  string memory _oldName = ISHProduct(_product).name();
  delete getProduct[_oldName];
  getProduct[_name] = _product;
  ISHProduct(_product).updateName(_name);

  emit ProductUpdated(_product, _name);
}

However, the SHProduct contract does not implement the updateName() function, even though it is specified in the ISHProduct interface. This oversight means that any call to setProductName() will revert, rendering the function ineffective and unusable. Without the updateName() implementation in SHProduct, setProductName() will always fail, preventing updates to product names and disrupting intended contract operations.

This issue highlights a larger problem noted in the "Missing Interface Inheritance" finding of this report: SHProduct does not inherit from ISHProduct. This results in non-compliance with the expected interface, leading to broken functionality and potential misunderstandings about the contract's behavior.

To address this issue, implement a dedicated updateName() function in SHProduct that aligns with the ISHProduct interface specification. This function should only take the new name as input and update it accordingly. Doing so will ensure that setProductName() in SHFactory functions as intended without requiring unnecessary data or permissions. Additionally, make sure SHProduct inherits from ISHProduct to ensure compliance with the interface and prevent similar issues in the future.

Note: While SHProduct does have the updateParameters() function, which allows updating the product's name, it is not suitable for use as a direct replacement for updateName() in the setProductName() function. This is because updateParameters() requires additional parameters, including an IssuanceCycle struct and addresses for the router and market, making it more complex and restrictive for simple name updates.

SOLVED: The SuperHedge team solved this finding in commit cd86de1 by removing the affected function.

The current implementation of the deposit() function in the SHProduct contract does not account for fee-on-transfer tokens. When tokens are transferred to the contract, the deposit() function assumes that the transferred amount matches the _amount parameter specified by the user. However, fee-on-transfer tokens deduct a fee during the transfer process, resulting in the contract receiving less than the _amount. This discrepancy can cause issues, including blocked deposits and inconsistencies, as the minting and event emission operations are based on the _amount parameter rather than the actual amount received.

function deposit(uint256 _amount, bool _type) external whenNotPaused nonReentrant onlyAccepted{
    require(_amount > 0, "Greater zero");
    uint256 decimals = _currencyDecimals();
    
    uint256 amountToDeposit = _amount;
    if (_type == true) {
        amountToDeposit += userInfo[msg.sender].coupon + userInfo[msg.sender].optionPayout;
    }
    require((maxCapacity * 10 ** decimals) >= (currentCapacity + amountToDeposit), "Product is full");

    uint256 supply = amountToDeposit / (1 * 10 ** decimals);

    currency.safeTransferFrom(msg.sender, address(this), _amount);
    IERC20Token(tokenAddress).mint(msg.sender,_amount);
  ...
}

The SuperHedge Finance team indicated that any ERC20 token supported by Pendle Finance may be accepted as _currency, which includes over 30 tokens on Ethereum and a growing list. This flexibility further emphasizes the need to handle various token mechanics, including fee-on-transfer behavior.

Implement checks to measure the contract's balance before and after the safeTransfer() and safeTransferFrom() operations to accurately determine the actual amount received, accounting for potential fee-on-transfer discrepancies. This approach ensures the correct amount is used for subsequent operations, avoiding issues with underfunded deposits. Alternatively, clearly document that fee-on-transfer tokens are not supported, so users are aware of this limitation.

The balance-based approach can be implemented as follows:

• Record the balance of the contract before the transfer.

• Perform the transfer operation.

• Calculate the received amount by subtracting the pre-transfer balance from the post-transfer balance.

This method accounts for deflationary tokens and avoids potential issues with arbitrary ERC20 tokens that have non-standard transfer mechanisms.

SOLVED: The SuperHedge team mitigated this finding by clearly documenting that fee-on-transfer tokens are not supported, so users are aware of this limitation.

The audit revealed that the project lacks any form of test suite, which poses significant risks to the reliability and security of the smart contracts. Without tests, it is impossible to verify that the contract functions as intended or to identify potential vulnerabilities and edge cases.

Additionally, several code elements exhibit incomplete or inconsistent NatSpec documentation. This lack of comprehensive documentation can lead to misunderstandings during development and maintenance, potentially resulting in the misinterpretation of contract functionality or use.

Comprehensive test coverage and clear NatSpec documentation are essential for ensuring smart contract quality, maintainability, and safety.

Implement Tests: Develop and integrate a complete test suite to validate all functionalities and scenarios within the project. Proper testing helps identify bugs, verify expected behaviors, and provide assurance of contract security.

Update NatSpec Documentation: Ensure that all functions and structures are fully documented with complete NatSpec annotations. Detailed documentation assists developers in understanding the code's purpose and parameters, facilitating future maintenance and auditing efforts.

Addressing these issues will significantly enhance the codebase's reliability, maintainability, and overall quality.

SOLVED: The SuperHedge team solved this finding in commits 69beead, 38c3e52, 0f73d88 and 3ceab43 by adding tests and NatSpec documentation and unity tests as recommended.

The SHProduct contract includes several ERC20 operations that do not use OpenZeppelin's SafeERC20 library, which is considered best practice for handling token transfers securely. This can lead to potential issues if the ERC20 token used does not revert on failed transfers or approvals, resulting in silent failures that could affect the contract's accounting and state.

Instances where ERC20 operations are not using SafeERC20 or where return values are not verified include:

• In the distributeFunds() function:

function distributeFunds(
    uint8 _yieldRate
) external onlyManager onlyLocked {
    require(!isDistributed, "Already distributed");
    require(_yieldRate <= 100, "Less than 100");
    uint8 optionRate = 100 - _yieldRate;

    uint256 optionAmount;
    if (optionRate > 0) {
        optionAmount = currentCapacity * optionRate / 100;
        currency.transfer(exWallet, optionAmount);
    }
...
}

• In other parts of the contract:

IERC20(currencyAddress).approve(address(router), yieldAmount);

and:

IERC20(PT).approve(address(router), exactPtIn);

Using transfer(), transferFrom(), and approve() directly can be problematic if the token contract does not conform strictly to the ERC20 standard or does not return a boolean value. This can lead to undetected failures or vulnerabilities in certain edge cases.

These examples might not be exhaustive, and it is recommended to review the entire codebase for similar instances where SafeERC20 functions should be applied.

It is recommended to consistently use OpenZeppelin's SafeERC20 library to ensure secure ERC20 operations. Replace direct calls like transfer(), transferFrom(), and approve() with safeTransfer(), safeTransferFrom(), and safeApprove() respectively. This approach ensures that the return values are correctly checked and that any failure in the operation results in a revert, thereby preventing silent failures and maintaining the integrity of the contract's state.

Using SafeERC20 functions will improve the security and reliability of the contract, especially when interacting with tokens that may not fully adhere to the ERC20 standard.

PENDING: The SuperHedge team indicated that they will address this in the future.

In SHProduct, the fundAccept(), deposit() and earlyWithdraw() functions perform division operations that are susceptible to division by zero errors. This could cause transactions to revert unexpectedly. For example:

• In the fundAccept() function:

function fundAccept() external whenNotPaused onlyWhitelisted {
  uint256 totalSupply = IERC20(tokenAddress).totalSupply();
  ...
  userInfo[totalHolders[i]].optionPayout += prevSupply * _optionProfit / totalSupply;
...

If totalSupply() returns zero, this line will result in a division by zero, causing the transaction to revert. This situation may occur if no tokens have been minted yet, potentially disrupting any operations that rely on the successful execution of fundAccept().

• In the deposit() function:

function deposit(uint256 _amount, bool _type) external whenNotPaused nonReentrant onlyAccepted{
  require(_amount > 0, "Greater zero");
  uint256 decimals = _currencyDecimals();
  ...
  uint256 supply = amountToDeposit / (1 * 10 ** decimals);
...

If decimals is zero, the division could lead to unintended behavior.

• In the earlyWithdraw() function:

function earlyWithdraw(uint256 _noOfBlock) external onlyIssued{
  ...
  uint256 withdrawBlockSize = (issuanceCycle.underlyingSpotRef * 10**(decimals) * issuanceCycle.optionMinOrderSize)/10;
  uint256 totalBlock = currentToken / withdrawBlockSize;
...

If withdrawBlockSize is zero, the division by withdrawBlockSize will also result in a transaction revert.

Introduce conditional checks to verify that totalSupply, supply, and withdrawBlockSize are greater than zero before performing these divisions. If any of these values are zero, consider reverting the transaction with an informative error message or handling the situation appropriately to avoid unexpected failures. This ensures that the fundAccept(), deposit(), and earlyWithdraw() functions handle edge cases gracefully and maintain robustness.

SOLVED: The SuperHedge team solved this finding in commit 53b0b93 by implementing the recommended checks.

The SHToken contract utilizes OpenZeppelin's AccessControl for managing roles but only defines and grants the MINTER_ROLE, without including other roles such as a BURNER_ROLE for the burn() function. This could lead to security and operational challenges, as there is no specific access control for burning tokens or for other potentially sensitive functions. Additionally, the contract does not make use of the DEFAULT_ADMIN_ROLE, which limits flexibility in role assignment and management.

Note: The lack of access control in the burn() function, which allows unauthorized users to burn tokens from other accounts, has already been discussed in the finding titled "Anyone Can Burn SHToken From Other Users".

The SHProduct contract, while not directly using AccessControl, implements custom role management via modifiers such as onlyAdmin, onlyManager, and onlyWhitelisted. This contract could also benefit from adopting a structured approach using AccessControl or even OpenZeppelin's AccessControlDefaultAdminRules for better security and oversight of role management.

Implications:

• Security Risks: The contract's minimal use of roles means that the burn() function and other potential privileged operations are not managed by specific access controls. This could lead to improper use if new restricted functions are added without sufficient role differentiation.

  
  

• Role Management Limitations: Not using DEFAULT_ADMIN_ROLE or similar mechanisms restricts the contract's ability to securely manage role assignments and transitions, which could lead to role mismanagement or difficulty in future scalability.

Introduce additional roles such as a BURNER_ROLE to manage access to the burn() function and ensure that only authorized accounts can execute this action. Utilize DEFAULT_ADMIN_ROLE in the SHToken contract to provide better oversight and role management. Additionally, consider implementing OpenZeppelin's AccessControlDefaultAdminRules to enforce best practices, such as a two-step process for role transfers, ensuring that transitions are secure and controlled.

For SHProduct, evaluate integrating AccessControl or AccessControlDefaultAdminRules to replace or enhance the custom role-checking logic (onlyAdmin, onlyManager, onlyWhitelisted). This approach standardizes role management, makes it easier to extend permissions, and provides a more transparent and auditable structure for maintaining contract security.

SOLVED: The SuperHedge team solved this finding in commit 120bb62 by implementing access control on the burn() function as recommended.

During the security assessment, it was identified that some functions in the smart contracts lack proper input validation, allowing critical parameters to be set to undesired or unrealistic values. This can lead to potential vulnerabilities, unexpected behavior, or erroneous states within the contract.

Examples include, but are not limited to:

• SHToken contract: The burn() function includes a check to ensure the amount argument is not zero. However, the mint() function does not have a similar validation, potentially allowing the minting of zero or unrealistic token amounts.

• SHProduct contract: The functions updateCoupon(), updateParameters(), and updateStructure() lack checks for the validity of their input parameters, which could allow the contract to accept unrealistic or unintended values.

• SHFactory contract: The initialize() function could benefit from input validation to prevent the use of incorrect or unexpected initial parameters.

This list is not exhaustive. It is recommended to conduct a comprehensive review of the codebase to identify and assess other functions that may require additional input validation. Ensuring appropriate checks are in place for critical parameters will enhance the overall reliability, security, and predictability of the contracts.

To mitigate these issues, implement input validation in all constructor functions and other critical functions to ensure that inputs meet expected criteria. This can prevent unexpected behaviors and potential vulnerabilities.

SOLVED: The SuperHedge team solved this finding in commits 9154686 and fba6ed6 by adding validations to different inputs as recommended.

It was identified that the SHFactory contract inherited from OpenZeppelin's OwnableUpgradeable library. Ownership of the contracts that are inherited from the OwnableUpgradeable module can be lost, as the ownership is transferred in a single-step process.

The address that the ownership is changed to should be verified to be active or willing to act as the owner. Ownable2StepUpgradeable is safer than OwnableUpgradeable for smart contracts because the owner cannot accidentally transfer smart contract ownership to a mistyped address. Rather than directly transferring to the new owner, the transfer only completes when the new owner accepts ownership.

To mitigate the risks associated with single-step ownership transitions and enhance contract security, it is recommended to adopt a two-step ownership transition mechanism, such as OpenZeppelin's Ownable2StepUpgradeable. This approach introduces an additional step in the ownership transfer process, requiring the new owner to accept ownership before the transition is finalized. The process typically involves the current owner calling a function to nominate a new owner, and the nominee then calling another function to accept ownership.

Implementing Ownable2StepUpgradeable provides several benefits:

1. Reduces Risk of Accidental Loss of Ownership: By requiring explicit acceptance of ownership, the risk of accidentally transferring ownership to an incorrect or zero address is significantly reduced.

2. Enhanced Security: It adds another layer of security by ensuring that the new owner is prepared and willing to take over the responsibilities associated with contract ownership.

3. Flexibility in Ownership Transitions: Allows for a smoother transition of ownership, as the nominee has the opportunity to prepare for the acceptance of their new role.

By adopting Ownable2StepUpgradeable, contract administrators can ensure a more secure and controlled process for transferring ownership, safeguarding against the risks associated with accidental or unauthorized ownership changes.

SOLVED: The SuperHedge team solved this finding in commit fc013ba by switching from OwnableUpgradeable to Ownable2StepUpgradeable as recommended.

A potential issue was identified in the fundAccept() function of the SHProduct contract related to division operations that can lead to rounding errors and precision loss. The specific line involved is:

userInfo[totalHolders[i]].optionPayout += prevSupply * _optionProfit / totalSupply;

Solidity truncates the result of division operations, which can lead to rounding errors when the result includes fractional values. If prevSupply * _optionProfit is not properly scaled relative to totalSupply, the division result may be smaller than expected or even zero. This impacts the accuracy of optionPayout calculations and can result in users receiving less than their fair share or nothing at all when they should receive a non-zero amount.

To mitigate precision loss and rounding errors, modify the code to apply a scaling factor to the numerator before performing the division. This approach helps maintain the accuracy of calculations by preserving fractional values. After performing the division, adjust the result by dividing by the same scaling factor to ensure the final value is correctly interpreted. It is also important to adapt downstream calculations accordingly to maintain consistency and avoid excessively large results.

Additionally, test the contract with edge cases that involve large and small values to confirm that the scaling solution works as intended and does not introduce unexpected behavior or inaccuracies. Implementing these changes will help ensure precise and fair optionPayout calculations, leading to better reliability and trust in the contract’s payout distribution logic.

SOLVED: The SuperHedge team solved this finding in commit d3494d1 by removing the affected code.

It was identified that the SHFactory contract inherited from OpenZeppelin's OwnableUpgradeable library. In the OwnableUpgradeable contracts, the renounceOwnership() function is used to renounce the Owner permission. Renouncing ownership before transferring would result in the contract having no owner, eliminating the ability to call privileged functions.

/**
 * @dev Leaves the contract without owner. It will not be possible to call
 * `onlyOwner` functions. Can only be called by the current owner.
 *
 * NOTE: Renouncing ownership will leave the contract without an owner,
 * thereby disabling any functionality that is only available to the owner.
 */
function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
}

It is recommended that the Owner cannot call renounceOwnership() without first transferring ownership to another address. In addition, if a multi-signature wallet is used, the call to the renounceOwnership() function should be confirmed for two or more users.

SOLVED: The SuperHedge team solved this finding in commit f9fa434 by overriding the renounceOwnership() function as recommended.

The SHTokenFactory contract implements the ISHTokenFactory interface, but does not inherit from it. See its declaration:

contract SHTokenFactory {

Similarly, the SHProduct and SHToken contracts are expected to implement the ISHProduct and ISHToken interfaces respectively, but do not inherit from them either:

contract SHProduct is StructGen, ReentrancyGuardUpgradeable, PausableUpgradeable,EventFunctions{

When attempts were made to explicitly inherit from the ISHProduct and ISHToken interfaces, the compiler raised "Missing implementation" errors, indicating that required functions have not been fully implemented as per the interfaces.

Ensure that both SHTokenFactory, SHProduct and SHToken explicitly inherit from their respective interfaces (ISHTokenFactory, ISHProduct and ISHToken) and that all required functions defined in these interfaces are implemented in the contracts. This practice improves code clarity, ensures consistency, and prevents potential interface discrepancies.

SOLVED: The SuperHedge team solved this finding in commits 6297706, b58f557 and 1d3fc77 by making SHTokenFactory inherit from ISHTokenFactory as recommended. ISHTokenFactory and ISHProduct were removed.

In Solidity smart contract development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

It is recommended to replace hard-coded revert strings in require statements for custom errors, which can be done following the logic below.

1. Standard require statement (to be replaced):

require(condition, "Condition not met");

2. Declare the error definition to state

error ConditionNotMet();

3. Use the Custom Error in require or if statements.

Using custom errors inside require statements is only available from Solidity versions 0.8.26 and above. In case of older compiler version, consider using if statements + revert with Custom Error.

ACKNOWLEDGED: The SuperHedge team indicated that they will not be addressing this finding.

When the length of an array is not cached outside of a loop, the Solidity compiler reads the length of the array during each iteration. For storage arrays, this results in an extra sload operation (100 additional gas for each iteration except the first). For memory arrays, this results in an extra mload operation (3 additional gas for each iteration except the first).

Detecting loops that use the length member of a storage array in their loop condition without modifying it can reveal opportunities for optimization.

See the following example in contracts/SHProduct.sol:

function storageOptionPosition(address[] memory _userList, uint256[] memory _amountList) external onlyIssued onlyAdmin
{
    for (uint256 i = 0; i < _userList.length; i++) 
    {
        UserOptionPositions.push(UserOptionPosition({
                userAddress: _userList[i],
                value: _amountList[i]
        }));
        totalOptionPosition += _amountList[i];
    }
}

Cache the length of the array in a local variable outside the loop to optimize gas usage. This reduces the number of read operations required during each iteration of the loop. See the example fixes below:

function storageOptionPosition(address[] memory _userList, uint256[] memory _amountList) external onlyIssued onlyAdmin
{
  uint256 length = _userList.length;
    for (uint256 i = 0; i < length; i++) 
    {
        UserOptionPositions.push(UserOptionPosition({
                userAddress: _userList[i],
                value: _amountList[i]
        }));
        totalOptionPosition += _amountList[i];
    }
}

SOLVED: The SuperHedge team solved this finding in commits c480743 and 2bdd1fb by implementing the recommended changes.

The contracts in scope use multiple pragma versions with unlocked versions. Contracts should be deployed with the same compiler version and flags with which they were thoroughly tested. Locking the pragma helps to ensure that contracts are not accidentally deployed using another pragma, for example, either an outdated pragma version that could introduce bugs that negatively affect the contract system or a recently released pragma version that has not been extensively tested.

**Reference: https://github.com/ethereum/solidity/releases**

In the Solidity GitHub repository, there is a json file where are all the bugs found in the different compiler versions.

**Reference: https://github.com/ethereum/solidity/blob/develop/docs/bugs_by_version.json**"

Consider locking the pragma version. When possible, do not use floating pragmas. Specifying a fixed compiler version ensures that the bytecode produced does not vary between builds. This is especially important if you rely on bytecode-level verification of the code.

SOLVED: The SuperHedge team solved this finding in commit b58f557 by implementing recommended fix.

The StructGen.sol smart contract is marked as unlicensed, as indicated by the SPDX license identifier at the top of the file:

// SPDX-License-Identifier: UNLICENSED

Using unlicensed contract can lead to legal uncertainties and potential conflicts regarding the usage, modification and distribution rights of the code. This may deter other developers from using or contributing to the project and could potentially lead to legal issues in the future.

It is strongly recommended to choose and apply an appropriate open-source license to the smart contract. Some popular options for blockchain and smart contract projects include:

1. MIT License: A permissive license that allows for reuse with minimal restrictions.

2. GNU General Public License (GPL): A copyleft license that ensures derivative works are also open-source.

3. Apache License 2.0: A permissive license that provides an express grant of patent rights from contributors to users.

SOLVED: The SuperHedge team solved this finding in commit e7c09e2 by implementing recommended fix.

It has been observed that some functionalities are missing emitting events. Events are a method of informing the transaction initiator about the actions taken by the called function. It logs its emitted parameters in a specific log history, which can be accessed outside of the contract using some filter parameters. Events help non-contract tools to track changes, and events prevent users from being surprised by changes.

Examples:

• Some examples from the SHProduct contract: the functions whitelist(), removeFromWhitelist() and addAdmin().

• Some examples from the SHToken contract: the _updateUserBalance() or deleteUserFromArray() functions.

This list is not exhaustive, and it is recommended to review the entire codebase to identify additional instances where event emissions may be beneficial. A thorough analysis should be conducted to determine whether adding event emissions aligns with the intended design and provides value in tracking state changes.

All functions updating important parameters should emit events.

SOLVED: The SuperHedge team solved this finding in commit 3a5fb93 by implementing recommended fix.

The SHToken contract imports and uses OpenZeppelin's SafeMath library. However, since Solidity 0.8 and later versions have built-in overflow and underflow protection, the use of SafeMath is redundant:

* NOTE: `SafeMath` is generally not needed starting with Solidity 0.8, since the compiler
* now has built in overflow checking.

Consider removing the use of SafeMath to simplify the code and reduce unnecessary dependencies. If mathematical functions beyond basic operations are required, consider using OpenZeppelin's Math.sol from version 5.0 or higher. This ensures up-to-date practices and leverages more advanced utility functions without redundant overflow checks.

SOLVED: The SuperHedge team solved this finding in commit 358750a by implementing recommended fix.

In Solidity development, adhering to the official style guide is best practice to ensure code consistency, readability, and maintainability. Throughout the contracts, there are several instances where the code does not follow these guidelines. Some examples include:

• Function Visibility: Some public functions are not called within the contract that declares them and could be declared as external for better optimization.

  
  

• Function Modifiers: view functions, such as decimals() in SHToken, could be declared as pure when they do not access any state variables.

  
  

• Naming Conventions: Internal functions, like deleteUserFromArray() in SHToken, should follow the convention of starting with an underscore (e.g., _deleteUserFromArray()).

  
  

• State Variable Naming: Internal state variables, such as users in SHToken, should start with an underscore to differentiate them from local variables (e.g., _users).

  
  

• Formatting Issues: The SHProduct contract has instances of inconsistent formatting, such as extra spaces before semicolons (IPAllActionV3 public router ;).

  
  

• Inheritance Formatting: The SHProduct contract's inheritance declaration lacks spacing consistency:

contract SHProduct is StructGen, ReentrancyGuardUpgradeable, PausableUpgradeable,EventFunctions{

• Event Argument Naming: Events in contracts/libraries/EventFunctions.sol have arguments with leading underscores, which is not typical practice:

event DistributeFunds(
    address indexed _exWallet,
    uint256 _optionRate,
    address indexed _pendleRouter,
    uint8 _yieldRate
);

event RedeemYield(
    address _pendleRouter,
    uint256 _amount
);

This list is not exhaustive, and it is recommended to review the entire codebase to identify other areas where style guide improvements can be made.

Apply the following style guide enhancements throughout the codebase:

• Evaluate all public functions to determine if they can be declared as external.

• Review all view functions to check if they can be declared as pure.

• Prefix internal function names and state variables with an underscore.

• Ensure consistent spacing and formatting, especially in inheritance declarations and other code structures.

• Remove leading underscores from event argument names.

Implementing these style optimizations will improve the code's readability, consistency, and maintainability, aligning it with Solidity best practices.

SOLVED: The SuperHedge team solved this finding in commit 6b8e517 by implementing recommended fix.

The burn() function in the SHToken contract includes redundant code that can be simplified for better readability and efficiency. Specifically, the burn() function performs a check for balanceOf(account) >= amount:

require(balanceOf(account) >= amount, "Insufficient balance");
_burn(account, amount);

However, the internal _burn() function, inherited from OpenZeppelin's ERC20 contract, already includes a balance check to ensure that the account holds enough tokens before proceeding. This makes the additional balance check in the burn() function redundant.

Implications:

• Code Redundancy: Redundant checks can increase code complexity without providing additional security benefits.

• Gas Costs: While minimal, performing unnecessary checks can slightly increase gas consumption for each call to the burn() function.

Remove the require(balanceOf(account) >= amount, "Insufficient balance"); line from the burn() function. This change will streamline the code by relying on the internal _burn() function’s built-in check, improving readability and maintaining efficient operation without redundant logic.

SOLVED: The SuperHedge team solved this finding in commit 5a1649c by implementing recommended fix.

During the security assessment, several instances of commented-out code were found in the SHProduct contract. Commented-out code, especially if it is outdated or unused, can clutter the codebase, reduce readability, and make maintenance more difficult. Additionally, it can cause confusion among developers and auditors about the purpose and functionality of the code.

Examples include:

• Commented-out import:

// import "./interfaces/ISHFactory.sol";

• Commented-out function calls:

// IERC20(tokenAddress).transferFrom(msg.sender, deadAddress, currentToken);

• Commented-out documentation:

// /**
//  * @notice Withdraws user's option payout
//  */

• Commented-out logic:

// (netTokenOut,,) = router.swapExactPtForToken(
// address(this), address(market), exactPtIn, createTokenOutputStruct(currencyAddress, 0), emptyLimit);

These examples might not be exhaustive, and it is recommended to review the entire codebase for additional instances of commented-out code.

It is advised to remove or uncomment code that is no longer needed or should be active. Keeping the codebase clean and up-to-date improves readability and maintainability. If certain parts of the code are kept for reference, consider documenting why they are commented out and whether they are part of future plans or deprecated. This ensures that the code remains organized and avoids confusion for future development and auditing.

SOLVED: The SuperHedge team solved this finding in commit 3193b05 by implementing recommended fix.

The codebase includes instances where error messages are incorrect or ambiguous, potentially leading to confusion when transactions revert. Clear and precise error messages are crucial for understanding why a function failed, aiding both developers and users in debugging and ensuring smooth interactions with the contract. Examples from the SHProduct contract include:

• Misleading or Inaccurate Modifier Messages:

modifier LockedOrMature() {
    require(status == DataTypes.Status.Locked || status == DataTypes.Status.Mature, 
        "Neither mature nor locked");
    _;
}

modifier AcceptedOrLockedOrMature() {
    require(status == DataTypes.Status.Locked || status == DataTypes.Status.Mature || status == DataTypes.Status.Accepted, 
        "Neither mature nor locked");
    _;
}

The error message "Neither mature nor locked" in the AcceptedOrLockedOrMature modifier does not accurately reflect the condition being checked. It can be misleading, especially since the modifier also checks for status == DataTypes.Status.Accepted.

• Vague Error Messages:

function whitelist(address _account) external onlyManager {
    require(!whitelisted[_account], "Whitelisted");
    whitelisted[_account] = true;
}

The message "Whitelisted" could be more descriptive, such as "Account is already whitelisted".

function withdrawCoupon() external nonReentrant {
  uint256 _couponAmount = userInfo[msg.sender].coupon;
  require(_couponAmount > 0, "No CP");
  require(totalBalance() >= _couponAmount, "Balance");
...

The error message "No CP" is cryptic and could be improved to "No coupon payout available". Similarly, "Balance" could be changed to "Insufficient contract balance" for better clarity.

These examples might not be exhaustive, and a thorough review of the codebase is recommended to identify and improve other unclear or incorrect error messages.

Ensure that all error messages are descriptive and accurately reflect the conditions being checked. This helps users and developers understand the cause of a revert and enhances the maintainability of the contract. Revisiting error messages and updating them for clarity and accuracy will improve the user experience and simplify debugging.

SOLVED: The SuperHedge team solved this finding in commit 0e6469f by implementing recommended fix.

Two instances of redundant boolean equality checks were found in contracts/SHProduct.sol:

if (_type == true) {

This style adds unnecessary complexity to the code.

Simplify the code by removing the explicit equality check:

if (_type) {

This approach is cleaner and aligns with best practices for readability and maintainability.

SOLVED: The SuperHedge team solved this finding in commits 3e30fbb and 3a16255 by implementing recommended fix.

The project accepts using a Solidity compiler version greater than 0.8.18, which supports named mappings. Using named mappings can improve the readability and maintainability of the code by making the purpose of each mapping clearer. This practice helps developers and auditors understand the mappings' intent more easily.

Consider refactoring the mappings to use named arguments, which will enhance code readability and make the purpose of each mapping more explicit.

For example, on contracts/SHFactory.sol, instead of declaring:

/// @notice Mapping from product name to product address
mapping(string => address) public getProduct;

It could be declared as:

mapping(string productName => address productAddress) public getProduct;

SOLVED: The SuperHedge team solved this finding in commit d5b0f8c by implementing recommended fix.

The EventFunctions contract includes events that emit block.timestamp as a parameter. For example:

event FundLock(
  uint256 _timestamp
);

This use of block.timestamp as an event parameter is redundant because the timestamp of when the event is emitted is already included in the transaction metadata and can be accessed directly from the event logs. Including it as an explicit parameter increases the size of the emitted event, leading to slightly higher gas costs without adding meaningful value.

Remove block.timestamp as a parameter from event declarations unless it serves a specific, necessary purpose beyond the default timestamping available in transaction logs. This will make the event more concise, reduce gas usage, and maintain optimal efficiency in the contract.

SOLVED: The SuperHedge team solved this finding in commit 7d1757e by removing block.timestamp as a parameter from event declarations, as recommended.

During the security assessment, several unused imports and libraries were found across the codebase, potentially increasing complexity and clutter. Unused code can lead to confusion, unnecessary dependencies, and potential maintenance challenges. Examples include:

• Unused Import in SHProduct:

import "./interfaces/ISHProduct.sol";

This import is present in the SHProduct contract but is not used anywhere in the code.

• Unused Import in SHToken:

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

This import is included but not referenced in the SHToken contract.

• Unused Library in SHProduct:

import "./libraries/Array.sol";

Although Array is imported and referenced with using Array for address[];, none of the address[] variables utilize its remove() function or any other functionality provided by the library.

These examples might not cover all instances, so it is recommended to review the entire codebase for additional unused imports and libraries.

For better clarity, consider removing all unused code. Keeping the code clean and relevant helps in maintaining a secure and efficient codebase.

If the imported library or functionality is intended for future use, document this in the code to avoid confusion.

SOLVED: The SuperHedge team solved this finding in commits a919cb8 and 127a22f by removing removing the unused code as recommended.

In programming, magic numbers refer to the use of unexplained numerical or string values directly in code, without any clear indication of their purpose or origin. The use of magic numbers can lead to confusion and make the code more difficult to understand, maintain, and update.

To improve the readability and maintainability of smart contracts, it is recommended to avoid using magic numbers and instead use named constants or variables to represent these values. By doing so, you provide clear context for the values, making it easier for developers to understand their purpose and significance.

Additionally, unnecessary operations, such as multiplying by 1, can make the code less efficient and harder to read. Examples found in the codebase include:

• In SHProduct:

uint256 _amount = prevSupply * issuanceCycle.coupon / 10000;

The value 10000 is used without context, making it unclear what it represents.

And:

uint256 supply = amountToDeposit / (1 * 10 ** decimals);

The expression "1 * 10 ** decimals" includes a redundant 1 operation that could be simplified to "10 ** decimals" for better clarity.

• In SHToken:

return 6;

The number 6 is returned without explanation, which can be confusing for those unfamiliar with its significance.

• In StructGen:

ApproxParams public defaultApprox = ApproxParams(0, type(uint256).max, 0, 256, 1e14);

The value 256 and 1e14 could benefit from a named constant to clarify its purpose.

These examples may not be exhaustive, and a comprehensive review of the codebase is recommended to identify and refactor other instances of magic numbers and unnecessary operations.

To improve code maintainability, readability, and reduce the risk of potential errors, replace magic numbers with well-defined constants. This approach helps provide clear and descriptive names for values, making the code easier to understand and maintain. Simplify unnecessary operations, such as 1 *, to streamline code readability. Additionally, updating values becomes more straightforward, as changes can be made in one location, reducing the risk of inconsistencies. For large numbers, consider using scientific notation (e.g., 1e4).

SOLVED: The SuperHedge team solved this finding in commits 8e5d907 and a0eb41f by removing magic numbers as recommended. Notice, however, that regular global state variables were used instead of constants.

To mitigate the risk of reentrancy attacks, a modifier named nonReentrant is commonly used. This modifier acts as a lock, ensuring that a function cannot be called recursively while it is still in execution. A typical implementation of the nonReentrant modifier locks the function at the beginning and unlocks it at the end. However, it is critical to place the nonReentrant modifier before all other modifiers in a function. Placing it first ensures that all other modifiers cannot bypass the reentrancy protection. In the current implementation, some functions use other modifiers before nonReentrant, which compromises the protection it provides.

For example, in the deposit() function below, the nonReentrant is placed after two other modifier calls, a reentrancy attack could potentially bypass the lock and manipulate the contract by exploiting the privileges of the owner:

function deposit(uint256 _amount, bool _type) external whenNotPaused nonReentrant onlyAccepted{

The risk of this finding was decreased to informational risk after reviewing that the whenNotPaused modifier of the deposit() function is not interacting with other addresses.

By following the best practice of placing the nonReentrant modifier before all other modifiers, one can significantly reduce the risk of reentrancy-related vulnerabilities. This is simple yet effective approach can help augment the security posture of any Solidity smart contract.

SOLVED: The SuperHedge team solved this finding in commit dce0ad0 by placing the nonReentrant modifier before all other modifiers as recommended.

In the SHProduct contract, multiple functions do not adhere to the checks-effects-interactions (CEI) pattern, a recommended best practice in Solidity that helps prevent reentrancy vulnerabilities. The CEI pattern dictates that functions should first perform checks (validating conditions), then make state variable updates (effects), and finally make external calls (interactions).

Below are examples where this pattern is not followed:

1. deposit() Function:

   • The function makes an external call to currency.safeTransferFrom() and IERC20Token(tokenAddress).mint() before updating state variables like currentCapacity and userInfo[msg.sender].

2. distributeFunds() Function:

   • External calls to currency.transfer(), IERC20(currencyAddress).approve(), and router.swapExactTokenForPt() occur before updating isDistributed.

3. earlyWithdraw() Function:

   • External calls to IERC20Token(tokenAddress).burn() and IERC20(PT).approve() are made before updating currentCapacity and netPtOut.

4. redeemYield() Function:

   • External calls to IERC20(PT).approve() and router.redeemPyToToken() occur before updating isDistributed.

To mitigate potential reentrancy risks, adhere to the CEI pattern by updating state variables (effects) before making any external calls (interactions). For instance:

• Update state variables such as currentCapacity, isDistributed, and userInfo[msg.sender].

• Make the external call afterward.

Rearranging the code to follow the CEI pattern ensures that all relevant state changes are made before any interactions with external contracts, reducing the risk of reentrancy attacks and enhancing the overall security of the contract.

SOLVED: The SuperHedge team solved this finding in commit b788a5a by implementing recommended fix.