Intent Assets - DappOs

Chainlink price feeds exhibit varying update frequencies, also known as 'heartbeats.' For instance, USDC/USD and USDT/USD price feeds have an update frequency of 24 hours (86400 seconds), while ETH/USD updates every hour (3600 seconds).

Also, in the PriceOracle.sol contract initializer, there is no specific configurations that set customizedDelay for a set of assets, which could be done by calling the setCustomizedAcceptableDelay function upon contract's initialization and setting initial configuration for the Price Oracle to operate.

- contracts/core/utils/oracle/PriceOracle.sol

    function initialize(address admin) external initializer {
        _grantRole(DEFAULT_ADMIN_ROLE, admin);
    }

    function _getAssetPriceInBase(
        address tokenAddress
    ) internal view returns (uint256) {
        address feed = chainlinkFeeds[tokenAddress];
        if (feed != address(0)) {
            (, int price, , uint256 updatedAt, ) = AggregatorV3Interface(feed)
                .latestRoundData(); // using Chainlink price feed method
            uint256 _acceptableDelay = customizedDelay[tokenAddress] == 0
                ? acceptableDelay
                : customizedDelay[tokenAddress];
            require(
                updatedAt + _acceptableDelay >= block.timestamp,
                "PriceOracle: update delay exceeds threshold"
            );
            return uint256(price);
        } else {
            ICustomizedOracle customizedOracle = customizedOracles[
                tokenAddress
            ];
            require(
                address(customizedOracle) != address(0),
                "PriceOracle: Unsupported asset"
            );
            return customizedOracle.getPrice();
        }
    }

Neglecting to enforce customizedDelay or using a uniform acceptableDelay for all Chainlink feeds can result in the _getAssetPriceInBase function operating with outdated data or consistently reverting due to mismatched update frequencies.

Furthermore, the current implementation does not include a revert mechanism to handle cases where the Oracle response returns a value of '0' (zero) for price, which can lead to unexpected behavior or errors.

To reproduce this vulnerability, consider that no customized delay is enforced for USDC or USDT collateral, and the system is currently using the Chainlink Oracle.

• Call getAssetPriceInBase in the PriceOracle.sol contract or any function that would subsequently fetch the price using the Chainlink oracle.

• As no custom delay is enforced for these assets (correct is 86400 seconds), all the transactions depending on this feed information will consistently revert with message PriceOracle: update delay exceeds threshold.

PoC Code:

    function test_chainlink_price_oracle_custom_delay() public {
        /// user `seraph` creates some deposits.
        vm.startPrank(seraph);
        /// usdc
        IERC20(usdc_address).forceApprove(address(intent_token_minting), 5_000 * 1e6);
        vm.expectRevert();
        intent_token_minting.submit(usdc_address, 500 * 1e6);
        /// usdt
        IERC20(usdt_address).forceApprove(address(intent_token_minting), 5_000 * 1e6);
        vm.expectRevert();
        intent_token_minting.submit(usdt_address, 500 * 1e6);

        console.logUint(intent_token.balanceOf(seraph));
        vm.stopPrank();
    }

Stack traces:

    ├─ [73383] TransparentUpgradeableProxy::submit(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48, 500000000 [5e8])
    │   ├─ [68413] IntentTokenMinting::submit(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48, 500000000 [5e8]) [delegatecall]
    │   │   ├─ [7356] TransparentUpgradeableProxy::isCollectingStats() [staticcall]
    │   │   │   ├─ [2411] IntentToken::isCollectingStats() [delegatecall]
    │   │   │   │   └─ ← [Return] false
    │   │   │   └─ ← [Return] false
    │   │   ├─ [3182] TransparentUpgradeableProxy::isUnderlyingAsset(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48) [staticcall]
    │   │   │   ├─ [2734] IntentToken::isUnderlyingAsset(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48) [delegatecall]
    │   │   │   │   └─ ← [Return] true
    │   │   │   └─ ← [Return] true
    │   │   ├─ [2917] TransparentUpgradeableProxy::priceOracle() [staticcall]
    │   │   │   ├─ [2472] IntentToken::priceOracle() [delegatecall]
    │   │   │   │   └─ ← [Return] TransparentUpgradeableProxy: [0x3eE590DCD55FCD21F8F65F1e208c619188079330]
    │   │   │   └─ ← [Return] TransparentUpgradeableProxy: [0x3eE590DCD55FCD21F8F65F1e208c619188079330]
    │   │   ├─ [2865] TransparentUpgradeableProxy::getBaseAsset() [staticcall]
    │   │   │   ├─ [2420] IntentToken::getBaseAsset() [delegatecall]
    │   │   │   │   └─ ← [Return] 0xdAC17F958D2ee523a2206206994597C13D831ec7
    │   │   │   └─ ← [Return] 0xdAC17F958D2ee523a2206206994597C13D831ec7
    │   │   ├─ [3164] 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48::decimals() [staticcall]
    │   │   │   ├─ [2381] 0x43506849D7C04F9138D1A2050bbF3A0c054402dd::decimals() [delegatecall]
    │   │   │   │   └─ ← [Return] 6
    │   │   │   └─ ← [Return] 6
    │   │   ├─ [30824] TransparentUpgradeableProxy::getAssetPriceInBase(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48) [staticcall]
    │   │   │   ├─ [25857] PriceOracle::getAssetPriceInBase(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48) [delegatecall]
    │   │   │   │   ├─ [15643] 0x8fFfFfd4AfB6115b954Bd326cbe7B4BA576818f6::latestRoundData() [staticcall]
    │   │   │   │   │   ├─ [7410] 0x789190466E21a8b78b8027866CBBDc151542A26C::latestRoundData() [staticcall]
    │   │   │   │   │   │   └─ ← [Return] 1715, 99996915 [9.999e7], 1721030447 [1.721e9], 1721030447 [1.721e9], 1715
    │   │   │   │   │   └─ ← [Return] 36893488147419104947 [3.689e19], 99996915 [9.999e7], 1721030447 [1.721e9], 1721030447 [1.721e9], 36893488147419104947 [3.689e19]
    │   │   │   │   └─ ← [Revert] revert: PriceOracle: update delay exceeds threshold
    │   │   │   └─ ← [Revert] revert: PriceOracle: update delay exceeds threshold
    │   │   └─ ← [Revert] revert: PriceOracle: update delay exceeds threshold
    │   └─ ← [Revert] revert: PriceOracle: update delay exceeds threshold

It is recommended to initialize proper customizedDelay for all the ChainLink Oracles - in accordance with accepted collaterals, so the checks are consistent with each heart-beat. Additionally, it is recommended to revert the call to the getAssetPriceInBase in case the answer is 0 (zero).

SOLVED: The DappOs team solved this issue through the commit hash a45df0114829497e86ffa420ec5b7993b056d207.

During the assessment of the sUSDeOracle.sol contract, the following areas of improvement were identified:

1. Unchecked zero answer: The getPrice() function does not validate if the value for usdePriceInUsd is greater than zero.

   
   

2. Price Update Delay Misconfiguration: The state variable PRICE_UPDATE_DELAY is set to 1 hour, incorrectly assuming a heartbeat of 1 hour for sUSDe price feeds. However, the heartbeat for this asset is 1 day (24 hours or 86,400 seconds) in both Chainlink and RedStone oracle networks. To ensure accurate price updates, the PRICE_UPDATE_DELAY should be updated to reflect the correct heartbeat interval.

- contracts/intentUSD/sUSDeOracle.sol

contract sUSDeOracle is ICustomizedOracle {
    uint256 public constant PRICE_UPDATE_DELAY = 1 hours;
    uint8 public immutable USDE_DECIMALS;

    IPriceOracle public immutable parentOracle;
    address public immutable usde;
    IERC4626 public immutable sUSDe;

    constructor(address _parentOracle, address _usde, address _sUSDe) {
        parentOracle = IPriceOracle(_parentOracle);
        usde = _usde;
        sUSDe = IERC4626(_sUSDe);
        USDE_DECIMALS = IERC20Metadata(usde).decimals();
    }

    function decimals() external view override returns (uint8) {
        return USDE_DECIMALS;
    }

    function getPrice() public view virtual override returns (uint256) {
        (uint256 usdePriceInUsd, uint256 usdePriceDecimals) = parentOracle
            .getAssetPriceInBase(usde);
        uint256 usdeAmount = sUSDe.convertToAssets(10 ** sUSDe.decimals());
        require(usdeAmount > 0, "convertToAssets");
        return (usdeAmount * usdePriceInUsd) / (10 ** usdePriceDecimals);
    }
}

Both cases can lead to consistent reverts or undesired values, either because of PriceOracle: update delay exceeds threshold, in case of heart-beat interval misconfiguration or unchecked answer value for usdePriceInUsd, which is obtained by a call to the parentOracle and not verified against 0 (zero).

To reproduce this vulnerability, consider that no customized delay is enforced for sUSDe collateral, falling back to the defaulted value in the contract. Call getPrice in the sUSDeOracle.sol contract or any function that would subsequently fetch the price using this oracle.

• As no custom delay is enforced for these assets (correct is 86400 seconds), all the transactions depending on this feed information will consistently revert with messagePriceOracle: update delay exceeds threshold.

PoC Code:

    function test_susde_oracle_get_price() public {
        vm.startPrank(attacker);
        uint256 susde_price_custom_oracle = susde_oracle.getPrice();
        console.logString("sUSDe Reported price in Custom Oracle (sUSDE.sol) is:");
        console.logUint(susde_price_custom_oracle);
        vm.stopPrank();
    }

Stack traces:

    ├─ [33885] sUSDeOracle::getPrice() [staticcall]
    │   ├─ [30916] TransparentUpgradeableProxy::getAssetPriceInBase(0x4c9EDD5852cd905f086C759E8383e09bff1E68B3) [staticcall]
    │   │   ├─ [25949] PriceOracle::getAssetPriceInBase(0x4c9EDD5852cd905f086C759E8383e09bff1E68B3) [delegatecall]
    │   │   │   ├─ [15735] 0xa569d910839Ae8865Da8F8e70FfFb0cBA869F961::latestRoundData() [staticcall]
    │   │   │   │   ├─ [7502] 0xB735cC58d71dEAc4cfC46dE68d3b04988F7D7b2d::latestRoundData() [staticcall]
    │   │   │   │   │   └─ ← [Return] 88, 99895379 [9.989e7], 1721339447 [1.721e9], 1721339447 [1.721e9], 88
    │   │   │   │   └─ ← [Return] 18446744073709551704 [1.844e19], 99895379 [9.989e7], 1721339447 [1.721e9], 1721339447 [1.721e9], 18446744073709551704 [1.844e19]
    │   │   │   └─ ← [Revert] revert: PriceOracle: update delay exceeds threshold
    │   │   └─ ← [Revert] revert: PriceOracle: update delay exceeds threshold
    │   └─ ← [Revert] revert: PriceOracle: update delay exceeds threshold
    └─ ← [Revert] revert: PriceOracle: update delay exceeds threshold

It is recommended to set proper delay, accordingly to the heart-beat of the targeted feed. Additionally, it is recommended to revert the call in case the oracle's answer is 0 (zero).

SOLVED: The DappOs team solved this issue through the commit hashes 22b22ada6cbc33a1f7abee1d17107d4d99362de1.

The RedStoneInETHOracle.sol contract is designed for ETH pairs, which have different heart-beats than the hard-coded 6-hour PRICE_UPDATE_DELAY value in the contract under analysis. According to the RedStone Price Feeds documentation, most ETH pairs in RedStone, such as ETH-WETH, operate with a 24-hour heartbeat.

Using a hard-coded value for PRICE_UPDATE_DELAY can lead to reverts due to "Data is outdated" in price feeds with longer heart-beats, such as 24 hours. To prevent this issue, it is recommended to use a dynamic value that accurately reflects the heart-beat of the specific ETH pair being used.

- contracts/intentETH/RedStoneInETHOracle.sol

    function getPrice() external view override returns (uint256 price) {
        uint256 latestUpdateTime = IRedstoneAdapter(feed)
            .getBlockTimestampFromLatestUpdate();
        require(
            latestUpdateTime + PRICE_UPDATE_DELAY >= block.timestamp,
            "Data is outdated"
        );
        uint256 basePrice = IPriceCalculator(feed).priceOfETH();
        uint256 tokenPrice = token == address(0)
            ? basePrice
            : IPriceCalculator(feed).priceOf(token);
        return (tokenPrice * 10 ** 18) / basePrice;
    }

Additionally, the affected contract does not validate the answer values for basePrice and tokenPrice against zero, which should be enforced to prevent undesired and unpredicted behavior.

To reproduce this vulnerability, call getPrice in the RedStoneInETHOracle.sol contract. By default, the call should revert because of heart-beat misconfiguration, as follows:

PoC Code:

    function test_redstone_eth_get_price() public {
        vm.startPrank(ghost);
        redstone_eth_oracle.getPrice();
        vm.stopPrank();
    }

Stack traces:

    ├─ [12848] RedStoneInETHOracle::getPrice() [staticcall]
    │   ├─ [9604] 0x8751F736E94F6CD167e8C5B97E245680FbD9CC36::getBlockTimestampFromLatestUpdate() [staticcall]
    │   │   ├─ [2447] 0x031A4f6342175c6B1207c98575258be3B55407EB::getBlockTimestampFromLatestUpdate() [delegatecall]
    │   │   │   └─ ← [Return] 1721289827 [1.721e9]
    │   │   └─ ← [Return] 1721289827 [1.721e9]
    │   └─ ← [Revert] revert: Data is outdated
    └─ ← [Revert] revert: Data is outdated

Enforce the correct price update delay based on each asset heat-beat on corresponding price feed in RedStone ecosystem. In addition, it is recommended to ensure that the values for basePrice and tokenPrice (answers) are not zero (0).

The official documentation should be used as a reference.

SOLVED: The DappOs team solved this issue through the commit hashes c5f1af0b8aa1f131c9d0fc104e25b515de87cdb5 and 728fd1c04228b911fda135aee98f1e925704fcff.

The current implementation of the sDaiMintingPlugin.sol contract utilizes the approve method to set allowances for ERC20 tokens.

However, this approach may cause a revert if the target ERC20 token employs a non-standard implementation with a different function signature for the approve() function.

- contracts/intentUSD/sDaiMintingPlugin.sol

    function execute(
        address tokenToReceive,
        address inputToken,
        uint256 inputAmount,
        bytes memory
    ) external override {
        address dai = ISavingsDai(sDAI).asset();
        require(dai == inputToken, "SDaiMintingPlugin: Invalid input token");
        IERC20(inputToken).approve(sDAI, inputAmount);
        ISavingsDai(sDAI).deposit(inputAmount, tokenToReceive);
    }

In the example above, if inputToken is a non-standard ERC-20 implementation, the transaction will revert consistently because of mis-handle of the approve function.

To ensure compatibility with all ERC20 tokens, including those with non-standard implementations, it is recommended to use SafeERC20's forceApprove method instead. This will provide a more robust solution for managing token allowances across various ERC20 token contracts.

SOLVED: The DappOs team solved this issue through the commit hash c6b4b09904ea8d8a542cf6e8604ad090636aabde.

The redeem function's behavior in the CachePool.sol contract can be manipulated with donation attacks. The function relies on the _getTokenBalance helper function to check the contract's balance for the underlying asset. If the balance is equal to or greater than the requested underlyingAssetAmount, the _redeem function is called, and the redemption is processed immediately (returning 0).

- contracts/core/pools/cachePool/CachePool.sol

    function redeem(
        address user,
        uint256 intentTokenAmount,
        address underlyingAsset,
        uint256 underlyingAssetAmount
    )
        external
        nonReentrant
        onlyIntentTokenMinting
        whenNotPaused
        returns (uint256)
    {
        TransferHelper.safeTransferFrom(
            address(intentToken),
            user,
            address(this),
            intentTokenAmount
        );
        if (Common._getTokenBalance(underlyingAsset) >= underlyingAssetAmount) {
            _redeem(
                user,
                underlyingAsset,
                underlyingAssetAmount,
                intentTokenAmount
            );

            // We return 0 to indicate a complete redemption
            return 0;
        } else {
            RequestData memory _requestData = RequestData(
                underlyingAsset,
                underlyingAssetAmount,
                intentToken,
                intentTokenAmount,
                block.timestamp + claimExpiryPeriod,
                RequestStatus.CREATED
            );
            requests[user][++requestId] = _requestData;
            emit RequestSent(
                user,
                _requestData.underlyingAsset,
                _requestData.intentToken,
                requestId
            );

            return requestId;
        }
    }

However, if the contract's balance is lower than the requested amount, a redemption request is created, and the function returns a requestId.

In this scenario, a malicious actor could donate the required ERC-20 underlying asset (or native ETH) directly to the CachePool contract. This donation would bypass the redemption request mechanism, as described in the specification document.

The specification states that if the CachePool has sufficient funds and meets certain rules, users can immediately obtain the underlying assets. Otherwise, a request is created, and users must wait for the administrator to deposit funds into the CachePool before they can claim the underlying assets.

The donation of underlyingAsset allows the malicious actor to bypass this process. Since there is no apparent financial incentive for the attacker, the severity of this vulnerability is low.

Use internal methods (like mappings) for tracking balances instead of balanceOf and/or balance.

RISK ACCEPTED: The DappOs team accepted the risk related to this finding.

In the PriceOracle.sol contract, it was identified an unprotected initializer, which is susceptible to front-running attacks.

- contracts/core/utils/oracle/PriceOracle.sol

    function initialize(address admin) external initializer {
        _grantRole(DEFAULT_ADMIN_ROLE, admin);
    }

Use _disableInitializers() in the constructor in order to prevent initialization front-running.

RISK ACCEPTED: The DappOs team accepted the risk related to this finding.

The checkWear function in the WearChecker.sol contract lacks proper access control, allowing any low-privileged user to call the function and manipulate the dayUsage state variable. This vulnerability enables an attacker to write malicious state data, effectively disrupting all transactions processed through plugins in the CachePool.sol contract.

- contracts/checker/WearChecker.sol

    function checkWear(
        address tokenToReceive,
        address tokenToUse,
        uint256 receivedAmount,
        uint256 usedAmount
    ) external returns (bool) {
        uint256 receivedValueByIntentToken = Calculator
            .getIntentTokenAmountByUnderlyingAsset(
                tokenToReceive,
                receivedAmount,
                intentToken,
                intentToken.priceOracle()
            );
        uint256 usedValueByIntentToken = Calculator
            .getIntentTokenAmountByUnderlyingAsset(
                tokenToUse,
                usedAmount,
                intentToken,
                intentToken.priceOracle()
            );

        uint256 wearValueByIntentToken = receivedValueByIntentToken >=
            usedValueByIntentToken
            ? 0
            : usedValueByIntentToken - receivedValueByIntentToken;

        if (
            wearValueByIntentToken * maxSingleTransactionWearRate >
            usedValueByIntentToken
        ) {
            return false;
        }
        uint256 wearValue = intentToken.toBaseAssetAmount(
            wearValueByIntentToken
        );

        uint256 today = block.timestamp / 86400;
        if (today != dayUsageLastUpdateDay) {
            dayUsageLastUpdateDay = today;
            dayUsage = 0;
        }

        if (wearValue + dayUsage > dayUsageTotalLimit) {
            return false;
        }
        dayUsage += wearValue;
        return true;
    }

By sending malicious calls to the checkWear function, an attacker can force the daily limit to be reached. Consequently, the checkWear function will return false, halting the execution of the processPluginTransaction function in the CachePool.sol contract.

- contracts/core/pools/cachePool/CachePool.sol

    function processPluginTransaction(
        address tokenToReceive,
        address tokenToUse,
        uint256 usedAmount,
        IExecutionPlugin pluginContract,
        bytes memory data
    ) external onlyRole(TRANSFER_ROLE) nonReentrant whenNotPaused {
        require(plugins[pluginContract], "CachePool: Unauthorized plugin");
        uint256 balanceBefore = Common._getTokenBalance(tokenToReceive);

        TransferHelper.safeTransfer2(
            tokenToUse,
            address(pluginContract),
            usedAmount
        );
        pluginContract.execute(tokenToReceive, tokenToUse, usedAmount, data);

        uint256 receivedAmount = Common._getTokenBalance(tokenToReceive) -
            balanceBefore;
        require(
            wearChecker.checkWear(
                tokenToReceive,
                tokenToUse,
                receivedAmount,
                usedAmount
            ),
            "CachePool: Excessive wear"
        );
        emit PluginExecuted(
            tokenToReceive,
            tokenToUse,
            usedAmount,
            receivedAmount,
            pluginContract
        );
    }

This disruption can lead to denial of service, as all transactions processed through processPluginTransaction will consistently revert, because the require statement won't be met, considering checkWear will return false after the limit is manipulated by the attacker.

It is recommended to add proper access control to the checkWear function in the WearChecker.sol contract, in order to prevent low-privileged users to call this function, preventing potential deviations from the original intended behavior.

SOLVED: The DappOs team solved this issue through the commit hash d33618340d547ed9f61baa3e838483e9432108b8.

In-scope there are upgradeable contracts, and therefore, it is recommended to use the __gap state variable to ensure that the state of the contract will be preserved across upgrades.

In any of the upgradeable contracts, neither on their Storage contract, was identified the uint256[50] private __gap variable.

As by convention, and to preserve the functionalities of the gap variable, which are, keeping consistency of the contract's storage layout across updates, it must be inserted at the end of the contract, or of the Storage contract of each upgradeable contract.

Place the uint256[50] private __gap; variable at the end of upgradeable contracts or their Storage contract.

RISK ACCEPTED: The DappOs team accepted the risk related to this finding.

The contracts in-scope are safe-guarded by an access control mechanism, which is a good practice in smart contract development and prevents non-authorized parties and bad actors to perform unauthorized activities within the contracts and ecosystem.

Owners with privileged rights to perform administrative operations need to be trusted and have power mitigated to prevent unilateral actions.

It is recommended to enforce a Multi-signature mechanism, in order to mitigate potential centralization concerns regarding administrative tasks.

SOLVED: The DappOs team informed that is mitigating centralization risk through a multi-signature wallet.

To enhance the security and flexibility of ownership transfer in smart contracts, it is recommended to use a two-step ownership transfer process. This approach allows for a more controlled transfer of ownership, reducing the risk of unintended consequences or lost access to contract functionalities.

OpenZeppelin provides a convenient implementation of this pattern in the form of the Ownable2Step contract. By using Ownable2Step, you can separate the process of ownership transfer into two distinct steps: proposing a new owner and accepting the proposed ownership.

Implement Ownable2Step instead of Ownable, for enhanced security when transferring contract's ownership.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

Contracts MainPool.sol, CachePool.sol, IntentTokenMinting.sol and IntentTokenBridge.sol utilize OpenZeppelin's Role-Based Access Control (RBAC) to manage function permissions. One of the roles in these contracts is the PAUSE_ROLE, which is responsible for controlling the ability to pause and unpause the contracts. However, during contract initialization, the PAUSE_ROLE is not assigned to any entity, be it a contract or a wallet.

It is recommended to grant a PAUSE_ROLE to at least one entity, that will be capable of pausing and unpausing the contract since the initialization.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

The MainPool.sol and CachePool.sol contracts leverage OpenZeppelin's (OZ) Role-Based Access Control (RBAC) for managing function permissions.

During both contracts' initialization, the DEFAULT_ADMIN_ROLE is assigned, but the TRANSFER_ROLE is not attributed to any entity, be it a contract or a wallet. As a consequence, functions that require the caller to have the TRANSFER_ROLE will consistently revert, rendering them inoperable.

- contracts/core/pools/cachePool/CachePool.sol

    function initialize(address admin) external initializer {
        __AccessControl_init();
        __ReentrancyGuard_init();
        _grantRole(DEFAULT_ADMIN_ROLE, admin);
    }

- contracts/core/pools/mainPool/MainPool.sol

    function initialize(
        address admin,
        IIntentToken _intentToken
    ) external initializer {
        intentToken = _intentToken;
        emit IntentTokenSet((address(_intentToken)));
        __AccessControl_init();
        __Pausable_init();
        __ReentrancyGuard_init();
        _grantRole(DEFAULT_ADMIN_ROLE, admin);
    }

Specifically, functions transferToMainPool, processPluginTransaction and callWhitelisted in the CachePool.sol, and callWhitelisted and transferToWhitelist in the MainPool.sol are affected by this issue.

It is recommended to assign the TRANSFER_ROLE to the appropriate entities during contract initialization. This will enable authorized parties to execute the affected functions as intended.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

The IntentToken.sol contract employs OpenZeppelin's Role-Based Access Control (RBAC) to manage function permissions. Specifically, the mint function requires the caller to possess the MINT_ROLE. During contract initialization, the DEFAULT_ADMIN_ROLE is assigned through the initialize function.

    function initialize(
        address admin,
        string memory name,
        string memory symbol,
        address baseAsset_
    ) external initializer {
        __AccessControl_init();
        __Pausable_init();
        __IntentToken_init(name, symbol, baseAsset_);

        _grantRole(DEFAULT_ADMIN_ROLE, admin);
    }

However, the contract does not attribute the MINT_ROLE to any party or entity. As a consequence, the mint function becomes impossible to execute, as there is no match for the required role, resulting in no allowed callers to the function.

To resolve this issue, it is essential to assign the MINT_ROLE to at least one address during the contract's initialization. This will ensure that the mint function can be executed by authorized parties, enabling the proper functioning of the contract.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

Using require / revert statements within a loop can pose a risk to transaction processing, as a single problematic item can cause the entire transaction to fail. This behavior is noticed in the callWhitelisted function of the MainPool.sol contract, as follows:

- contracts/core/pools/mainPool/MainPool.sol

    function callWhitelisted(
        address to,
        ICalldataValidator calldataValidator,
        bytes calldata data
    )
        public
        payable
        override(WhitelistedCaller)
        onlyRole(TRANSFER_ROLE)
        nonReentrant
        whenNotPaused
        returns (bool success, bytes memory returnData)
    {
        (success, returnData) = super.callWhitelisted(
            to,
            calldataValidator,
            data
        );
        require(success, "MainPool: Call failed");
        uint256 underlyingAssetsLength = underlyingAssets.length();
        for (uint256 i = 0; i < underlyingAssetsLength; ++i) {
            require(
                Common._getTokenBalance(underlyingAssets.at(i)) >=
                    totalDeposited[underlyingAssets.at(i)],
                "MainPool: Insufficient balance"
            );
        }
    }

While iterating over the underlyingAssets array in the given function, a potential issue arises when using require statements within the loop. If the asset at position i does not satisfy the require statement, the entire transaction will fail, even if the subsequent asset at position i+1 would have satisfied the requirement.

Consider handling cases where the requirement is not met more gracefully, using try/catch or exclude them from the iteration by creating an array of only valid assets, which balance would satisfy the requirement.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

In the transferToMainPool function of the CachePool.sol contract, multiple transfers are executed inside a loop. This implementation can lead to reverted transactions if any individual transfer fails, causing the entire operation to revert.

The function accepts two arrays, tokens and amounts, and iterates over them to perform transfers using the safeTransfer2 function from TransferHelper. If any transfer fails due to insufficient balance, invalid token contracts, or other unforeseen issues, the entire transaction will be reverted.

- contracts/core/pools/cachePool/CachePool.sol

    function transferToMainPool(
        address[] memory tokens,
        uint256[] memory amounts
    ) external onlyRole(TRANSFER_ROLE) whenNotPaused nonReentrant {
        require(
            tokens.length == amounts.length,
            "CachePool: Mismatched array lengths"
        );
        for (uint256 i = 0; i < tokens.length; ++i) {
            TransferHelper.safeTransfer2(
                tokens[i],
                address(intentToken.mainPool()),
                amounts[i]
            );
            emit TransferToMainPool(tokens[i], amounts[i]);
        }
    }

Consider excluding the tokens without sufficient balance from the iteration, in order to prevent undesired reverts.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

In order to avoid reentrancy in other modifiers, the nonReentrant modifier should be placed before all other modifiers.

The behavior was identified in the following instances:

- contracts/core/pools/cachePool/CachePool.sol

	    ) external onlyRole(TRANSFER_ROLE) whenNotPaused nonReentrant {

- contracts/core/pools/mainPool/MainPool.sol

	    ) external payable whenNotPaused whenIsNotCollectingStats nonReentrant {

Shift the nonReentrant modifier so it is the first modifier on the function declaration.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

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.

During the analysis of the contracts in-scope, it was identified multiple (22) instances of event fields that could have indexed attribute.

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

    event Indexed(
        address indexed from,
        bytes32 indexed id,
        uint indexed value
    );

ACKNOWLEDGED: The DappOs team acknowledged this issue.

During the analysis of the BaseCalldataValidator.sol contract, it was identified a possible typo in the oladEthAmount, which is believed to be oldEthAmount correct in this case.

- contracts/checker/common/BaseCalldataValidator.sol

    event MaxEthAmountChanged(
        uint256 indexed newEthAmount,
        uint256 indexed oladEthAmount
    );

Make sure to avoid typos in declarations for enhanced code readability.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

During the analysis of the smart contracts in-scope, it was identified that some Custom Errors are not being used. The following instances were detected:

- contracts/core/utils/crossChainMessageRelayer/LzSenderReceiver.sol

error NotEnoughNative(uint256 msgValue);

- contracts/core/utils/crossChainMessageRelayer/LzSenderReceiver.sol

error LzTokenUnavailable();

- contracts/core/utils/crossChainMessageRelayer/OptionsBuilder.sol

error InvalidSize(uint256 max, uint256 actual);

Unused custom errors should have their definition removed.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

Using explicit size declarations helps avoid confusion and ensures that developers are aware of the exact variable size they are working with. This practice promotes better code quality, reduces the potential for errors, and enhances the overall maintainability of the smart contract.

- contracts/checker/common/ToValidator.sol

for (uint i = 0; i < length; i++) {

- contracts/core/libraries/TransferHelper.sol

uint value

To maintain consistency and improve code readability within a contract, opt for uint256 and int256 instead of the implicit uint and int declarations.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

If a modifier is invoked only a single time within a contract, it may be more efficient and cleaner to integrate the modifier's logic directly into the function's body.

- contracts/core/pools/cachePool/CachePool.sol

modifier onlyIntentTokenMinting() {

- contracts/core/pools/mainPool/MainPool.sol

modifier onlyIntentToken() {

Consider incorporating the modifier logic inside the function that is using it.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

Solc compiler version 0.8.20 switches the default target EVM version to Shanghai. The generated bytecode will include PUSH0 opcodes. The recently released Solc compiler version 0.8.25 switches the default target EVM version to Cancun, so it is also important to note that it also adds-up new opcodes such as TSTORE, TLOAD and MCOPY.

Be sure to select the appropriate EVM version in case you intend to deploy on a chain apart from Ethereum mainnet, like L2 chains that may not support PUSH0, TSTORE, TLOAD and/or MCOPY, otherwise deployment of your contracts will fail.

It is important to consider the targeted deployment chains before writing immutable contracts because, in the future, there might exist a need for deploying the contracts in a network that could not support new opcodes from Shanghai or Cancun EVM versions.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

Magic numbers are direct numerical or string values used in the code without any explanation or context. This practice can lead to code maintainability issues, potential errors, and difficulty in understanding the code's logic.

By using a constant state variable, you can enhance code clarity, reduce the potential for errors, and facilitate easier updates. If the value needs to be changed, you can modify it in a single location, rather than searching for and updating each instance of the magic number. This practice promotes better code quality and simplifies the maintenance process.

- contracts/core/libraries/AddressString.sol

stringBytes.length != 42 ||

- contracts/core/libraries/AddressString.sol

for (uint256 i = 2; i < 42; ++i) {

- contracts/core/libraries/AddressString.sol

else if ((stringByte >= 48) && (stringByte <= 57)) stringByte -= 48;

- contracts/intentETH/BaseCustomizedOracle.sol

return 18;

- contracts/intentETH/RedStoneInETHOracle.sol

return (tokenPrice 10 * 18) / basePrice;

- contracts/intentETH/RedStoneInETHOracle.sol

return 18;

To improve code maintainability, readability, and reduce the risk of potential errors, it is recommended to replace magic numbers with well-defined constants. By using constants, developers can provide clear and descriptive names for specific values, making the code easier to understand and maintain. Additionally, updating the values becomes more straightforward, as changes can be made in a single location, reducing the risk of errors and inconsistencies.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

In some of the smart contracts in-scope, values are assigned to address state variables without checking whether the assigned address is the zero address (address(0)). This oversight can lead to unintended behavior and potential security vulnerabilities in the contract.

This behavior was identified in multiple (22) instances. The following is a non-exhaustive list:

- contracts/checker/WearChecker.sol

	        intentToken = _intentToken;

- contracts/core/pools/cachePool/CachePool.sol

	        intentToken = _intentToken;

- contracts/core/pools/cachePool/CachePool.sol

	        wearChecker = _wearChecker;

- contracts/core/pools/cachePool/CachePool.sol

	        intentTokenMinting = _intentTokenMinting;

- contracts/core/pools/cachePool/CachePool.sol

	        plugins[_executionPlugin] = isEnabled;

- contracts/core/pools/mainPool/MainPool.sol

	        intentToken = _intentToken;

- contracts/core/pools/mainPool/MainPoolConfiguration.sol

	        intentToken = _intentToken;

To prevent unintended behavior and potential security vulnerabilities, it is essential to include checks for address(0) when assigning values to address state variables. This can be achieved by adding a simple check to ensure that the assigned address is not equal to address(0) before proceeding with the assignment.

ACKNOWLEDGED: The DappOs team acknowledged this issue.

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

In multiple (+50) locations it was identified the floating pragma, as follows:

pragma solidity ^0.8.20;

Lock the pragma version and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

ACKNOWLEDGED: The DappOs team acknowledged this issue.