The SwapManager contract does not allow updating or removing existing configurations for collateral assets, creating significant inflexibility in maintaining the protocol. If a security vulnerability is discovered in a swapper contract or if more efficient implementations become available, the protocol cannot adapt, potentially forcing reliance on outdated or insecure configurations.

The limitation is caused by the following condition in the setConfig() function, which prevents changes once a configuration is set:

function setConfig(address[] memory colls, SwapConfig[] memory configs) external onlyOwner {
    // ...
    for (uint256 i = 0; i < colls.length; i++) {
        // Prevents updating existing configurations
        if (swapConfigs[colls[i]].swapper != address(0)) {
            revert SwapManager__ConfigAlreadySet(colls[i]);
        }
        swapConfigs[colls[i]] = configs[i];
        // ...
    }
}

This restriction means:

• The protocol is locked into using configurations defined during their initial setup, even if they become obsolete, inefficient, or insecure.

• Incorrectly set configurations cannot be corrected without deploying a new contract, leading to operational risks and higher maintenance costs.

Consider adding a functionality to update and remove configurations with proper access controls.

SOLVED: The Tren finance team fixed this finding in commit 089b65c by updating the setConfig() function to allow setting new configurations for specific collateral addresses and adding the UpdateConfig event to track configuration updates. Additionally, the Tren finance team introduced a debtSwapConfigs mapping to handle configurations for token swaps to obtain the Debt Token, and differentiated it from swapConfigs, which manages underlying asset swaps involving reward tokens and other assets.

The CurveSwapper contract does not enforce slippage protection in its swap operations. Specifically, in the swap() function, when adding liquidity or removing liquidity, the min_amount parameter is hardcoded to 0:

function swap(
  address tokenIn,
  uint256 amount,
  address recipient
)
  external
  onlySwapManager
  returns (uint256 tokenAmountOut)
{
    if (tokenIn == underlyingAsset) {
        uint256[] memory amounts = new uint256[](2);
        amounts[coinPosition] = amount;
        IERC20(underlyingAsset).approve(address(curvePool), amount);
        tokenAmountOut = curvePool.add_liquidity(amounts, 0, recipient);
    } else {
        coll.approve(address(curvePool), amount);
        tokenAmountOut = curvePool.remove_liquidity_one_coin(
            amount, int128(int256(coinPosition)), 0, recipient
        );
    }
}

This absence of a minimum amount check allows transactions to succeed even if the resulting output tokens are far less than expected. As a result, users may lose funds due to unexpected price movements or poor pool liquidity conditions, operations may be executed at unfavorable rates without alerting the user, and the protocol could unintentionally facilitate adverse trades, leading to reputational damage.

Consider implementing slippage protection by requiring a minAmountOut parameter in the swap() function, representing the minimum acceptable amount of tokens a user expects to receive. This value should be passed to the Curve pool methods:

tokenAmountOut = curvePool.add_liquidity(amounts, minAmountOut, recipient);
...
tokenAmountOut = curvePool.remove_liquidity_one_coin(amount, int128(int256(coinPosition)), minAmountOut, recipient);

Additionally, validate the minAmountOut parameter to ensure it is meaningful and consistent with expected trading conditions. This will safeguard users against slippage and improve the robustness of the contract.

RISK ACCEPTED: The Tren finance team accepted the risk of this finding, citing the use of low-volatility pools like Curve that minimize slippage risk, supported by historical data and similar implementations in audited protocols. They emphasized prioritizing user experience while allowing users to manage slippage risks themselves if needed.

The SwapManager contract performs Uniswap V3 swaps without enforcing slippage protection. In both direct and indirect swaps, the amountOutMinimum parameter is hardcoded to 0, as shown below:

...
if (directSwap) {
    IRouter.ExactInputSingleParams memory params = IRouter.ExactInputSingleParams({
        tokenIn: tokenIn,
        tokenOut: tokenOut,
        fee: uint24(fee), //@audit unsafe casting
        recipient: receiver,
        deadline: block.timestamp,
        amountIn: amountIn,
        amountOutMinimum: 0,
        sqrtPriceLimitX96: 0
    });

    _amountOut = router.exactInputSingle(params);
} else {
    IRouter.ExactInputParams memory params = IRouter.ExactInputParams({
        path: abi.encodePacked(
            address(tokenOut), uint24(pathFee[1]), stablecoin, uint24(pathFee[0]), tokenIn //@audit q? what if pathFee.length != 2?
        ),
        recipient: receiver,
        deadline: block.timestamp,
        amountIn: amountIn,
        amountOutMinimum: 0
    });
...

This lack of slippage protection means that swap transactions can succeed even when the output is significantly lower than expected, exposing users to unfavorable trades and potential fund loss. In Uniswap V3, the amountOutMinimum parameter is crucial to prevent execution under poor liquidity conditions or adverse price movements.

Replace the hardcoded amountOutMinimum with a dynamic value passed as an argument to the function. This value should represent the minimum acceptable amount of tokens the user expects to receive, protecting them from slippage. For example:

function swapExactInput(
    address tokenIn,
    address tokenOut,
    uint256 amountIn,
    uint256 minAmountOut,
    uint256 fee
) external {
    IRouter.ExactInputSingleParams memory params = IRouter.ExactInputSingleParams({
        tokenIn: tokenIn,
        tokenOut: tokenOut,
        fee: uint24(fee),
        recipient: msg.sender,
        deadline: block.timestamp,
        amountIn: amountIn,
        amountOutMinimum: minAmountOut,
        sqrtPriceLimitX96: 0
    });

    router.exactInputSingle(params);
}

This ensures that the swap only proceeds if the output meets or exceeds the user-specified minimum amount, safeguarding users against slippage and improving overall protocol security.

RISK ACCEPTED: The Tren Finance team accepted the risk of this finding, emphasizing that their swaps primarily involve low-volatility pools where slippage risk is minimal, following design precedents from other audited protocols. They acknowledged the potential for edge cases, but plan to explore optional user-defined slippage protection for added flexibility.

The CurveSwapper constructor assumes that the Curve pool contains exactly two coins and that the provided underlyingAsset is one of them. This logic is implemented as follows:

if (underlyingAsset == curvePool.coins(0)) {
  coinPosition = 0;
} else {
  coinPosition = 1;
}

If the underlyingAsset is not part of the pool or if the pool has more than two coins:

• The coinPosition may be incorrectly set.

• Subsequent operations could fail or interact with the wrong asset.

• Deployment errors may render the contract non-functional, as coinPosition is immutable and cannot be corrected.

Although this issue can be mitigated by the deployer validating inputs before deployment, it introduces a risk of misconfiguration.

Add explicit validation in the constructor to ensure that the underlyingAsset is one of the pool’s coins, reverting with a descriptive error otherwise:

if (_underlyingAsset == coin0) {
  coinPosition = 0;
} else if (_underlyingAsset == coin1) {
  coinPosition = 1;
} else {
  revert CurveSwapper__UnderlyingAssetNotInPool();
}

SOLVED: The Tren finance team fixed this finding in commit 10f2786 by implementing the recommendation.

The SwapManager contract follows an upgradeable pattern, inheriting from the Initializable module from OpenZeppelin. In order to prevent leaving the contracts uninitialized, OpenZeppelin's documentation recommends adding the _disableInitializers() function in the constructor to automatically lock the contracts when they are deployed. However, all upgradeable contracts in scope are missing this function call.

This omission can lead to potential security vulnerabilities, as an uninitialized implementation contract can be taken over by an attacker, which may impact the proxy.

Consider adding a constructor and calling the _disableinitializers() method within the contracts to prevent the implementation from being initialized.

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
  _disableInitializers();
}

SOLVED: The Tren finance team fixed this finding in commit 178a78d by implementing the recommendation.

It was identified that the SwapManager contract inherited from OpenZeppelin's OwnableUpgradeable library through ConfigurableAddresses. 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.

RISK ACCEPTED: The Tren Finance team accepted the risk of this finding.

Several functions in both SwapManager and CurveSwapper lack proper input validation, relying on assumptions about parameters that may not always hold true. This oversight introduces potential vulnerabilities and operational risks.

Examples:

• SwapManager's _swapExactInput Function: The function assumes the pathFee array always contains exactly two elements but performs no validation:

function _swapExactInput(..., uint256[] memory pathFee) private returns (uint256) {
    // ...
    IRouter.ExactInputParams memory params = IRouter.ExactInputParams({
        path: abi.encodePacked(
            address(tokenOut),
            uint24(pathFee[1]),
            stablecoin,
            uint24(pathFee[0]),
            tokenIn
        ),
        // ...
    });
}

• CurveSwapper's swap Function: The function assumes that any input token not equal to underlyingAsset must be the collateral token (coll):

function swap(address tokenIn, uint256 amount, address recipient) external onlySwapManager returns (uint256) {
    if (tokenIn == underlyingAsset) {
        // ... add liquidity logic ...
    } else {
        // @audit No validation that tokenIn == coll
        coll.approve(address(curvePool), amount);
        tokenAmountOut = curvePool.remove_liquidity_one_coin(...);
    }
}

If tokenIn is neither the underlyingAsset nor the coll, the function will incorrectly approve and interact with curvePool, leading to potentially undefined behavior.

Implement robust input validation for all critical functions to enforce expected parameter requirements. This will prevent unexpected behavior, reduce debugging complexity, and enhance overall security.

For the SwapManager example, make sure the pathFee argument only has two elements:

function _swapExactInput(..., uint256[] memory pathFee) private returns (uint256) {
    if (pathFee.length != 2) {
        revert SwapManager__InvalidPathLength();
    }
    // ... rest of the function
}

For the CurveSwapper example, validate that the tokenIn address is the expected by not assuming it is coll:

function swap(address tokenIn, uint256 amount, address recipient) external onlySwapManager returns (uint256) {
    if (tokenIn == underlyingAsset) {
        // ... add liquidity logic ...
    } else if (tokenIn == address(coll)) {
        // ... remove liquidity logic ...
    } else {
        revert CurveSwapper__InvalidTokenIn();
    }
}

Consider adding comprehensive input validation across all functions that make assumptions about their parameters.

SOLVED: The Tren finance team fixed this finding in commit a956282 by implementing the recommendation.

The SwapManager contract performs unsafe downcasting from uint256 to uint24 in the _swapExactInput() and _swapExactInput() functions when interacting with the Uniswap V3 router. Specifically, it transforms the arguments fee and pathFee as shown below:

function _swapExactInput(
...
    fee: uint24(fee),
    recipient: receiver,
    deadline: block.timestamp,
    amountIn: amountIn,
    amountOutMinimum: 0,
    sqrtPriceLimitX96: 0
  });

  _amountOut = router.exactInputSingle(params);
} else {
  IRouter.ExactInputParams memory params = IRouter.ExactInputParams({
    path: abi.encodePacked(
      address(tokenOut), uint24(pathFee[1]), stablecoin, uint24(pathFee[0]), tokenIn
    ),
...

Use OpenZeppelin's SafeCast library to safely downcast integers. Alternatively, implement a safety mechanism to ensure that the downcasted value does not exceed the range of the target type.

SOLVED: The Tren finance team fixed this finding in commit 9ecb5ed by implementing the recommendation.

The two contracts in scope include several direct calls to approve() for setting token allowances, which can cause issues when dealing with tokens that do not support allowance updates from non-zero values without first resetting to zero. This behavior is common in tokens like USDT, which revert if an approve() operation is attempted without first clearing the existing allowance.

Furthermore, none of these approve() calls check the return value, which is a significant oversight since some ERC20 tokens return false instead of reverting on failure, potentially allowing the contract to proceed under incorrect assumptions.

Instances of this issue were found in the following contracts:

• In SwapManager.sol:

IERC20(tokenIn).approve(address(router), amountIn);
...
IERC20(config.underlyingAsset).approve(address(router), amountIn);
...
IERC20(config.underlyingAsset).approve(address(router), 0);

• In CurveSwapper.sol:

IERC20(underlyingAsset).approve(address(curvePool), amount);
... 
coll.approve(address(curvePool), amount);
...
coll.approve(address(gauge), lpAmount);

These direct calls to approve() are unsafe because they do not account for tokens with non-standard behaviors or handle potential failures from the function call.

Consider using SafeERC20 functions such as safeIncreaseAllowance(), safeDecreaseAllowance() and/or forceApprove() to manage allowances safely.

SOLVED: The Tren finance team fixed this finding in commit 3ea3086 by implementing the recommendation.

The _compound() function in SwapManager performs a division using the totalColl parameter without validating that it is non-zero. This introduces a risk of a division by zero error, which would revert the transaction and prevent the function from completing. The relevant line of code is:

uint256 userRate = (userColl * DECIMAL_PRECISION) / totalColl;

This issue arises because totalColl is used as the denominator without a preceding check. If totalColl is zero, the division operation will revert. This situation could occur under the following scenarios:

1. All collateral has been withdrawn, leaving no active collateral in the pool.

2. The function is invoked before any collateral is deposited.

3. A bug elsewhere in the code incorrectly sets totalColl to zero.

The lack of a validation check increases the likelihood of unexpected transaction failures, potentially affecting user experience or disrupting protocol operations.

Consider adding a validation check to ensure totalColl is non-zero before performing any division operations. For example:

if (totalColl == 0) {
  revert SwapManager__NoCollateral();
}

By introducing this validation, you can:

1. Prevent division by zero errors and unintended reverts.

2. Provide a clear and descriptive revert reason for better debugging and user understanding.

3. Improve overall protocol resilience by gracefully handling edge cases.

Integrating this check ensures the _compound() function operates safely and predictably, even under edge cases.

SOLVED: The Tren finance team fixed this finding in commit e4099a5 by adding a checker (totalActiveColl != 0) to prevent situations where reward tokens are sent directly to the SwapManager while totalActiveColl is 0, avoiding a scenario where no loans can be opened due to a SwapManager__NoCollateral revert. The Tren finance team also clarified that totalActiveColl only increases after the compounding process, as managed by the _trenBoxStorageAddColl function in BorrowerOperations and the increaseActiveCollateral function in TrenBoxStorage.

It was identified that the SwapManager contract inherited from OpenZeppelin's OwnableUpgradeable library through ConfigurableAddresses. 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));
}

Furthermore, the contract owner or single user with a role is not prevented from renouncing the role/ownership while the contract is paused, which would cause any user assets stored in the protocol, to be locked indefinitely.

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 Tren finance team fixed this finding in commit f28d84f by overriding the renounceOwnership() function to return a custom error, effectively disabling its functionality.

The SwapManager contract correctly overrides _authorizeUpgrade() as specified in OpenZeppelin's UUPS proxy pattern. However, it introduces an additional public authorizeUpgrade() function that serves no meaningful purpose:

function authorizeUpgrade(address newImplementation) public {
  _authorizeUpgrade(newImplementation);
}

This function is not part of the UUPS proxy's intended upgrade flow. In OpenZeppelin's implementation, the upgradeTo() or upgradeToAndCall() functions in the proxy contract invoke _authorizeUpgrade() internally to check if the upgrade is permitted. Introducing a separate authorizeUpgrade() function creates unnecessary exposure and deviates from the standard pattern.

This confusion extends to the test cases, where the tests rely on calling the public authorizeUpgrade() function instead of simulating a real-world upgrade through the proxy. For example:

context("when caller is owner", function () {
  it("should allow to call authorizeUpgrade", async function () {
    const { wETH } = this.collaterals.active;

    await this.contracts.swapManager
      .connect(this.signers.deployer)
      .authorizeUpgrade(wETH.address);
  });
});

context("when caller is not an owner", function () {
  it("reverts custom error OwnableUnauthorizedAccount", async function () {
    const impostor = this.signers.accounts[1];
    const { wETH } = this.collaterals.active;

    await expect(
      this.contracts.swapManager.connect(impostor).authorizeUpgrade(wETH.address)
    ).to.be.revertedWithCustomError(this.contracts.swapManager, "OwnableUnauthorizedAccount");
  });
});

These tests inaccurately validate upgradeability because they bypass the actual upgrade process, which is performed using upgradeTo() or upgradeToAndCall() in the proxy context. As a result, the tests fail to properly ensure that the UUPS proxy mechanism functions as expected.

Remove the misleading public authorizeUpgrade() function. It deviates from OpenZeppelin's UUPS best practices and creates unnecessary complexity. Tests should be updated to verify the upgrade process by interacting directly with the proxy's upgradeTo() or upgradeToAndCall() functions, as in a real-world scenario.

SOLVED: The Tren finance team fixed this finding in commit b7b63fe by implementing the recommendation.

The SwapManager contract inherits from ReentrancyGuardUpgradeable to protect against reentrancy attacks. However, it does not use the nonReentrant modifier in any of its functions. This oversight can leave the contract vulnerable to reentrancy attacks, where an attacker repeatedly calls a function before its initial execution is complete, potentially causing unexpected behavior and financial loss.

Review all external and public functions in the SwapManager contract and apply the nonReentrant modifier to functions that involve state changes, external calls, or transfers of Ether or tokens. This ensures protection against reentrancy attacks. If the nonReentrant modifier is not needed, consider removing the ReentrancyGuardUpgradeable inheritance.

SOLVED: The Tren finance team fixed this finding in commit f28d84f by removing the ReentrancyGuardUpgradeable inheritance.

Neither the SwapManager nor the CurveSwapper contracts have an emergency withdrawal function. This functionality could help recover any residual funds left in the contracts, such as small balances from rounding, or cases where users mistakenly send tokens to these contracts.

Evaluate the need for emergency withdrawal functions in both the SwapManager and CurveSwapper contracts. If deemed necessary:

1. Implement Emergency Withdrawal: Add a function to allow recovery of any tokens held in the contracts.

2. Restrict Access: Ensure the function is accessible only to authorized entities, such as the contract owner or a multisig wallet.

3. Emit Events: Include events to log emergency withdrawals for transparency.

SOLVED: The Tren finance team fixed this finding in commit 2e6bd8e by implementing emergency withdraw functions.

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 SwapManager.sol:

function _compound(address coll) private returns (uint256) {
  ...
  address[] memory ownersList = ITrenBoxManager(trenBoxManager).getTrenBoxOwners(coll);
  address collAsset = coll; // to not get error 'Stack too deep'.
  for (uint256 i = 0; i < ownersList.length; 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 _compound(address coll) private returns (uint256) {
  ...
  address[] memory ownersList = ITrenBoxManager(trenBoxManager).getTrenBoxOwners(coll);
  address collAsset = coll; // to not get error 'Stack too deep'.
  uint256 ownersLength = tokens.length;
  for (uint256 i = 0; i < ownersLength; i++) {
...

SOLVED: The Tren finance team fixed this finding in commit b32d8f5 by implementing the recommendation.

The functions setConfig() and _swapExactInput() in SwapManager.sol accept arguments as memory arrays, even though the arrays are not mutated in the external function itself. This results in unnecessary gas overhead when copying data from calldata to memory during the abi.decode() process.

Using calldata directly for such arguments bypasses the copying loop, reducing gas costs, especially for larger arrays.

Optimizing setConfig() and _swapExactInput() to accept the arrays as calldata instead of memory can reduce gas costs for external calls. The savings grow with the size of the input array.

Consider updating the function signatures as follows:

function setConfig(address[] calldata colls, SwapConfig[] calldata configs) external onlyOwner {

By switching the argument type to calldata, the colls and configs arrays are read directly from the transaction's calldata, eliminating unnecessary memory allocations and reducing gas costs.

SOLVED: The Tren finance team fixed this finding in commit 089b65c by implementing the recommendation.

The following custom error defined in ISwapManager.sol is never used:

error SwapManager__SetupIsInitialized();

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

SOLVED: The Tren finance team fixed this finding in commit 9a87da1 by implementing the recommendation.

Indexed event fields make the data more quickly accessible to off-chain tools that parse events, and add them to a special data structure known as "topics" instead of the data part of the log. A topic can only hold a single word (32 bytes) so if you use a reference type for an indexed argument, the Keccak-256 hash of the value is stored as a topic instead.

Each event can use up to three indexed fields. If there are fewer than three fields, all the fields can be indexed. It is important to note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed fields per event (three indexed fields).

This is specially recommended when gas usage is not particularly of concern for the emission of the events in question, and the benefits of querying those fields in an easier and straight-forward manner surpasses the downsides of gas usage increase.

It is recommended to add the indexed keyword when declaring events, considering the following example in ISwapManager.sol:

/// @dev Emitted when the new config is set.
event NewConfigSet(address indexed asset);

/// @dev Emitted when the new uniswap router is set.
event NewUniswapRouterSet(address indexed router);

SOLVED: The Tren finance team fixed this finding in commit 8beab36 by implementing the recommendation.

In the SwapManager contract, several functions ignore the return values of internal or external calls. In Solidity, return values often provide critical information about the success or failure of an operation. Disregarding these values can result in missed error handling.

The following instances were identified:

ISwapper(config.swapper).swap(config.underlyingAsset, remainingAmount, recipient);
...
ITrenBoxManager(trenBoxManager).increaseTrenBoxColl(
  collAsset, ownersList[i], amountToIncrease
);
...
ITrenBoxManager(trenBoxManager).updateStakeAndTotalStakes(collAsset, ownersList[i]);

It is recommended to:

1. Handle All Return Values: Review all instances of ignored return values and ensure they are either explicitly checked or deemed unnecessary with proper documentation.

2. Update Affected Functions: Refactor the relevant functions to utilize or log return values for additional validation and error handling.

By addressing this issue, the contract's robustness and reliability will be improved, ensuring that critical operations are validated and potential errors are caught effectively.

ACKNOWLEDGED: The Tren Finance team acknowledged this finding.

The following comment with a typographical error was found in the SwapManager.sol file:

/// this amount of USDT to the Curve Pool to get another LP, wthich we will stake again into the

The word wthich above should be spelled as which instead.

To maintain clarity and trustworthiness, it is essential to rectify any typographical errors present within the contracts. Correcting such errors minimizes the likelihood of confusion and reinforces confidence in the accuracy and integrity of the documentation.

SOLVED: The Tren finance team fixed this finding in commit 089b65c by implementing the recommendation.

The NatSpec documentation of the contracts and interfaces in scope contain inconsistencies and omissions that reduce clarity and could lead to misunderstandings for developers and auditors:

• Inconsistent Caller Documentation

The unstake() function in SwapManager checks for trenBoxStorage as the caller, but the NatSpec comment incorrectly states that the caller should be borrowerOperations:

/// @inheritdoc ISwapManager
function unstake(address coll, uint256 collAmount) external {
  if (msg.sender != trenBoxStorage) {
    revert SwapManager__CallerIsNotTrenBoxStorage();
  }
...

/**
 * @notice Unstakes collateral
 * @dev Only BorrowerOperations contract can call
 * We just unstake LP token from Gauge without claiming rewards
 * @param coll The address of collateral asset
 * @param collAmount The amount of collateral to unstake
 */
function unstake(address coll, uint256 collAmount) external;

• Misordered Parameters

In the _swapExactInput() function in SwapManager, the NatSpec comments list @param stablecoin and @param directSwap in the wrong order, leading to confusion when reviewing the code:

/**
 ...
 * @param stablecoin The address of stablecoin if the swap is indirect
 * @param directSwap True if the swap is direct, false if it's indirect
 ...
 */
function _swapExactInput(
  ...
  bool directSwap,
  address stablecoin,
  ...
)

• Missing NatSpec in ISwapper Interface

The ISwapper interface lacks NatSpec comments for its functions, creating inconsistency with the rest of the codebase and reducing overall readability.

In order to improve the NatSpec documentation of the project, consider the following recomendations:

• Correct NatSpec Comments: Update the unstake() function's NatSpec to reflect that trenBoxStorage is the intended caller, ensuring consistency between the comments and the code.

• Reorder Parameters in NatSpec: Fix the order of @param stablecoin and @param directSwap in _swapExactInput() to match the function signature.

• Add NatSpec to ISwapper: Include comprehensive NatSpec comments for all functions in the ISwapper interface to ensure consistency and clarity across the codebase.

These improvements will enhance readability, minimize misunderstandings, and ensure a more maintainable and developer-friendly codebase.

SOLVED: The Tren finance team fixed this finding in commit 1820f68 by implementing the recommendation.