c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/registries/ExecutorRegistry.sol#L75

Vulnerability details

Impact

The values() function when called within the getExecutorsForSubAccount function can contribute to the gas shortage.

The values function is part of the EnumerableSet.sol library from OpenZeppelin. It retrieves all the values in the set and returns them in an array. The values function in involves iterating over the set and populates an array from storage, which requires higher computational operation as indicated in the contract and thus can lead to out of gas error.

Proof of Concept

The values() function is view but it is inherited in the ExecutorRegistry.sol contract and called internally by getExecutorsForSubAccount which will cost gas.

/**
     * @dev Return the entire set in an array
     *
     * WARNING: This operation will copy the entire storage to memory, which can be quite expensive. This is designed
     * to mostly be used by view accessors that are queried without any gas fees. Developers should keep in mind that
     * this function has an unbounded cost, and using it as part of a state-changing function may render the function
     * uncallable if the set grows to a point where copying to memory consumes too much gas to fit in a block.
     */
    function values(Bytes32Set storage set) internal view returns (bytes32[] memory) {
        bytes32[] memory store = _values(set._inner);
        bytes32[] memory result;

        /// @solidity memory-safe-assembly
        assembly {
            result := store
        }

        return result;
    }

    // AddressSet

    struct AddressSet {
        Set _inner;
    }

OpenZeppelin clearly warn about using of values function of EnumerableSet.sol contract as it can lead to shortage of gas.

Tools Used

Manual Review

Recommended Mitigation Steps

The first recommendation is made in area of limiting number of iteration in the function to avoid out of gas error. The second is made in area of modifying the set params pointer for values function from storage to calldata so that it is not stored and just called when function executes.

- function values(Bytes32Set storage set) internal view returns (bytes32[] memory) {
+ function values(Bytes32Set calldata set) internal view returns (bytes32[] memory) {

- bytes32[] memory store = _values(set._inner);
+ bytes32[] calldata store = _values(set._inner);

- bytes32[] memory result;
+ bytes32[] calldata result;

Assessed type

Other

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #3

c4-judge commented 1 year ago

alex-ppg marked the issue as not a duplicate

alex-ppg commented 1 year ago

The Warden fails to demonstrate how the relevant call would fail to execute as expected or how gas is of a concern.