howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/AccessControlHooks.sol#L220 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/FixedTermLoanHooks.sol#L254

Vulnerability details

Impact

The Documentation suggests that a role provider can be a "push" provider (one that "pushes" credentials into the hooks contract by calling grantRole) and a "pull" provider (one that the hook calls via getCredential or validateCredential).

The documentation also states that:

Role providers do not have to implement any of these functions - a role provider can be an EOA.

But in fact, only the initial deployer can be an EOA provider, since it is coded in the constructor. Any other EOA provider that the borrower tries to add via addRoleProvider will fail because it does not implement the interface.

Proof of Concept

PoC will revert because EOA does not implement interface obviously:

  [118781] AuditMarket::test_PoC_EOA_provider()
    ├─ [0] VM::startPrank(BORROWER1: [0xB193AC639A896a0B7a0B334a97f0095cD87427f2])
    │   └─ ← [Return]
    ├─ [29883] AccessControlHooks::addRoleProvider(RoleProvider: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 2592000 [2.592e6])
    │   ├─ [2275] RoleProvider::isPullProvider() [staticcall]
    │   │   └─ ← [Return] false
    │   ├─ emit RoleProviderAdded(providerAddress: RoleProvider: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], timeToLive: 2592000 [2.592e6], pullProviderIndex: 16777215 [1.677e7])
    │   └─ ← [Stop]
    ├─ [74243] AccessControlHooks::addRoleProvider(RoleProvider: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 2592000 [2.592e6])
    │   ├─ [2275] RoleProvider::isPullProvider() [staticcall]
    │   │   └─ ← [Return] true
    │   ├─ emit RoleProviderAdded(providerAddress: RoleProvider: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], timeToLive: 2592000 [2.592e6], pullProviderIndex: 0)
    │   └─ ← [Stop]
    ├─ [5547] AccessControlHooks::addRoleProvider(EOA_PROVIDER1: [0x6aAfF89c996cAa2BD28408f735Ba7A441276B03F], 2592000 [2.592e6])
    │   ├─ [0] EOA_PROVIDER1::isPullProvider() [staticcall]
    │   │   └─ ← [Stop]
    │   └─ ← [Revert] EvmError: Revert
    └─ ← [Revert] EvmError: Revert

PoC:

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

import "forge-std/Test.sol";
import "forge-std/console2.sol";

import {WildcatArchController} from "../src/WildcatArchController.sol";
import {HooksFactory} from "../src/HooksFactory.sol";
import {LibStoredInitCode} from "src/libraries/LibStoredInitCode.sol";
import {WildcatMarket} from "src/market/WildcatMarket.sol";
import {AccessControlHooks} from "../src/access/AccessControlHooks.sol";
import {DeployMarketInputs} from "../src/interfaces/WildcatStructsAndEnums.sol";
import {HooksConfig, encodeHooksConfig} from "../src/types/HooksConfig.sol";

import {MockERC20} from "../test/shared/mocks/MockERC20.sol";
import {MockSanctionsSentinel} from "./shared/mocks/MockSanctionsSentinel.sol";
import {deployMockChainalysis} from "./shared/mocks/MockChainalysis.sol";
import {IRoleProvider} from "../src/access/IRoleProvider.sol";

contract AuditMarket is Test {
    WildcatArchController wildcatArchController;
    MockSanctionsSentinel internal sanctionsSentinel;
    HooksFactory hooksFactory;

    MockERC20 ERC0 = new MockERC20();

    address immutable ARCH_DEPLOYER = makeAddr("ARCH_DEPLOYER");
    address immutable FEE_RECIPIENT = makeAddr("FEE_RECIPIENT");
    address immutable BORROWER1 = makeAddr("BORROWER1");

    address immutable EOA_PROVIDER1 = makeAddr("EOA_PROVIDER1");
    address immutable PROVIDER1 = address(new RoleProvider(false));
    address immutable PROVIDER2 = address(new RoleProvider(true));

    address accessControlHooksTemplate = LibStoredInitCode.deployInitCode(type(AccessControlHooks).creationCode);

    AccessControlHooks accessControlHooksInstance;

    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 {
        deployMockChainalysis();
        vm.startPrank(ARCH_DEPLOYER);
        wildcatArchController = new WildcatArchController();
        sanctionsSentinel = new MockSanctionsSentinel(address(wildcatArchController));
        (address initCodeStorage, uint256 initCodeHash) = _storeMarketInitCode();
        hooksFactory =
            new HooksFactory(address(wildcatArchController), address(sanctionsSentinel), initCodeStorage, initCodeHash);

        wildcatArchController.registerControllerFactory(address(hooksFactory));
        hooksFactory.registerWithArchController();
        wildcatArchController.registerBorrower(BORROWER1);

        hooksFactory.addHooksTemplate(
            accessControlHooksTemplate, "accessControlHooksTemplate", FEE_RECIPIENT, address(ERC0), 1 ether, 500
        );
        vm.startPrank(BORROWER1);

        DeployMarketInputs memory marketInput = DeployMarketInputs({
            asset: address(ERC0),
            namePrefix: "Test",
            symbolPrefix: "TT",
            maxTotalSupply: uint128(100_000e27),
            annualInterestBips: uint16(500),
            delinquencyFeeBips: uint16(500),
            withdrawalBatchDuration: uint32(5 days),
            reserveRatioBips: uint16(500),
            delinquencyGracePeriod: uint32(5 days),
            hooks: encodeHooksConfig(address(0), true, true, false, true, false, false, false, false, false, true, false)
        });
        bytes memory hooksData = abi.encode(uint32(block.timestamp + 30 days), uint128(1e27));
        deal(address(ERC0), BORROWER1, 1 ether);
        ERC0.approve(address(hooksFactory), 1 ether);
        (address market, address hooksInstance) = hooksFactory.deployMarketAndHooks(
            accessControlHooksTemplate,
            abi.encode(BORROWER1),
            marketInput,
            hooksData,
            bytes32(bytes20(BORROWER1)),
            address(ERC0),
            1 ether
        );
        accessControlHooksInstance = AccessControlHooks(hooksInstance);
        vm.stopPrank();
    }

    function test_PoC_EOA_provider() public {
        vm.startPrank(BORROWER1);

        accessControlHooksInstance.addRoleProvider(PROVIDER1, uint32(30 days));
        accessControlHooksInstance.addRoleProvider(PROVIDER2, uint32(30 days));
        accessControlHooksInstance.addRoleProvider(EOA_PROVIDER1, uint32(30 days));
    }
}

contract RoleProvider is IRoleProvider {
    bool public isPullProvider;
    mapping(address account => uint32 timestamp) public getCredential;

    constructor(bool _isPullProvider) {
        isPullProvider = _isPullProvider;
    }

    function setCred(address account, uint32 timestamp) external {
        getCredential[account] = timestamp;
    }

    function validateCredential(address account, bytes calldata data) external returns (uint32 timestamp) {
        if (data.length != 0) {
            return uint32(block.timestamp);
        } else {
            revert("Wrong creds");
        }
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Replace the interface call with a low-level call and check if the user implements the interface in order to be a pull provider:

(bool succes, bytes memory data) =
    providerAddress.call(abi.encodeWithSelector(IRoleProvider.isPullProvider.selector));
bool isPullProvider;
if (succes && data.length == 0x20) {
    isPullProvider = abi.decode(data, (bool));
} else {
    isPullProvider = false;
}

With this code all logic works as expected, for EOA providers pullProviderIndex is set to type(uint24).max, for contracts - depending on the result of calling isPullProvider:

Traces:
  [141487] AuditMarket::test_PoC_EOA_provider()
    ├─ [0] VM::startPrank(BORROWER1: [0xB193AC639A896a0B7a0B334a97f0095cD87427f2])
    │   └─ ← [Return]
    ├─ [30181] AccessControlHooks::addRoleProvider(RoleProvider: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 2592000 [2.592e6])
    │   ├─ [2275] RoleProvider::isPullProvider()
    │   │   └─ ← [Return] false
    │   ├─ emit RoleProviderAdded(providerAddress: RoleProvider: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], timeToLive: 2592000 [2.592e6], pullProviderIndex: 16777215 [1.677e7])
    │   └─ ← [Stop]
    ├─ [74541] AccessControlHooks::addRoleProvider(RoleProvider: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 2592000 [2.592e6])
    │   ├─ [2275] RoleProvider::isPullProvider()
    │   │   └─ ← [Return] true
    │   ├─ emit RoleProviderAdded(providerAddress: RoleProvider: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], timeToLive: 2592000 [2.592e6], pullProviderIndex: 0)
    │   └─ ← [Stop]
    ├─ [27653] AccessControlHooks::addRoleProvider(EOA_PROVIDER1: [0x6aAfF89c996cAa2BD28408f735Ba7A441276B03F], 2592000 [2.592e6])
    │   ├─ [0] EOA_PROVIDER1::isPullProvider()
    │   │   └─ ← [Stop]
    │   ├─ emit RoleProviderAdded(providerAddress: EOA_PROVIDER1: [0x6aAfF89c996cAa2BD28408f735Ba7A441276B03F], timeToLive: 2592000 [2.592e6], pullProviderIndex: 16777215 [1.677e7])
    │   └─ ← [Stop]
    └─ ← [Stop]

Assessed type

Invalid Validation

laurenceday commented 1 month ago

Not really a medium in that it doesn't 'matter' for the most part: this is sort of a documentation issue in that we'd never really expect an EOA that wasn't the borrower (which is an EOA provider) to be a role provider.

It's vanishingly unlikely that a borrower is going to add some random arbiter that they don't control - possible that they add another address that THEY control but in that case they might as well use the one that's known to us.

Disputing, but with a light touch: we consider this a useful QA.

3docSec commented 1 month ago

Thanks for the context. If we ignore the documentation, the fact that the initial role provider is the borrower, and the NotPullProviderIndex value that is used in this case makes it clear that the intention is allowing for EOAs to be there.

While not the most requested feature, it's something a borrower may want to do, and given the above, may reasonably expect to see working. For this reason, I think a Med is reasonable because we have marginal I admit, but still tangible, availability impact

c4-judge commented 1 month ago

3docSec marked the issue as satisfactory