code423n4 commented 2 years ago

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/CommunityPool.sol#L199

Vulnerability details

Impact

The setMinTransactionGas function does not have any upper or lower bounds. Values that are too large will lead to reversions in several critical functions. User will never have enough gas if the gas is set to so high.

Proof of Concept

Navigate to the following contract.

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/CommunityPool.sol#L199

    function setMinTransactionGas(uint newMinTransactionGas) external override {
        require(hasRole(CONSTANT_SETTER_ROLE, msg.sender), "CONSTANT_SETTER_ROLE is required");
        emit MinTransactionGasWasChanged(minTransactionGas, newMinTransactionGas);
        minTransactionGas = newMinTransactionGas;
    }

Tools Used

Code Review

Recommended Mitigation Steps

Consider to define upper and lower bounds on the minTransactionGas.

DimaStebaev commented 2 years ago

This parameter is adjusted by foundation and reflects upper bound of how many gas a node should spend to process single message. This value is not tend to be changed and is connected to min ETH amount requirement for a node in SKALE network and Ethereum block gas limit. If a new value is set it is carefully reviewed and voted by the SKALE foundation.

GalloDaSballo commented 2 years ago

I believe that there is an argument to be made for admin privilege here, that said the warden did not provide much detail as to how the privilege can be abused, the mechanics of it and there's no mention of the impact.

From what I can tell the setting will mostly deny gas refunds, for that reason I believe Low Severity to be more appropriate.

In the future I'd ask the warden to specify a detailed impact so that their submission can withstand basic scrutiny

code-423n4 / 2022-02-skale-findings