operator customizes the slashStore address, it results in slashAssets always reverting

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L75 https://github.com/code-423n4/2024-07-karak/blob/main/src/entities/CoreLib.sol#L89-L116

Vulnerability details

Impact

operator can customizes the slashStore address when create new vault, it results in slashAssets always reverting , if the slashStore address is different with slashingHandler.

Proof of Concept

DSSs have the right to slash vaults staked to it if it feels that an operator had failed to perform its tasks adequately. From the NativeVault.sol::slashAssets

function slashAssets(uint256 totalAssetsToSlash, address slashingHandler)
    external
    onlyOwner
    nonReentrant
    whenFunctionNotPaused(Constants.PAUSE_NATIVEVAULT_SLASHER)
    returns (uint256 transferAmount)
{
    NativeVaultLib.Storage storage self = _state();

    if (slashingHandler != self.slashStore) revert NotSlashStore();   <@

there is a slashingHandler != self.slashStore check.

The slashingHandler is comes from assetSlashingHandlers setting. It's a mapping data structure, where each asset corresponds to a specific slashingHandler address. MANAGER_ROLE in core.sol can set above mapping via Core.sol::allowlistAssets

function allowlistAssets(address[] memory assets, address[] memory slashingHandlers)
    external
    onlyRolesOrOwner(Constants.MANAGER_ROLE)
{
    _self().allowlistAssets(assets, slashingHandlers);
    emit AllowlistedAssets(assets);
}

While the slashStore is an operator custom address pass in via extraData when creating new vault CoreLib.sol::createVault

function createVault(
    Storage storage self,
    address operator,
    address depositToken,
    string memory name,
    string memory symbol,
    bytes memory extraData,
    address implementation
) internal returns (IKarakBaseVault) {
    // Use Create2 to determine the address before hand
    bytes32 salt = keccak256(abi.encodePacked(operator, depositToken, self.vaultNonce++));

    address expectedNewVaultAddr =
        LibClone.predictDeterministicAddressERC1967BeaconProxy(address(this), salt, address(this));

    self.vaultToImplMap[address(expectedNewVaultAddr)] = implementation;

    IKarakBaseVault vault = cloneVault(salt);
    vault.initialize(address(this), operator, depositToken, name, symbol, extraData);  <@

NativeVault.sol::initialize

(address manager, address slashStore, address nodeImplementation) =
    abi.decode(_extraData, (address, address, address));    <@

if (manager == address(0) || slashStore == address(0) || nodeImplementation == address(0)) revert ZeroAddress();
_grantRoles(manager, Constants.MANAGER_ROLE);

NativeVaultLib.Storage storage self = _state();
VaultLib.Config storage config = _config();

config.asset = _depositToken;
config.name = _name;
config.symbol = _symbol;
config.decimals = 18;
config.operator = _operator;
config.extraData = _extraData;

self.slashStore = slashStore;     <@
self.nodeImpl = nodeImplementation;

If those two address is not the same slashAssets will always revert.

test:

function testOperatorCreateCustomAvoidSlashRequest() public {
    // Setup NativeNode implementation
    address nativeNodeImpl = address(new NativeNode());

    //operator can specific a slashStore address.
    address slashStoreRandom = makeAddr('randomAddress');

    // Deploy Vaults
    VaultLib.Config[] memory vaultConfigs = new VaultLib.Config[](1);
    vaultConfigs[0] = VaultLib.Config({
        asset: Constants.DEAD_BEEF,
        decimals: 18,
        operator: operator,
        name: "NativeTestVault",
        symbol: "NTV",
        extraData: abi.encode(address(manager), slashStoreRandom, address(nativeNodeImpl))
    });

    vm.prank(operator);
    IKarakBaseVault[] memory vaults = core.deployVaults(vaultConfigs, address(0));

    nativeVault = NativeVault(address(vaults[0]));

    //transaction revert due to slashstore address not equal.
    vm.expectRevert(NotSlashStore.selector);
    //use owner(core) slash assets.
    vm.prank(address(core));
    nativeVault.slashAssets(1,slashStore);
}

out:

[PASS] testOperatorCreateCustomAvoidSlashRequest() (gas: 1748139)

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.10ms (1.89ms CPU time)

Ran 1 test suite in 150.85ms (13.10ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry

Recommended Mitigation Steps

@@ -305,8 +305,6 @@ contract NativeVault is ERC4626, IBeacon, Pauser, INativeVault, OwnableRoles, Re
     {
         NativeVaultLib.Storage storage self = _state();

-        if (slashingHandler != self.slashStore) revert NotSlashStore();
-
         // avoid negative totalAssets if slashing amount is greater than totalAssets
         if (totalAssetsToSlash > self.totalAssets) {
             totalAssetsToSlash = self.totalAssets;

Assessed type

Invalid Validation

[c4-judge]

MiloTruck marked the issue as satisfactory

[coffiasd]

@MiloTruck I think this issue is dup with 49 instead of 55

cuz above submission pointed out that there is a slashingHandler != self.slashStore check when it's not equal , the slash always revert.

[MiloTruck]

This is a duplicate of 55. https://github.com/code-423n4/2024-07-karak-findings/issues/49 is about the slashing handler being changed.