coreBTC - CoreDAO

The contracts, which are designed to be upgradeable using the UUPS pattern, seem to be missing proper initialization functions. The initial setup for a UUPS upgradeable contract is crucial to ensure that the contract can be safely upgraded in the future without losing state or introducing vulnerabilities.

Code Location : https://github.com/coredao-org/btcwrap-contracts/blob/86a3b4f2d0569e1bd520a88356c31076a584ec8d/contracts/common/relay/BitcoinRelayLogic.sol#L11

To address this concern, ensure that all base contracts are properly initialized in the initialize function.

Remediation Plan

SOLVED : The CoreDAO team solved the issue by calling the init function.

The contracts are upgradable, inheriting from the Initializable contract. However, the current implementations are missing the _disableInitializers() function call in the constructors. Thus, an attacker can initialize the implementation. Usually, the initialized implementation has no direct impact on the proxy itself; however, it can be exploited in a phishing attack. In rare cases, the implementation might be mutable and may have an impact on the proxy.

It is recommended to call [_disableInitializers](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/utils/Initializable.sol#L145) within the contract's constructor to prevent the implementation from being initialized.

Remediation Plan

SOLVED : The CoreDAO team solved the issue by using disableInitializers.

Within the CcTransferRouterLogic contract, an "instant router" variable setter (setInstantRouter) is defined but appears to be redundant, as there is no subsequent utilization or reference to the "instant router" variable within the contract logic. This redundancy not only bloats the contract code but also introduces potential confusion regarding the contract's intended functionality and architecture.

The presence of unused variables and functions can obscure the contract's intended logic and make the codebase harder to maintain and understand.

Code Location : https://github.com/coredao-org/btcwrap-contracts/blob/main/contracts/routers/CcTransferRouterLogic.sol#L99

    /// @notice                             Setter for instant router
    /// @dev                                Only owner can call this
    /// @param _instantRouter               Address of the instant router contract
    function setInstantRouter(address _instantRouter) external override nonZeroAddress(_instantRouter) onlyOwner {
        _setInstantRouter(_instantRouter);
    }

Conduct a thorough review of the contract code to identify and remove unused variables, functions, and logic paths. Specifically, evaluate the necessity of the "instant router" variable and its associated setter function. If confirmed as redundant, remove the setInstantRouter function and any related code.

Remediation Plan

SOLVED : The CoreDAO team solved the issue by deleting redundant function/variable.

The BitcoinRelayLogic contract includes functionality to set the finalizationParameter, which determines the number of confirmations required for a transaction to be considered finalized. While a setter function exists for adjusting this parameter post-deployment, the initial value of the finalizationParameter is hardcoded to 3 during contract initialization. This design limits the contract's flexibility and adaptability at deployment, as it does not allow for the initial configuration of the finalization parameter.

Modify the initialize function to accept the finalizationParameter as an input argument. This adjustment will enable the contract deployer to set an appropriate initial value for the finalization parameter based on specific needs and conditions.

Remediation Plan

SOLVED : The CoreDAO team solved the issue by setting a parameter on the constructor.

Currently, the contracts do not adequately handle atypical ERC20 tokens on the staking. According to the ERC20 specification, tokens should return "false" when a transfer fails, but it does not guarantee that the function will revert. This discrepancy could potentially lead to silent failures, making it difficult to detect issues when they occur.

For instance, from the below, It can be seen that [LDO](https://etherscan.io/token/0x5a98fcbea516cf06857215779fd812ca3bef1b32#code) does not revert If the user balance is not enough for the transfer.

Note : Even if It does not pose risk for CoreBTC, It is recommended to use safeApprove & safeTransfer/safeTransferFrom.

Consider using safeApprove - safeTransfer - safeTransferFrom from Openzeppelin.

Remediation Plan

ACKNOWLEDGED : The CoreDAO team acknowledged the issue.

The BitcoinRelayLogic contract, which implements Ownable2StepUpgradeable among other functionalities, incorrectly uses OwnableUpgradeable.__Ownable_init() for initialization instead of the expected Ownable2StepUpgradeable.__Ownable2Step_init().

    /// @notice Gives a starting point for the relay
    /// @param  _height The starting height
    /// @param  _btcLightClient BTC light cient address
    function initialize(
        uint256 _height,
        address _btcLightClient
    ) public initializer {

        OwnableUpgradeable.__Ownable_init();
        ReentrancyGuardUpgradeable.__ReentrancyGuard_init();
        PausableUpgradeable.__Pausable_init();

        // Relay parameters
        btcLightClient = _btcLightClient;

        _setFinalizationParameter(3);
        initialHeight = _height;
    }

Update the initialize function within the BitcoinRelayLogic contract to use the correct initialization method for Ownable2StepUpgradeable. Replace OwnableUpgradeable.__Ownable_init() with Ownable2StepUpgradeable.__Ownable2Step_init() to ensure that all intended functionalities and security enhancements are correctly initialized and available.

Remediation Plan

SOLVED : The CoreDAO team solved the issue by calling Ownable2Step_init()function.