Spokes V1 - Concrete

When the CCCM begins the process of opening a protection policy, the claimProtection function requires the encoded protection information.

The protection policy data is initialized by encoding these parameters using the encodeProtectionDataFromAbsoluteValues method provided in the ProtectionLibV1 library. This data, along with the promised protection amount is then used to open the protection policy for a given user.

        ProtectionDataAbsolute memory protectionDataAbsolute = ProtectionDataAbsolute({
            endTime: uint40(block.timestamp + DURATION),
            numberOfTranches: 3,
            protocolRights: 0,
            remoteProxyRights: 0,
            remoteConcreteerRights: 0,
            remotePublicRights: 0,
            ltvProtectForClaimsInBP: concreteClaimBufferInBP,
            ltvProtectForForeclosureInBP: concreteForeclosureBufferInBP,
            openingFeeInBase: promisedAmountInCollateral.mulDiv(concreteOpeningFeePromisedAmoiuntFractionInWad, WAD),
            cancellationFeeInBase: promisedAmountInCollateral.mulDiv(
                concreteCancellationFeePromisedAmountFractionInWad, WAD
            ),
            trancheAmountInBase: trancheAmountInCollateral,
            trancheFeeInBase: trancheFeeInCollateral
        });

        encodedProtectionData =
            ProtectionLibV1.encodeProtectionDataFromAbsoluteValues(promisedAmountInCollateral, protectionDataAbsolute);
    }

However, when the policy is claimed, no validation is performed to ensure that the claim adheres to the encoded protection data. Additionally, the protection system is designed to ensure that claims are made within a specified buffer from the current LTV position, but this check is also not enforced. This can lead to financial losses due to unauthorized claims, protection being claimed outside the intended LTV buffer, protection claims being too large or executed outside the intended timeframe, and a decrease in protocol integrity.

    function test_ProtectionDataNotChecked() external {
        uint256 supplyAmount = 1000 ether;
        uint256 borrowAmount = 1_500_000 * 10 ** 6;

        address userBlueprint = _createAaveV3UserBlueprint();
        weth.mint(alice, supplyAmount);
        vm.startPrank(alice);
            weth.approve(userBlueprint, supplyAmount);
            usdc.approve(userBlueprint, type(uint256).max);
        vm.stopPrank();
        
        (uint256 encodedProtectionInfo, uint256 promisedAmountInCollateral) = _createConcreteProtectionEncoding(supplyAmount, borrowAmount);

        vm.startPrank(address(cccm));
            //User supply WETH and borrow USDC
            IUserIntervention(userBlueprint).supply(supplyAmount, 0);
            IUserIntervention(userBlueprint).setBorrowToken(address(usdc));
            IUserIntervention(userBlueprint).borrow(borrowAmount, 0);

            //User opens protection policy
            IProtocolIntervention(userBlueprint).openProtection(
                encodedProtectionInfo, concreteForeclosureFeeDebtFractionInWad, promisedAmountInCollateral
            );
            uint256 protectionAmount =
            promisedAmountInCollateral.mulDiv(conreteClaimOneAmountAsPromisedAmountFractionInWad, WAD);
            uint256 protectionFee = promisedAmountInCollateral.mulDiv(conreteClaimOneFeeAsPromisedAmountFractionInWad, WAD);
            assertEq(protectionAmount, 30 ether);
            
            //Duration is set to 30 days
            vm.warp(ProtectionLibV1.getEndTime(encodedProtectionInfo) + 100 days);

            //Number of tranches is set to 3
            for(uint i=0; i < 15; i++){
                IProtocolIntervention(userBlueprint).claimProtection(999 ether, 555 ether, address(0));
        }
        vm.stopPrank();
    }

Implement comprehensive validation checks against the protection policy data to ensure that all claims adhere to the encoded parameters, including verification of the LTV buffer, the number of tranches, and the claim amounts and timeframe.

SOLVED: The Concrete team solved the issue. Additional checks were added to the protocol.

• repayTotalDebtAndCancel: Uses a policy cancellation fee if the issuer is 0, otherwise checks if the cancellation fee is within bounds using ProtectionViewLibV1.checkAmountsFromInfo.

• cancelPolicy: Follows the same approach as repayTotalDebtAndCancel.

• reclaimDebtOrForeclose: A new function for hub contracts that checks ProtectionViewLibV1.surpassCriticalLtvAfterWithdraw and either calls reclaimDebt or foreclose, ensuring foreclosure fees are within bounds via ProtectionViewLibV1.checkForeclosureFeeAmount.

• claimProtection: Verifies with QueryInterventionV1.canClaimProtection and ensures claim amounts and fees are within bounds using ProtectionViewLibV1.checkAmountsFromInfo.

• foreclose: First checks QueryInterventionV1.isLiteForeclosableFromInfo - if not, checks if it is seizable by claims using QueryInterventionV1.isForeclosableByClaims, or by expiration via reclaimDebtOrForeclose, ensuring fees are checked with ProtectionViewLibV1.checkForeclosureFeeAmount.

• openProtection: Ensures the borrow token is set, validates protection data consistency using ProtectionLibV1.validateEncodedProtection, and checks if the promised amount exceeds limits using a formula from PolicyInsightsLibV1.getLowerBoundForMaxPromisedAmountFromInfo.

The canClaimProtection function from UserBase contract is designed to determine whether a user can claim protection under a specific Blueprint. This function evaluates the eligibility based on several parameters, including the end time of the protection, the number of claims already made, the number of available tranches and LTV ratio. 

    function canClaimProtection() external view override(IViewUserState) returns (bool isClaimable) {
        uint256 protectionInfo = _getProtectionInfo();
        if (protectionInfo == 0) return false;
        uint256 liqLtvInWad = _getLiquidationLtvInWad();
        isClaimable = QueryInterventionV1.isClaimable(
            protectionInfo.getEndTime() <= block.timestamp,
            protectionInfo.getNumberOfClaims(),
            protectionInfo.getNumberOfTranches(),
            _getCurrentLtvInWad(),
            liqLtvInWad - protectionInfo.getLtvProtectBufferForClaimsInWad().mulDiv(liqLtvInWad, WAD)
        );
    }

However, the condition protectionInfo.getEndTime() <= block.timestamp, used to check whether the protection period has expired, is inverted. It checks if the protection end time is less than or equal to the current timestamp and returns true if the protection period has already expired. This is in contrary to the intended purpose of verifying whether protection is still active.

    function isClaimable(
        bool isProtected,
        uint8 numberOfClaimsUsed,
        uint8 availableClaims,
        uint256 currentLtvInWad,
        uint256 foreclosureThresholdInWad
    ) internal pure returns (bool) {
        return _isClaimablePrimitive(
            isProtected,
            numberOfClaimsUsed >= availableClaims, // not used up all claims
            currentLtvInWad >= foreclosureThresholdInWad
        ); //
    }

    function _isClaimablePrimitive(bool isProtected, bool hasUsedUpAllClaims, bool crossLtvProtect)
        private
        pure
        returns (bool)
    {
        return isProtected && !hasUsedUpAllClaims && crossLtvProtect;
    }

The same logic is used in the claimProtection function defined in the UserBaseFull contract. As the canClaimProtection function will return false when the position is in the actual protection period, this can result in legitimate claims being incorrectly denied, undermining the functionality of the protection mechanism.

    function test_canClaimProtectionFails() external {
        address userBlueprint = _createAaveV3UserBlueprint();
        // how much the user supplies and borrows
        uint256 supplyAmount = 1000 ether;
        uint256 borrowAmount = 1_500_000 * 10 ** 6;
        (uint256 encodedProtectionInfo, uint256 promisedAmountInCollateral) =
            _createConcreteProtectionEncoding(supplyAmount, borrowAmount);
        
        weth.mint(alice, supplyAmount);
        vm.prank(alice);
        weth.approve(userBlueprint, supplyAmount);
        
        vm.startPrank(address(cccm));
            // Supply WETH as a collateral and borrow USDC tokens
            IUserIntervention(userBlueprint).supply(supplyAmount, 0);
            IUserIntervention(userBlueprint).setBorrowToken(address(usdc));
            IUserIntervention(userBlueprint).borrow(borrowAmount, 0);
            
            //Open a protection policy
            IProtocolIntervention(userBlueprint).openProtection(
                encodedProtectionInfo, concreteForeclosureFeeDebtFractionInWad, promisedAmountInCollateral
            );
            //Simulate a price drop of 10% 
            uint256 newEthPrice = initialEthPriceInBaseUnits.mulDiv(90, 100);
            _updatePrice(aaveSetup.priceOracle, address(weth), newEthPrice);

            //The current LTV is greater than or equal to the liquidation LTV minus a buffer
            uint256 currentLTV = IViewUserState(userBlueprint).getCurrentLtvInWad();
            uint256 liquidationLTV = IViewUserState(userBlueprint).getLiquidationLtvInWad();
            assertGe(currentLTV, liquidationLTV - (liteForeclosureThresholdBufferInWad * liquidationLTV / 1e18));

            //The canClaimProtection indicate that user still cannot claim protection.
            assertEq(IViewUserState(userBlueprint).canClaimProtection(), false);

        vm.stopPrank();
    }   

Correct the logic in the canClaimProtection function by replacing protectionInfo.getEndTime() <= block.timestamp with protectionInfo.getEndTime() >= block.timestamp to ensure valid claims during the protection period are not mistakenly denied.

SOLVED: The Concrete team fixed the issue as recommended. The direction of the inequality has been changed from <= to >=. The canClaimProtection function is now a library function inside the QueryInterventionV1 library.

The supplyBorrowAndProtect function facilitates supplying liquidity, borrowing, and opening a protection policy within a single transaction. It internally calls the _openProtectionPolicy function, passing the provided parameters.

    function supplyBorrowAndProtect(
        uint256 supplyAmount_,
        uint256 borrowAmount_,
        address borrowToken_,
        uint256 protectionInfo_,
        uint256 protectionForeclosureFeeFractionInWad_,
        uint256 promisedAmountInToken_
    ) external virtual override(IProtocolIntervention) {
        bytes32 previousBorrowTokenInfo = _borrowTokenInfo;
        bytes32 collateralTokenInfo = _collateralTokenInfo;
        bool borrowTokenNeedsToBeSet = previousBorrowTokenInfo.getAddress() == address(0);
        if (borrowTokenNeedsToBeSet) {
            // set Borrow token
            _lenderSpecificAssetValidation(collateralTokenInfo.getAddress(), borrowToken_);
            _setBorrowTokenInfo(borrowToken_);
        } else if (previousBorrowTokenInfo.getAddress() != borrowToken_) {
            revert Errors.AssetDivergence();
        }

        bool callerIsProtocol = _callerIsEndpoint();
        bool mayOverwriteExistingProtection = _callerIsRemoteProtocolProxy() || callerIsProtocol;
        if (_getProtectionInfo() != 0 && !mayOverwriteExistingProtection) {
            revert Errors.ProtectionAlreadyOpened();
        }

        if (callerIsProtocol) {
            if (borrowTokenNeedsToBeSet) {
                previousBorrowTokenInfo = _borrowTokenInfo;
            }
            _openProtectionPolicy(
                protectionInfo_,
                protectionForeclosureFeeFractionInWad_,
                promisedAmountInToken_,
                supplyAmount_,
                borrowAmount_,
                collateralTokenInfo,
                previousBorrowTokenInfo
            );
        }

As the borrowAmountInToken was set, the following branch will be executed:

        if (borrowAmountInToken > 0 && borrowTokenInfo.getAddress() != address(0)) {
            newCreditInfo = _updateUserFractions(
                newCreditInfo, _getLenderDebt(false), borrowAmountInToken, PositionInteractionType.Borrow, true
            );

            _borrow(
                borrowTokenInfo.getAddress(),
                borrowAmountInToken,
                Uint8CodecLib.encodeOnlyMode(uint8(SendFundsModality.ONLY_SECOND_STEP))
            );
        }

However, since the ONLY_SECOND_STEP modality in the _borrow function only transfers the provided amount of tokens to the owner, the function will revert because the funds were not sent to the Blueprint beforehand. This will cause the transaction to fail unexpectedly, preventing the operation from executing successfully.

    function test_incorrectModalityBorrow() public {
        address userBlueprint = _createAaveV3UserBlueprint();  
        
        uint256 supplyAmount = 1000 ether;
        uint256 borrowAmount = 1_500_000 * 10 ** 6;

        weth.mint(alice, supplyAmount + 0.5 ether);
        vm.startPrank(alice);
        weth.approve(userBlueprint, supplyAmount + 0.5 ether);
        vm.stopPrank();

        vm.startPrank(address(cccm));

        (uint256 encodedProtectionInfo, uint256 promisedAmountInCollateral) =
            _createConcreteProtectionEncoding(1 ether, 1500e6);

        vm.startPrank(address(cccm));

        //User supplies 1 WETH, and intends to borrow 1500 USDC
        //Transaction reverts due to insufficient funds
        vm.expectRevert();
        IProtocolIntervention(userBlueprint).supplyBorrowAndProtect(1 ether, 1500e6, address(usdc),
            encodedProtectionInfo, concreteForeclosureFeeDebtFractionInWad, promisedAmountInCollateral
        );
    }

Instead of using the ONLY_SECOND_STEP modality, consider using the ONLY_FIRST_STEP modality to borrow assets against the previously supplied collateral.

SOLVED: The Concrete team solved the issue. The functions supplyAndProtect and supplyBorrowAndProtect were removed from the User Blueprint Base contract.

The protocol allows users to open a protection policy for their positions to safeguard against risks that leads to position liquidation.

If the user anticipates issues with their position (such as market volatility), they can claim protection funds by calling the claimProtection function, provided the protection policy was opened beforehand. This function deducts fees, updates the user's credit position, and supplies the remaining credit to the lender. Once the protection is claimed, the protocol records the updated credit information to reflect the newly injected funds.

If a position falls into default, the protocol provides a foreclosure mechanism, allowing the system to liquidate the user's position and settle any outstanding debts. The foreclosure logic depends on the credit and collateral amounts in the user’s position.

However, when a user’s position is liquidated on the lender side the protocol still allows the user to claim protection, even though the position has already been liquidated. This creates a situation where the injected protection funds become trapped within the contract. Since the foreclosure process cannot handle this scenario, the position becomes unrecoverable, leading to assets being stuck in the protocol.

    function test_positionCannotBeForeclosed() public { 
        address userBlueprint = _createAaveV3UserBlueprint();

        uint256 supplyAmount = 1000 ether;
        uint256 borrowAmount = 1_500_000 * 10 ** 6;
        uint256 promisedAmountInCollateral;
        uint256 encodedProtectionInfo;
        (encodedProtectionInfo, promisedAmountInCollateral) = _createConcreteProtectionEncoding(supplyAmount, borrowAmount);
        assertEq(promisedAmountInCollateral, 100 ether);

        weth.mint(alice, supplyAmount);
        vm.prank(alice);
        weth.approve(userBlueprint, supplyAmount);

        vm.startPrank(address(cccm));
            //User supplies WETH collateral and borrow USDC
            IUserIntervention(userBlueprint).supply(supplyAmount, 0);
            IUserIntervention(userBlueprint).setBorrowToken(address(usdc));
            IUserIntervention(userBlueprint).borrow(borrowAmount, 0);
        
            //Protection policy is opened
            IProtocolIntervention(userBlueprint).openProtection(
            encodedProtectionInfo, concreteForeclosureFeeDebtFractionInWad, promisedAmountInCollateral);

            //User claims protection but their position gets liquidated in the lender right before
            IPool(aaveSetup.pool).setUserCollateralBalance(address(userBlueprint), 0);
            assertEq(IViewUserState(userBlueprint).getLenderSuppliedWithInterest(), 0);

            //User claims protection
            IProtocolIntervention(userBlueprint).claimProtection(20 ether, 0, address(0));
            assertEq(IViewUserState(userBlueprint).getLenderSuppliedWithInterest(),20 ether);

            //Position cannot be liquidated
            vm.expectRevert();
            IProtocolIntervention(userBlueprint).foreclose(0, address(0));
        vm.stopPrank();
    }

• Update the foreclosure logic to properly handle cases where protection claims are made after a position has been liquidated on the lender side. 

• Prevent users from claiming protection if their position has already been liquidated.

• Introduce a mechanism to allow recovery of protection funds injected into liquidated positions.

SOLVED: The Concrete team solved the issue. The function reclaimDebtOrForeclose was added to the User Base. It has two branches. Either the policy is expired, in which case the foreclosureFeeFraction is passed in the arguments and used after a check that it is within the bounds of the policy. Alternatively, the policy is not expired. This branch will be executed when funds are locked because of a lender side liquidation.

The _swap function defined in the SwapperUniV3 contract is designed to swap tokens for a given amount and ensure that the swap does not exceed a specified slippage. It is used during the foreclosure process, to swap the collateral to the borrowed asset. 

The function transfers the input tokens from the sender, approves them for the swap router, and determines the best possible path and fees for the swap using the _getPath function.

    function _swap(address fromToken, address toToken, uint256 amount, uint256 amountOutMin)
        internal
        override
        returns (uint256 swappedAmount)
    {
        IERC20(fromToken).transferFrom(msg.sender, address(this), amount);
        IERC20(fromToken).approve(address(SWAP_ROUTER), amount);
        // TransferHelper.safeTransferFrom(fromToken, msg.sender, address(this), amount);
        // TransferHelper.safeApprove(fromToken, address(SWAP_ROUTER), amount);

        (bytes memory path, uint24 bestFee, uint256 bestAmountOut) = _getPath(fromToken, toToken, amount);
        if (amountOutMin == 0) {
            amountOutMin = bestAmountOut.mulDiv(WAD - MAX_SLIPPAGE_IN_WAD, WAD); // consider 0.5% slippage
        }

The amountOutMin value is determined by fetching the best possible output amount via the _getBestFee method. The slippage is then calculated based on this value.

    function _getPath(address fromToken, address toToken, uint256 amount)
        internal
        returns (bytes memory, uint24, uint256)
    {
        (uint24 bestFee0, uint256 bestAmountOut0) = _getBestFee(fromToken, toToken, amount);
        if (bestAmountOut0 > 0) {
            return ("", bestFee0, bestAmountOut0);
        }
        // If no direct swap is possible, try to find a path with WETH
        (uint24 bestFee1, uint256 bestAmountOut1) = _getBestFee(fromToken, WETH, amount);
        (uint24 bestFee2, uint256 bestAmountOut2) = _getBestFee(WETH, toToken, bestAmountOut1);
        if (bestAmountOut1 == 0 || bestAmountOut2 == 0) {
            revert Errors.NoPathFound();
        }
        bytes memory path = abi.encodePacked(fromToken, bestFee1, WETH, bestFee2, toToken);
        return (path, bestFee2, bestAmountOut2);
    }

This code attempts to perform on-chain slippage calculations using Quoter.quoteExactInputSingle. However, this approach is prone to manipulations, because quoteExactInputSingle simulates a swap on-chain, making it vulnerable to sandwich attacks. Attackers can manipulate the price by front-running the transaction, leading to inaccurate slippage calculations. Additionally, the Quoter natspec states that these functions are not gas efficient and should not be called on-chain, which makes the current implementation not only insecure but also inefficient.

    function _getBestFee(address tokenA, address tokenB, uint256 amount)
        internal
        returns (uint24 bestFee, uint256 bestAmountOut)
    {
        for (uint8 i; i < feeTiers.length;) {
            if (hasPool(tokenA, tokenB, feeTiers[i])) {
                uint256 amountOut = QUOTER.quoteExactInputSingle(tokenA, tokenB, feeTiers[i], amount, 0);
                if (amountOut > bestAmountOut) {
                    bestFee = feeTiers[i];
                    bestAmountOut = amountOut;
                }
            }
            unchecked {
                ++i;
            }
        }
    }

Consider implementing an off-chain price feed for slippage checks to minimize the risk of manipulation during swaps.

RISK ACCEPTED: The Concrete team accepted the risk.

The supplyAndProtect function allows for supplying liquidity and opening the protection policy in a single transaction. It calls the internal _openProtectionPolicy function, passing the provided data. The _openProtectionPolicy calculates the opening fee based on the data passed in protectionInfo and checks if it’s greater than the supplied amount.

        // calculate the openingFee in Token
        uint256 openingFeeInToken = protectionInfo.getOpeningFeeFractionInWad().mulDiv(promisedAmountInCollateral, WAD);

        // get treatury and split
        (address claimRouter, bytes32 TreasuryAndRevenueSplit) =
            REMOTE_REGISTRY.getClaimRouterAndEncodedTreasuryAndRevenueSplitInWAD();

        TempAddresses memory tempAddresses;
        tempAddresses.claimRouter = claimRouter;
        tempAddresses.treasury = TreasuryAndRevenueSplit.getAddress();

        // take from the lender supplied collateral
        bool withdrawOpeningFeeFromLender = supplyAmountInToken < openingFeeInToken;

If it's greater, the following branch will be executed:

            } else {
                // fee will be deducted in the next step, but book-keeping will be done here
                newCreditInfo = _updateUserFractions(
                    newCreditInfo, totalSuppliedBefore, supplyAmountInToken, PositionInteractionType.Supply, true
                );

                // supply the collateral
                _supply(
                    collateralToken,
                    supplyAmountInToken,
                    Uint8CodecLib.encodeOnlyMode(uint8(SendFundsModality.ONLY_SECOND_STEP))
                );
            }

However, since the ONLY_SECOND_STEP modality in the _supply function only approves the provided amount and supplies it to the lender, this function will revert because the funds were not sent beforehand. This will result in the transaction failing unexpectedly, preventing the operation from being successfully executed.

    function test_incorrectModality() public {
        address userBlueprint = _createAaveV3UserBlueprint();  
        
        uint256 supplyAmount = 1000 ether;
        uint256 borrowAmount = 1_500_000 * 10 ** 6;

        weth.mint(alice, supplyAmount + 0.5 ether);
        vm.startPrank(alice);
        weth.approve(userBlueprint, supplyAmount + 0.5 ether);
        vm.stopPrank();


        //User supplies WETH collateral and borrow USDC
        vm.startPrank(address(cccm));
            IUserIntervention(userBlueprint).supply(supplyAmount, 0);
            IUserIntervention(userBlueprint).setBorrowToken(address(usdc));
            IUserIntervention(userBlueprint).borrow(borrowAmount, 0);
        vm.stopPrank();

            (uint256 encodedProtectionInfo, uint256 promisedAmountInCollateral) =
                _createConcreteProtectionEncoding(supplyAmount, borrowAmount);

        vm.startPrank(address(cccm));
            //User supplies only 0.5 WETH, which is less than the required fee
            //Transaction reverts due to insufficient funds
            vm.expectRevert();
            IProtocolIntervention(userBlueprint).supplyAndProtect(0.5 ether, 
                encodedProtectionInfo, concreteForeclosureFeeDebtFractionInWad, promisedAmountInCollateral
            );
    }

Instead of using the ONLY_SECOND_STEP modality, consider using the SEND_THROUGH modality for the described case. If withdrawOpeningFeeFromLender is true, the function will withdraw the opening fee using the updated balance.

SOLVED: The Concrete team solved the issue. The function supplyAndProtect was removed from the User Blueprint Base contract.

The openProtection, SupplyAndProtect, and SupplyBorrowAndProtect functions from the UserBase contract utilize similar logic to validate whether the caller is authorized to perform the intended operation. The logic begins by checking if the caller is the protocol endpoint using _callerIsEndpoint method. If this check is true, it sets the callerIsProtocol flag. Additionally, the logic checks if the caller is a remote protocol proxy using callerIsRemoteProtocolProxy and combines these two checks to determine if the existing protection can be overwritten.

        bool callerIsProtocol = _callerIsEndpoint();
        bool mayOverwriteExistingProtection = _callerIsRemoteProtocolProxy() || callerIsProtocol;
        if (_getProtectionInfo() != 0 && !mayOverwriteExistingProtection) {
            revert Errors.ProtectionAlreadyOpened();
        }
        if (callerIsProtocol) {
	        LOGIC HERE
            );
        } else {
            revert Errors.NotProtocolAndNotIntervenable();
        }

However, the logic incorrectly restricts the execution of the function's core logic to only when the caller is the protocol endpoint. If the caller is a valid remote proxy, the function will incorrectly prevent the remote proxy from executing its intended functionality, causing disruptions or unintended denials of service.

    function test_remoteProtocolProxyReverts() external {
        address userBlueprint = _createAaveV3UserBlueprint();
        vm.prank(address(cccm));
            IUserIntervention(userBlueprint).setBorrowToken(address(usdc));
        uint256 supplyAmount = 1000 ether;
        uint256 borrowAmount = 1_500_000 * 10 ** 6;
        (uint256 encodedProtectionInfo, uint256 promisedAmountInCollateral) =
            _createConcreteProtectionEncoding(supplyAmount, borrowAmount);

        //Expect a revert if the remote proxy is the caller
        vm.prank(address(remoteProtocolProxy));
            vm.expectRevert(Errors.NotProtocolAndNotIntervenable.selector);
            IProtocolIntervention(userBlueprint).openProtection(
                encodedProtectionInfo, concreteForeclosureFeeDebtFractionInWad, promisedAmountInCollateral);

        vm.prank(address(remoteProtocolProxy));
            vm.expectRevert(Errors.NotProtocolAndNotIntervenable.selector);
            IProtocolIntervention(userBlueprint).supplyAndProtect(
                supplyAmount, encodedProtectionInfo, liteForeclosureFeeFractionInWad, promisedAmountInCollateral);

        vm.prank(address(remoteProtocolProxy));
            vm.expectRevert(Errors.NotProtocolAndNotIntervenable.selector);
            IProtocolIntervention(userBlueprint).supplyBorrowAndProtect(
                supplyAmount, borrowAmount, address(usdc), encodedProtectionInfo, liteForeclosureFeeFractionInWad, promisedAmountInCollateral);
    }

Modify the access control logic to ensure that both the protocol endpoint and the remote proxy are allowed to execute the functions. Implement a function that allows the Remote Proxy to claim rewards as intended by the logic of the claimRewards function.

SOLVED: The Concrete team solved the issue. The mentioned logic was removed from the functions.

The ChainlinkPriceProvider contract uses Chainlink feed as their price oracle. When requesting the price via the _getPrice() function, the contracts query Chainlink for the underlying token price using the latestRoundData() function. This function returns uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt and uint80 answeredInRound.

• roundId denotes the identifier of the most recent update round,

• answer is the price of the asset,

• startedAt is the timestamp at which the round started.

• updatedAt is the timestamp at which the feed was updated,

• answeredInRound is already deprecated by Chainlink.

    function _getPrice(address priceFeedAddress) internal view override returns (uint256) { 
        (, int256 price,,,) = AggregatorV3Interface(priceFeedAddress).latestRoundData();
        return uint256(price);
    }

The getPrice() function does not check if the feed was updated at the most recent round nor does it verify that startedAt (timestamp at which the round started) is not 0, and this can result in accepting stale data which may threaten the stability of the protocol in a volatile market.

Modify the function to utilize Chainlink's latestRoundData. This approach provides detailed information about the most recent price round, ensuring that retrieved the price remains up-to-date. Also, consider saving all used the Chainlink aggregators into a mapping and using different grace period for different heartbeats.

Example implementation:

uint256 private constant GRACE_PERIOD_TIME = 3600; // duration until the price is considered stale

function getChainlinkPrice(AggregatorV2V3Interface feed) internal {
    (uint80 roundId, int256 price,, uint updatedAt,) = feed.latestRoundData();
    require(price > 0, "Invalid price");
    require(block.timestamp <= updatedAt + GRACE_PERIOD_TIME, "Stale price");
    return price;
}

SOLVED: The Concrete team fixed the issue. The check for stale prices was added. The 1-hour grace period was used. For L2 chains, the team have added sequencer check, where it checks if the sequencer is up and running as well as updatedAt timestamp falls within the grace period.

The RemoteProtocolProxy contract is intended to allow foreclosure and claim protection for Blueprint owners with Concreteer privileges. However, both the foreclose and claimProtection functions implemented in the Blueprint do not use the _callerIsRemoteProtocolProxy to allow the Remote Proxy to call, and are callable only by the endpoint, making this functionality broken.

function foreclose(uint256 foreclosureFeeFractionInWad, address beneficiary)
        external
        virtual
        override(IProtocolIntervention)
    {
        // _foreclose();
        uint256 protectionInfo = _getProtectionInfo();
        if (_callerIsEndpoint() && protectionInfo.getProtocolEnabledForClosureAndForeclosure()) {
            _requestFlashLoan(
                _borrowTokenInfo.getAddress(), _getLenderDebt(false), foreclosureFeeFractionInWad, beneficiary
            );
            return;
        }

        revert Errors.UnauthorizedTriggerOfForeclosure();
    }

    function claimProtection(uint256 amount, uint256 protectionFeeInToken, address externalBeneficiary)
        external
        virtual
        override(IProtocolIntervention)
    {
        uint256 protectionInfo = _getProtectionInfo();
        // if sender is endpoint then claim protection
        if (_callerIsEndpoint() && protectionInfo.getProtocolEnabledForClaimsAndReclaims()) {
            // external Beneficiary is disregarded.
            protectionInfo = _claimProtectionDefault(protectionInfo, amount, protectionFeeInToken);
            _setProtectionInfo(protectionInfo);
            return;
        }
        externalBeneficiary;
        revert Errors.UnauthorizedTriggerOfClaimProtection();
    }

    function test_remoteProtocolProxyReverts() external {
        address userBlueprint = _createAaveV3UserBlueprint();
        vm.prank(address(cccm));
            IUserIntervention(userBlueprint).setBorrowToken(address(usdc));
        uint256 supplyAmount = 1000 ether;
        uint256 borrowAmount = 1_500_000 * 10 ** 6;
        (uint256 encodedProtectionInfo, uint256 promisedAmountInCollateral) =
            _createConcreteProtectionEncoding(supplyAmount, borrowAmount);

        //Expect a revert if the remote proxy is the caller
        vm.prank(address(remoteProtocolProxy));
            vm.expectRevert(Errors.NotProtocolAndNotIntervenable.selector);
            IProtocolIntervention(userBlueprint).openProtection(
                encodedProtectionInfo, concreteForeclosureFeeDebtFractionInWad, promisedAmountInCollateral);

        vm.prank(address(remoteProtocolProxy));
            vm.expectRevert(Errors.NotProtocolAndNotIntervenable.selector);
            IProtocolIntervention(userBlueprint).supplyAndProtect(
                supplyAmount, encodedProtectionInfo, liteForeclosureFeeFractionInWad, promisedAmountInCollateral);

        vm.prank(address(remoteProtocolProxy));
            vm.expectRevert(Errors.NotProtocolAndNotIntervenable.selector);
            IProtocolIntervention(userBlueprint).supplyBorrowAndProtect(
                supplyAmount, borrowAmount, address(usdc), encodedProtectionInfo, liteForeclosureFeeFractionInWad, promisedAmountInCollateral);
    }

Adjust the access of the RemoteProxy to the intended functionalities.

SOLVED: The Concrete team fixed the issue. The mentioned invocation was removed from the contract. The RemoteProtocolProxy connectors and RemotePublicExecutor variables are retained in UserBaseV1 for potential future use.

When a new protection tranche is claimed, the claimProtection method from ProtectionHandler contract, is responsible for updating the credit information. This process involves recalculating the totalCreditInBase and totalCreditInToken values. The method currently attempts to update totalCreditInBase by adding the amount of tokens multiplied by quote denomination (10e8) and then dividing the result by the price returned from the oracle.

            params.creditInfo = _creditInfo;
            (params.totalCreditInBase, params.totalCreditInToken) = params.creditInfo.decodeCreditAmount();
            params.newCreditInfo = params.creditInfo.updateCredit(
                params.totalCreditInBase
                    + params.amountInToken.mulDiv(
                        PRICE_FEED_QUOTE_DENOMINATION, _getAssetPrice(collateralTokenInfo.getAddress())
                    ).mulDiv(BASE_DENOMINATION, collateralTokenInfo.getDenomination()),
                params.totalCreditInToken + params.amountInToken
            );

However, the calculation is performed in the wrong order. The amount of tokens should first be multiplied by the price and then normalized to the intended 10e8 denomination.

Adjust the calculation to first multiply the amountInToken by the asset price to ensure accurate conversion. 

SOLVED: The Concrete team solved the issue. The order of calculations was switched.

The _executeForeclosure function is invoked during the foreclosure flow to execute the logic required to repay unsecured debt. Using the assets withdrawn from the lender, it repays the borrowed amount, subtracts the fee, and sends the remaining collateral back to the position owner.

        // ------ repay to the claimRouter/fundRequester(flashloan) ------
        params.amountInBorrowTokenLeft -= params.totalRepayAmountInToken;
        if (isEarnFlashloan) {
            _increaseAllowanceIfNecessary(params.claimRouter, borrowToken, params.totalRepayAmountInToken);
        } else {
            IERC20(borrowToken).transfer(params.fundRequester, params.totalRepayAmountInToken);
        }
        // ------ deduct foreclosure fee ------
        uint256 foreclosureFeeInBorrow = repayAmount.mulDiv(foreclosureFeeFractionInWad, WAD);
        if (foreclosureFeeInBorrow > params.amountInBorrowTokenLeft) {
            foreclosureFeeInBorrow = params.amountInBorrowTokenLeft;
        }
        params.amountInBorrowTokenLeft -= foreclosureFeeInBorrow;
        _deductFee(
            TempAddresses(params.claimRouter, TreasuryAndRevenueSplit.getAddress(), beneficiary),
            params.borrowTokenInfo,
            foreclosureFeeInBorrow,
            TreasuryAndRevenueSplit.getMax96BitNumber()
        );

        // ------ send back the remaining amount to the owner ------
        IERC20(borrowToken).transfer(_owner(), params.amountInBorrowTokenLeft);

Some tokens implement a blacklist functionality (e.g., USDC), forbidding transfers to or from blocked addresses. Moreover, some tokens revert when attempting to transfer 0 tokens. If the transfer fails for any reason, the entire foreclosure process will revert, preventing the liquidation of the unsecured position.

Consider using the pull pattern. Instead of transferring tokens to the owner's address directly, increase the internal balance of pending withdrawals and introduce a claim functionality that allows the owner to manually withdraw the remaining funds.

        if (params.amountInBorrowTokenLeft > 0) {
            try IERC20(borrowToken).transfer(_owner(), params.amountInBorrowTokenLeft) {}
            catch {
                IERC20(borrowToken).safeIncreaseAllowance(_owner(), params.amountInBorrowTokenLeft);
            }
        }

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example, Tether (USDT)'s transfer() and transferFrom() functions on L1 do not return booleans as the specification requires, and instead have no return value. When these tokens are cast to IERC20, their function signatures do not match, causing calls to revert.

Use OpenZeppelin's SafeERC20 library, which provides safeTransfer() and safeTransferFrom() functions. These functions handle non-standard ERC20 implementations safely, ensuring compatibility with a wider range of tokens.

SOLVED: The Concrete team solved the issue as recommended.

The foreclose function implemented in the UserBase contract is designed to handle the liquidation of a position. It interacts with a flash loan mechanism to acquire the necessary funds for foreclosure, either via a primary method or a fallback, depending on availability. 

The function invokes the _requestFlashLoan method on the funds' requester contract. 

    function foreclose(uint256 foreclosureFeeFractionInWad, address beneficiary)
        external
        virtual
        override(IProtocolIntervention)
    {
        // _foreclose();
        uint256 protectionInfo = _getProtectionInfo();
        if (_callerIsEndpoint() && protectionInfo.getProtocolEnabledForClosureAndForeclosure()) {
            _requestFlashLoan(
                _borrowTokenInfo.getAddress(), _getLenderDebt(false), foreclosureFeeFractionInWad, beneficiary
            );
            return;
        }

        revert Errors.UnauthorizedTriggerOfForeclosure();
    }

The requestFlashLoan function from the FundsRequester attempts to obtain the funds necessary to repay the position on the lending platform, through a claim router. If successful, it proceeds with the foreclosure process and then repays the claim router. If the attempt with the claim router fails, it falls back to using Aave to obtain a flash loan for the required funds.

    function requestFlashLoan(address token, uint256 amountInBorrowToken, uint256 feeFractionInWad, address beneficiary)
        external
        onlyUserBlueprint
    {
        // try and except handling ... first try the earn claim router, then try Aave
        address userBlueprint = msg.sender;
        IClaimRouter claimRouter = IClaimRouter(REMOTE_REGISTRY.getEarnClaimRouter());
        try claimRouter.requestToken(VaultFlags(0), token, amountInBorrowToken, payable(userBlueprint)) {
            // now execute the flashloan
            IProtocolIntervention(userBlueprint).executeForecloseByFundsRequester(
                amountInBorrowToken, 0, feeFractionInWad, beneficiary
            );
            // now pull the funds from the user and repay the claim router
            claimRouter.repay(token, amountInBorrowToken, payable(userBlueprint));
        } catch {
            // fallback to Aave
            if (address(POOL) == address(0)) revert Errors.AavePoolNotSet();

            POOL.flashLoanSimple(
                address(this), token, amountInBorrowToken, abi.encode(userBlueprint, feeFractionInWad, beneficiary), 0
            );
        }
    }

During the flash loan process, the _executeForeclosure function in the ProtectionHandler contract is invoked. It repays the borrowed credit using the flash loaned amount, withdraws the position’s collateral, swaps it into the borrowed token, and then transfers the swapped amount to the fund requester to repay the flash loan.

        params.totalRepayAmountInToken = repayAmount + premium;
        params.collateralAmountLeft = _getLenderSupplied(false);

        // ------ repay lender ------
        _repay(borrowToken, repayAmount, Uint8CodecLib.encodeOnlyMode(uint8(SendFundsModality.ONLY_SECOND_STEP)));
        // ------ withdraw collateral from lender ------
        _withdraw(
            params.collateralToken,
            params.collateralAmountLeft,
            Uint8CodecLib.encodeOnlyMode(uint8(SendFundsModality.ONLY_FIRST_STEP))
        );

        // User has 3 ETH in AAVE, he has 5000 USDC debtToken in AAVE. OF THE 3 ETH, 0.5 ETH are credit injections (ConcreteDebt of the user). Then there is a foreclosure with a fee of 1%.
        // 1. We request 5000 USDC from the fundsRequester
        // 2. Either we get it from claimRouter or from AaveFlashloan
        // 3. _executeForeclosure is called with uint256 repayAmount, uint256 premium, uint256 foreclosureFeeFractionInWad,
        // 4. We use this money to repay the 5000 USDC debt in AAVE
        // 5. We get the 3 ETH from the AAVE
        // 6. We repay to the claimRouter the 0.5 ETH of users Concrete Debt.
        // 7. We swap as much as possible into USDC of the remaining 2.5 ETH
        // 8. We need to make sure that enough money (repayAmount + premium) is available in the contract in borrow denomination.
        // 9. We deduct the 50 USDC fee from the 5000 USDC (in borrow token)
        // 10 The rest of the ETH and USDC is send back to the user.

        // ------ repay the credit injections ------
        params.concreteDebtInCollateral = _getTotalConcreteDebtOfUser();

        if (params.concreteDebtInCollateral > 0) {
            if (params.concreteDebtInCollateral > params.collateralAmountLeft) {
                params.concreteDebtInCollateral = params.collateralAmountLeft;
            }
            _repayTokenToClaimRouter(params.claimRouter, params.collateralToken, params.concreteDebtInCollateral);
            params.collateralAmountLeft -= params.concreteDebtInCollateral;
        }

        // ------ swap collateral to borrow token ------
        if (params.collateralAmountLeft > 0) {
            params.amountInBorrowTokenLeft = _swap(params.collateralToken, borrowToken, params.collateralAmountLeft, 0);
        }
        if (params.amountInBorrowTokenLeft < params.totalRepayAmountInToken) {
            params.totalRepayAmountInToken = params.amountInBorrowTokenLeft;
        }

        // ------ repay to the claimRouter/fundRequester(flashloan) ------
        params.amountInBorrowTokenLeft -= params.totalRepayAmountInToken;
        if (isEarnFlashloan) {
            _increaseAllowanceIfNecessary(params.claimRouter, borrowToken, params.totalRepayAmountInToken);
        } else {
            IERC20(borrowToken).transfer(params.fundRequester, params.totalRepayAmountInToken);
        }

However, when a position becomes insolvent, the collateral swapped into the borrowed token is insufficient to repay the flash loan. This situation prevents the foreclosure process from successfully liquidating the position, leading to the position being stuck in an insolvent state. When integrating with high-liquidity and high-performance lending protocols like Aave or Compound, this will not be an issue, as positions are liquidated before reaching insolvency. In the future though, it may be necessary to ensure that the Blueprint position can be always liquidated by the protocol.

    function test_CannotLiquidateInsolventPosition() public { 
        address userBlueprint = _createAaveV3UserBlueprint();
        
        uint256 supplyAmount = 1000 ether;
        uint256 borrowAmount = 1_500_000 * 10 ** 6;
        uint256 promisedAmountInCollateral;
        uint256 encodedProtectionInfo;
        (encodedProtectionInfo, promisedAmountInCollateral) = _createConcreteProtectionEncoding(supplyAmount, borrowAmount);
        assertEq(promisedAmountInCollateral, 100 ether);

        weth.mint(alice, supplyAmount);
        vm.prank(alice);
        weth.approve(userBlueprint, supplyAmount);

        vm.startPrank(address(cccm));
            //The user supplies 1000 WETH and borrow 1500000 USDC
            IUserIntervention(userBlueprint).supply(supplyAmount, 0);
            IUserIntervention(userBlueprint).setBorrowToken(address(usdc));
            IUserIntervention(userBlueprint).borrow(borrowAmount, 0);
        vm.stopPrank();

        //Simulate a price drop of 25% 
        uint256 newEthPrice = initialEthPriceInBaseUnits.mulDiv(75, 1000);
        _updatePrice(aaveSetup.priceOracle, address(weth), newEthPrice);

        //The claim router fails to provide funds
        claimRouter.setRevertOnRequest(true);
        vm.prank(address(cccm));
            //Foreclosure reverts because the position is insolvent
            vm.expectRevert();
            IProtocolIntervention(userBlueprint).foreclose(liteForeclosureFeeFractionInWad, address(0));
    }

In cases where the collateral is insufficient to cover the full repayment of the flash loan, consider implementing a mechanism to obtain the remaining required tokens using a separate liquidity designated specifically for such situations.

SOLVED: The Concrete team solved the issue. In cases where the flashloan provider cannot be repaid, the missing amount is pulled from the user blueprint. The user blueprint can hold funds, either deposited by its owner or by Concrete’s treasury. If there are still insufficient funds (or none) in the user blueprint, the missing amount is pulled from the treasury. If this is not possible, the transaction reverts. This setup allows either the addition of funds to the user blueprint to rescue the position or the approval of the blueprint to pull the remaining funds from the treasury.

In order to allow for the transfer of tokens from one address, the protocol calls the approve function using the IERC20 interface in several places. This approach might be problematic for a few reasons:

• Some tokens, to protect against approval race conditions, do not allow approving an amount M > 0 when an existing amount N > 0 is already approved.

• The approve call does not return a boolean.

• The function reverts if the approval value is larger than uint96.

It is recommended to use safeIncreaseAllowance and safeDecreaseAllowance from the SafeERC20 library across the entire protocol, instead of calling approve directly.

SOLVED: The Concrete team fixed the issue as recommended.

The RemoteRegistry contract implements setter functions that allow the modification of critical roles and components within the system. The transfer of these critical roles is done through a single-step process. The address to which ownership is transferred should be verified to ensure it is active and willing to act as the owner. This is especially important for the setEndpointAddress and setNewRegistryAddress functions.

Consider implementing a two-step transfer pattern. In this approach, the privileged account nominates a new address, and the nominated account must then accept the role for the ownership transfer to be fully completed. By separating the process, this ensures that the new account is both active and willing to assume the associated permissions.

SOLVED: The Concrete team fixed the issue. The proposeEndpointAddress, confirmNewEndpointAddress and revokeEndpointProposal function were added. The setNewRegistry function was deleted.

The files in scope currently use floating pragma version ^0.8.24, which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.0, and less than 0.9.0. It is recommended that 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.

Additionally, using a newer compiler version that introduces default optimizations, including unchecked overflow for gas efficiency, presents an opportunity for further optimization.

Lock the pragma version to the same version used during development and testing.

SOLVED: The Concrete team fixed the issue as recommended.

The project is using Solidity 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 will enhance code readability and make the purpose of each mapping more explicit.

Consider refactoring the mappings to use named arguments.

For example, on PortfolioProxy, instead of declaring:
mapping(address => uint256) public _portfolioId;
The mapping could be declared as:
mapping(address userBlueprint => uint256 id) public _portfolioId;

SOLVED: The Concrete team fixed the issue as recommended.

Throughout the code in scope, there are several instances where the imports, errors, and events are declared but never used.

In PriceFeedManagerEvents.sol :

• event PriceProviderRemoved(address indexed token);

  
  

In RemoteRegistryEvent.sol:

• event RemoteChainwideInterventionTypeSetTo(InterventionConstraintType type_);

• event RemoteChainwideAllowUserDirectHandlingSetTo(bool enable);

• event OwnerTransferred(address user, address newOwner);

  
  

In Errors.sol:

• error ProtectionMetadataUnequalArrayLengths();

• error ProtectionMetadataArrayLengthExceedsLimit();

• error NotProtocolAndUserCannotControl();

• error PositionAlreadyProtected();

• error PositionNotProtected();

• error WithdrawalExceedsUserFractionOfFunds();

• error OwnerAddressNotSet();

• error ClaimRouterAddressZero();

• error AssetMustBeCollateral();

• error AssetMustBeBorrowToken();

• error EndpointCannotBeZeroAddress();

• error RemoteProtocolProxyCannotBeZeroAddress();

• error PublicExecutorCannotBeZeroAddress();

• error UnsupportedAsset(address token);

  
  

In RemoteExecutorsBookkeeper.sol:

• import {IProtocolIntervention} from "../userBase/interfaces/IProtocolIntervention.sol";

  
  

In FundsRequester.sol:

• import {IUserBlueprint} from "../userBlueprints/interfaces/IUserBlueprint.sol";

  
  

In Inspector.sol:

• import {IProtocolIntervention} from "../userBase/interfaces/IProtocolIntervention.sol";

• import {IUserIntervention} from "../userBase/interfaces/IUserIntervention.sol";

  
  

In ProtectionViewLib.sol:

• import {Errors} from "../helpers/Errors.sol";

  
  

In PortfolioProxyRemoteTransfer.sol:

• import {IConcreteCrossChainMessaging as ICCCM} from "@cccm-evm/src/interfaces/IConcreteCrossChainMessaging.sol";

• import {IRemoteRegistry} from "../remoteRegistry/interfaces/IRemoteRegistry.sol";

• import {IResponseHandler, IProtocol} from "../userBase/interfaces/IProtocolResponse.sol";

  
  

In CloneFactory.sol:

• import {EndpointAddressGetterStandalone} from "@cccm-evm/src/integration/Getters.sol";

• import {OnlyEndpointStandalone} from "@cccm-evm/src/integration/OnlyEndpoint.sol";

• import {IRemoteRegistry} from "./interfaces/IRemoteRegistry.sol"; import {Errors} from "../helpers/Errors.sol";

  
  

In RemoteRegistry.sol:

• import {OnlyEndpointStandalone} from "@cccm-evm/src/integration/OnlyEndpoint.sol";

• import {InterventionConstraintType} from "../helpers/DataTypes.sol";

In SwapperUniV3.sol:

• import {IRemoteRegistry} from "../../remoteRegistry/interfaces/IRemoteRegistry.sol";

In SwapperBase.sol:

• import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";

  
  

In UserBase.sol:

• import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";

• import {IERC20, IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";

• import {IResponseHandler, IProtocol} from "./interfaces/IProtocolResponse.sol";

In ExecutorConnector.sol:

• import {Errors} from "../../helpers/Errors.sol";

• import {Uint8CodecLib} from "@hub-and-spokes-libs/src/libraries/Uint8CodecLib.sol";

In ProtectionHandler.sol:

• import {IClaimRouter} from "../../fundsRequester/interfaces/IClaimRouter.sol";

• import {InterventionConstraintType} from "../../helpers/DataTypes.sol";

In RemoteRegistryConnector.sol:

• import {InterventionConstraintType} from "../../helpers/DataTypes.sol";

In UserAaveV3.sol:

• import {EthereumMainnetAddresses as ADDR} from "../addresses/EthereumMainnet.sol";

• import {TokenType, PositionInteractionType, SendFundsModality} from "../helpers/DataTypes.sol";

In UserCompoundV3.sol:

• import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";

• import {EthereumMainnetAddresses as ADDR} from "../../src/addresses/EthereumMainnet.sol";

• import {TokenType, PositionInteractionType, SendFundsModality} from "../../src/helpers/DataTypes.sol";

In UserRadiantV2.sol:

• import {EthereumMainnetAddresses as ADDR} from "../addresses/EthereumMainnet.sol";

• import {TokenType, PositionInteractionType, SendFundsModality} from "../helpers/DataTypes.sol";

In UserSiloV1.sol:

• import {IPriceOracleGetter} from "@aave-v3-core/contracts/interfaces/IPriceOracleGetter.sol";

• import {BASE_DENOMINATION, WAD, BP, PRICE_FEED_QUOTE_DENOMINATION} from "../helpers/Constants.sol";

• import {EthereumMainnetAddresses as ADDR} from "../addresses/EthereumMainnet.sol";

• import {TokenType, PositionInteractionType, SendFundsModality} from "../helpers/DataTypes.sol";

Consider removing all unused imports, errors and events.

SOLVED: The Concrete team fixed the issue as recommended.

The SwapperBEX contract is entirely commented out, including both the constructor and the swap function. While the contract structure is present, no functional code is active and the swap function contains only a placeholder comment for future implementation.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {IRemoteRegistry} from "../../remoteRegistry/interfaces/IRemoteRegistry.sol";
import {Errors} from "../../helpers/Errors.sol";
import {SwapperBase} from "../SwapperBase.sol";

// contract SwapperBEX is SwapperBase {
//     constructor(address remoteRegistry_) SwapperBase(remoteRegistry_) {}

//     function _swap(address fromToken, address toToken, uint256 amount)
//         internal
//         override
//         returns (uint256 swappedAmount)
//     {
//         // @Atul, please implement the swap logic here.
//         // If you need some addresses, put them into the constructor.
//     }
// }

Either complete the implementation of the swap logic or remove the unused SwapperBEX contract to reduce unnecessary complexity.

SOLVED: The Concrete team fixed the issue as recommended. The SwapperBEX contract was removed.

In logic of function implemented on SEND_THROUGH modality on some user blueprints, redundant token transfers are made by first withdrawing tokens to the contract and then transferring them to the user. This leads to unnecessary gas costs due to additional transfer steps. By utilizing withdrawTo, claimTo (for Compound V3), withdrawFor and borrowFor (for SiloV1) functions, tokens can be transferred directly to the user, reducing the number of operations and thus gas costs.

Replace the two-step process of withdrawing and transferring tokens with methods that allow direct sending of funds to the user's address whenever possible, to minimize gas usage.

SOLVED: The Concrete team acknowledged the issue.

It has been observed that Blueprint and Remote Registry 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.

All functions updating important parameters should emit events.

SOLVED: The Concrete team fixed the issue as recommended.

The ProtectionLibV1 library incorrectly uses InBase in variable names and function parameters, even though the values should represent collateral decimals. This mismatch in naming convention can cause confusion in the future maintenance.

Update variable names and parameters throughout the ProtectionLibV1 library to reflect that the values are in collateral decimals, ensuring clarity and consistency in the codebase.

SOLVED: The Concrete team fixed the issue as recommended.

The externalBeneficiary parameter in the claimProtection function of UserBase contract is unused and does not impact the function's logic. This parameter can be safely removed without affecting functionality, as it is disregarded when claims are triggered.

    function claimProtection(uint256 amount, uint256 protectionFeeInToken, address externalBeneficiary)
        external
        virtual
        override(IProtocolIntervention)
    {
        uint256 protectionInfo = _getProtectionInfo();
        // if sender is endpoint then claim protection
        if (_callerIsEndpoint() && protectionInfo.getProtocolEnabledForClaimsAndReclaims()) {
            // external Beneficiary is disregarded.
            protectionInfo = _claimProtectionDefault(protectionInfo, amount, protectionFeeInToken);
            _setProtectionInfo(protectionInfo);
            return;
        }
        externalBeneficiary;
        revert Errors.UnauthorizedTriggerOfClaimProtection();
    }

Remove the externalBeneficiary parameter from the claimProtection function to simplify the code and improve clarity.

ACKNOWLEDGED: The Concrete team keep the function signature with the external beneficiary parameter as it will be used in the future versions.