Operator can bypass MIN_STAKE_UPDATE_DELAY by spamming requestUpdateVaultStakeInDSS()

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/entities/Operator.sol#L61-L77

Vulnerability details

Impact

The MIN_STAKE_UPDATE_DELAY will not work as intended.

Proof of Concept

When staking a vault to DSS, the operator can call requestUpdateVaultStakeInDSS(), wait for Constants.MIN_STAKE_UPDATE_DELAY, then finalize the request through validateAndUpdateVaultStakeInDSS(). The same goes for unstaking a vault.

The operator can bypass the Constants.MIN_STAKE_UPDATE_DELAY by repeatedly calling requestUpdateVaultStakeInDSS() with both staking and unstaking requests. This way, the operator can skip the delay check since adding and removing into the Enumerable Set doesn't check the boolean values,

    function updateVaultStakeInDSS(State storage operatorState, address vault, IDSS dss, bool toStake) internal {
        if (toStake) {
>           operatorState.vaultStakedInDssMap[dss].add(vault);
        } else {
>           operatorState.vaultStakedInDssMap[dss].remove(vault);
        }
    }

In OpenZeppelin's EnumerableSet, the function add and remove returns a boolean value:

    function add(Bytes32Set storage set, bytes32 value) internal returns (bool) {
        return _add(set._inner, value);
    }

    function remove(Bytes32Set storage set, bytes32 value) internal returns (bool) {
        return _remove(set._inner, value);
    }

Without checking the boolean value, the operator can simply call remove on a vault before adding the vault.

Medium severity as anyone can finalize the request, so the operator has to submit multiple unstake and stake request in order to game the system.

Tools Used

Manual Review

Recommended Mitigation Steps

A bandaid mitigation is to check that the vault is in the Enumerable Set so that it can be removed. This way, the operator cannot call unstake before calling stake and vice versa.

    function updateVaultStakeInDSS(State storage operatorState, address vault, IDSS dss, bool toStake) internal {
        if (toStake) {
>           require(operatorState.vaultStakedInDssMap[dss].add(vault));
        } else {
>           require(operatorState.vaultStakedInDssMap[dss].remove(vault));
        }
    }

Assessed type

Context

[c4-judge]

MiloTruck marked the issue as unsatisfactory: Out of scope

[c4-judge]

MiloTruck marked the issue as not a duplicate

[c4-judge]

MiloTruck removed the grade

[c4-judge]

MiloTruck marked the issue as duplicate of #61

[c4-judge]

MiloTruck changed the severity to QA (Quality Assurance)

[MiloTruck]

MIN_STAKE_UPDATE_DELAY is not bypassed here.

There can only be one pending update request for a vault at a time, so the operatorState.pendingStakeUpdates[stakeUpdate.vault] != bytes32(0) will revert:

    function validateStakeUpdateRequest(State storage operatorState, StakeUpdateRequest memory stakeUpdate)
        internal
        view
    {
        if (operatorState.pendingStakeUpdates[stakeUpdate.vault] != bytes32(0)) revert PendingStakeUpdateRequest();
        if (!operatorState.vaults.contains(stakeUpdate.vault)) revert VaultNotAChildVault();
    }

[c4-judge]

MiloTruck marked the issue as grade-b