Exchange - LPBase

The current implementation of the LiquidityRouter contract lacks an upper bound on the execution fee. This could result in excessive fees being charged for executing orders. Without a limit, there's a potential for misuse where very high fees could be set, leading to "fee paid" orders that may not be economically viable for users.

To address this issue, it is recommended to introduce an upper bound on the execution fee. This can be done by adding a maximum allowable fee parameter and ensuring that any set fee does not exceed this limit. Additionally, include a function to update this maximum fee, with appropriate access control to prevent unauthorized changes.

Remediation Plan

SOLVED: The LPBase team solved the issue.

The current implementation of the OrderManager contract may lead to accounting errors when dealing with deflationary tokens. When placing a swap order using a deflationary token, the executeSwapOrder() function attempts to swap the same amount of tokens that were initially transferred during the order placement. However, due to the deflationary nature of the token, the actual received amount may be lower than the intended swap amount. This discrepancy can cause the transaction to revert if the OrderManager contract doesn't have a sufficient token balance to fulfill the swap. Additionally, when canceling an order using the cancelOrder() function, returning the funds to the order owner may fail if the OrderManager contract's token balance is insufficient due to the deflationary token's behavior.

To address this issue, consider the following approaches:

Determine whether deflationary tokens will be supported by the OrderManager contract. If support for deflationary tokens is desired, modify the contract to handle them properly. Instead of relying on the amount specified in the transfer() call, use the contract's actual token balance to determine the amount to swap or return. This can be achieved by calculating the difference in the contract's token balance before and after the transfer. If deflationary tokens are not intended to be supported, implement a validation mechanism to prevent their usage. For example, you can check if the received amount matches the expected amount after a transfer and revert the transaction if there is a mismatch.

Remediation Plan

ACKNOWLEDGED: The LPBase team acknowledged this finding.

The addToken() function allows the contract owner to add new tokens to the pool. The token's address is added to the allAssets array, and the token is marked as listed and whitelisted. However, there is no corresponding function to completely remove a token from the pool. The delistToken() function only marks the token as unlisted and sets its target weight to zero, but the token remains in the allAssets array. This means that once the MAX_ASSETS threshold is reached (currently set at 10 total assets), no new tokens can be added, and existing tokens cannot be removed, even if they are delisted.

    function addToken(address _token, bool _isStableCoin, bool _isPool) external onlyOwner {
        if (!isAsset[_token]) {
            isAsset[_token] = true;
            isListed[_token] = true;
            allAssets.push(_token);
            isStableCoin[_token] = _isStableCoin;
            isPool[_token] = _isPool;
            if (allAssets.length > MAX_ASSETS) {
                revert PoolErrors.TooManyTokenAdded(allAssets.length, MAX_ASSETS);
            }
            emit TokenWhitelisted(_token);
            return;
        }

        if (isListed[_token]) {
            revert PoolErrors.DuplicateToken(_token);
        }

        // token is added but not listed
        isListed[_token] = true;
        emit TokenWhitelisted(_token);
    }

Consider fixing the logic on the addAsset function.

Remediation Plan

ACKNOWLEDGED: The LPBase team acknowledged this finding.

The current implementation of the contracts inherits from OwnableUpgradeable, which provides a basic ownership mechanism. However, this approach has a potential risk: if the owner accidentally transfers ownership to an incorrect address, the contract's ownership may be permanently lost.

contract OrderManager is Initializable, OwnableUpgradeable, ReentrancyGuardUpgradeable {}

To mitigate this risk, it is recommended to use the Ownable2StepUpgradeable contract from the OpenZeppelin library instead of OwnableUpgradeable. Ownable2StepUpgradeable provides a two-step ownership transfer process, which adds an extra layer of security to prevent accidental ownership transfers.

Remediation Plan

ACKNOWLEDGED: The LPBase team acknowledged this finding.

Contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

pragma solidity ^0.8.15;

Consider locking the pragma version in the smart contracts. It is not recommended to use a floating pragma in production.

For example: pragma solidity 0.8.21;

Remediation Plan

SOLVED: The LPBase team solved the issue by locking pragma.

The provided code snippet contains a function named getAssetAum that is intended to calculate the Assets Under Management (AUM) for a specific token in a pool. However, the implementation of the function is incomplete and currently returns a hardcoded value of 0.

The function takes three parameters:

• poolAddress: The address of the pool.

• token: The address of the token for which the AUM needs to be calculated.

• _max: A boolean flag indicating whether to use the maximum price or not.

The function is marked as external and view, suggesting that it is intended to be called from outside the contract and does not modify the contract's state.

The issue with the current implementation is that it does not perform the actual AUM calculation and instead returns a constant value of 0. The commented-out code block below the return 0 statement indicates that there is an intended logic for calculating the AUM based on various factors such as the pool's stability, token price, pool amount, reserved amount, guaranteed value, and total short size. However, this code is currently commented out and not executed.

To address this issue and complete the implementation of the getAssetAum function, the following steps should be taken:

1. Uncomment the code block below the return 0 statement to enable the actual AUM calculation logic.

Remediation Plan

SOLVED: The LPBase team solved the issue by applying the suggested recommendation.

The ETHUnwrapper contract is designed to unwrap WETH (Wrapped ETH) tokens and transfer the resulting ETH to a specified address. However, there is an issue with the gas limit set for the ETH transfer operation, which prevents Gnosis Safe contracts from interacting with the unwrap function.

In the safeTransferETH function, the ETH transfer is performed using a low-level call with a fixed gas limit of 2400:

(bool success,) = *to.call{gas:2400, value: *amount}("");

The gas limit of 2400 is insufficient for Gnosis Safe contracts to successfully receive the ETH transfer. Gnosis Safe contracts require a higher gas limit to execute their internal logic and process the incoming ETH transfer.

Replace the fixed gas limit of 2400 with a higher value that is sufficient for Gnosis Safe contracts to process the ETH transfer successfully.

Remediation Plan

SOLVED: The LPBase team solved the issue by applying the suggested recommendation.

There is a typo in the comment intended to describe the setting of the liquidity execution fee. The word "liquidity" is misspelled as "liuqidity".

Location:

The typo appears in a comment within the onlyOperator modifier.

Current code:

modifier onlyOperator() {

if (operator != msg.sender && !hasRole(DEFAULT_ADMIN_ROLE, msg.sender)){ // set liuqidity execution fee

revert LiquidityErrors.OnlyOperator();

}

_;

}

Correct code:

modifier onlyOperator() {

    if (operator != msg.sender && !hasRole(DEFAULT_ADMIN_ROLE, msg.sender)){ // set liquidity execution fee

        revert LiquidityErrors.OnlyOperator();

    }

    _;

}

Correct the spelling of "liquidity" in the comment.

Remediation Plan

SOLVED: The LPBase team solved the issue.

In the LiquidityOrder struct, the expiresAt field is commented out:

struct LiquidityOrder {

    // ... other fields ...

    // uint256 expiresAt;

    // uint256 executionFee;

}

The absence of an expiration time for liquidity orders could lead to the following issue :

1. Without an expiration, orders could remain valid indefinitely, potentially being executed under market conditions very different from when they were created.

1. Re-enable the expiresAt field in the LiquidityOrder struct:

  struct LiquidityOrder {

       // ... other fields ...

       uint256 expiresAt;

       uint256 executionFee;

   }

Remediation Plan

ACKNOWLEDGED: The LPBase team acknowledged the issue.