When the `returnNative` parameter is set to true in the `_params` provided to `MoneyMarketHook.execute`, it is not handled properly and could disrupt user expectations

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/hook/MoneyMarketHook.sol#L168-L196 https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/hook/MoneyMarketHook.sol#L76-L80

Vulnerability details

Impact

When param.returnNative is set to true while calling MoneyMarketHook.execute, users expect the returned token from the withdraw operation to be in native form and sent to the caller. However, in the current implementation, this is not considered and could disrupt user expectations.

Proof of Concept

The withdraw functionality inside MoneyMarketHook will process the WithdrawParams provided by users and construct the operations using _handleWithdraw, which consist of calling decollateralize and burnTo in InitCore, providing the parameters accordingly.

https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/hook/MoneyMarketHook.sol#L168-L196

    function _handleWithdraw(uint _offset, bytes[] memory _data, uint _initPosId, WithdrawParams[] calldata _params)
        internal
        view
        returns (uint, bytes[] memory)
    {
        for (uint i; i < _params.length; i = i.uinc()) {
            // decollateralize to pool
            _data[_offset] = abi.encodeWithSelector(
                IInitCore.decollateralize.selector, _initPosId, _params[i].pool, _params[i].shares, _params[i].pool
            );
            _offset = _offset.uinc();
            // burn collateral to underlying token
            address helper = _params[i].rebaseHelperParams.helper;
            address uTokenReceiver = _params[i].to;
            // if need to unwrap to rebase token
            if (helper != address(0)) {
                address uToken = ILendingPool(_params[i].pool).underlyingToken();
                _require(
                    _params[i].rebaseHelperParams.tokenIn == uToken
                        && IRebaseHelper(_params[i].rebaseHelperParams.helper).YIELD_BEARING_TOKEN() == uToken,
                    Errors.INVALID_TOKEN_IN
                );
                uTokenReceiver = helper;
            }
            _data[_offset] = abi.encodeWithSelector(IInitCore.burnTo.selector, _params[i].pool, uTokenReceiver);
            _offset = _offset.uinc();
        }
        return (_offset, _data);
    }

As it can be observed, _handleWithdraw doesn't check param.returnNative and not adjust the uTokenReceiver token receiver to address(this) when param.returnNative is set to true.

Now, when execute finish perform the multicall and check that _params.returnNative is set to true, it will not work properly as the token is not send to the Hook.

https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/hook/MoneyMarketHook.sol#L76-L80

    function execute(OperationParams calldata _params)
        external
        payable
        returns (uint posId, uint initPosId, bytes[] memory results)
    {
        // create position if not exist
        if (_params.posId == 0) {
            (posId, initPosId) = createPos(_params.mode, _params.viewer);
        } else {
            // for existing position, only owner can execute
            posId = _params.posId;
            initPosId = initPosIds[msg.sender][posId];
            _require(IERC721(POS_MANAGER).ownerOf(initPosId) == address(this), Errors.NOT_OWNER);
        }
        results = _handleMulticall(initPosId, _params);
        // check slippage
        _require(_params.minHealth_e18 <= IInitCore(CORE).getPosHealthCurrent_e18(initPosId), Errors.SLIPPAGE_CONTROL);
        // unwrap token if needed
        for (uint i; i < _params.withdrawParams.length; i = i.uinc()) {
            address helper = _params.withdrawParams[i].rebaseHelperParams.helper;
            if (helper != address(0)) IRebaseHelper(helper).unwrap(_params.withdrawParams[i].to);
        }
        // return native token
        if (_params.returnNative) {
>>>         IWNative(WNATIVE).withdraw(IERC20(WNATIVE).balanceOf(address(this)));
>>>         (bool success,) = payable(msg.sender).call{value: address(this).balance}('');
            _require(success, Errors.CALL_FAILED);
        }
    }

This could disrupt user expectations. Consider a third-party contract integrated with this hook that can only operate using native balance, doesn't expect, and cannot handle tokens/ERC20. This can cause issues for the integrator.

Tools Used

Manual review.

Recommended Mitigation Steps

When constructing withdraw operation, check if _params.returnNative is set to true, and change uTokenReceiver to address(this.

-    function _handleWithdraw(uint _offset, bytes[] memory _data, uint _initPosId, WithdrawParams[] calldata _params)
+    function _handleWithdraw(uint _offset, bytes[] memory _data, uint _initPosId, WithdrawParams[] calldata _params, bool _returnNative)
        internal
        view
        returns (uint, bytes[] memory)
    {
        for (uint i; i < _params.length; i = i.uinc()) {
            // decollateralize to pool
            _data[_offset] = abi.encodeWithSelector(
                IInitCore.decollateralize.selector, _initPosId, _params[i].pool, _params[i].shares, _params[i].pool
            );
            _offset = _offset.uinc();
            // burn collateral to underlying token
            address helper = _params[i].rebaseHelperParams.helper;
            address uTokenReceiver = _params[i].to;
            // if need to unwrap to rebase token
            if (helper != address(0)) {
                address uToken = ILendingPool(_params[i].pool).underlyingToken();
                _require(
                    _params[i].rebaseHelperParams.tokenIn == uToken
                        && IRebaseHelper(_params[i].rebaseHelperParams.helper).YIELD_BEARING_TOKEN() == uToken,
                    Errors.INVALID_TOKEN_IN
                );
                uTokenReceiver = helper;
            }
+            if (_returnNative && uToken == WNATIVE) {
+                uTokenReceiver = address(this);
+            }
            _data[_offset] = abi.encodeWithSelector(IInitCore.burnTo.selector, _params[i].pool, uTokenReceiver);
            _offset = _offset.uinc();
        }
        return (_offset, _data);
    }

    function _handleMulticall(uint _initPosId, OperationParams calldata _params)
        internal
        returns (bytes[] memory results)
    {
        // prepare data for multicall
        // 1. repay (if needed)
        // 2. withdraw (if needed)
        // 3. change position mode (if needed)
        // 4. borrow (if needed)
        // 5. deposit (if needed)
        bool changeMode = _params.mode != 0 && _params.mode != IPosManager(POS_MANAGER).getPosMode(_initPosId);
        bytes[] memory data;
        {
            uint dataLength = _params.repayParams.length + (2 * _params.withdrawParams.length) + (changeMode ? 1 : 0)
                + _params.borrowParams.length + (2 * _params.depositParams.length);
            data = new bytes[](dataLength);
        }
        uint offset;
        // 1. repay
        (offset, data) = _handleRepay(offset, data, _initPosId, _params.repayParams);
        // 2. withdraw
-        (offset, data) = _handleWithdraw(offset, data, _initPosId, _params.withdrawParams);
+        (offset, data) = _handleWithdraw(offset, data, _initPosId, _params.withdrawParams, _params.returnNative);
        // 3. change position mode
        if (changeMode) {
            data[offset] = abi.encodeWithSelector(IInitCore.setPosMode.selector, _initPosId, _params.mode);
            offset = offset.uinc();
        }
        // 4. borrow
        (offset, data) = _handleBorrow(offset, data, _initPosId, _params.borrowParams);
        // 5. deposit
        (offset, data) = _handleDeposit(offset, data, _initPosId, _params.depositParams);
        // execute multicall
        results = IMulticall(CORE).multicall(data);
    }

Assessed type

Context

[JeffCX]

duplicate of #2

[c4-judge]

hansfriese marked the issue as primary issue

[c4-sponsor]

fez-init (sponsor) confirmed

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as selected for report