sherlock-audit / 2023-09-Gitcoin-judging

11 stars 7 forks source link

aycozynfada - Admin can assign manager roles to invalid addresses during pool creation #927

Closed sherlock-admin2 closed 11 months ago

sherlock-admin2 commented 12 months ago

aycozynfada

high

Admin can assign manager roles to invalid addresses during pool creation

Admin can mistakenly assign manager roles to an extruder who is not a member of a profile

Vulnerability Detail

During pool creation, the manager addresses passed into the _createPool function are not crosschecked to asses if they are a member of a profile before assigning manager roles to them. If an admin whether intentionally, mistakenly or randomly adds wrong addresses as manager roles. Although "registry.isOwnerOrMemberOfProfile" was called initially during pool creation to confirm profile owner. It was is not used to assess if addresses are valid and part of a profile. Thereby allowing extruder addresses to gain managerial roles and manipulate contract by causing denial of service attacks to recipients and causing managerial errors detrimental to normal functioning of the contract. https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/core/Allo.sol#L415-L467

Impact

HIGH

Code Snippet

` function _createPool( bytes32 _profileId, IStrategy _strategy, bytes memory _initStrategyData, address _token, uint256 _amount, Metadata memory _metadata, address[] memory _managers ) internal returns (uint256 poolId) { if (!registry.isOwnerOrMemberOfProfile(_profileId, msg.sender)) revert UNAUTHORIZED();

    poolId = ++_poolIndex;

    // Generate the manager & admin roles for the pool (this is the way we do this throughout the protocol for consistency)
    bytes32 POOL_MANAGER_ROLE = bytes32(poolId);
    bytes32 POOL_ADMIN_ROLE = keccak256(abi.encodePacked(poolId, "admin"));`

` // grant pool managers roles uint256 managersLength = _managers.length; for (uint256 i; i < managersLength;) { address manager = _managers[i]; if (manager == address(0)) revert ZERO_ADDRESS();

        _grantRole(POOL_MANAGER_ROLE, manager);
        unchecked {
            ++i;
        }
    }

`

Tool used

Manual Review

Recommendation

the isOwnerOrMemberOfProfile function from the registry contract should be utilized again during pool creation to ascertain if manager addresses are valid addresses.

sherlock-admin commented 11 months ago

1 comment(s) were left on this issue during the judging contest.

n33k commented:

invalid, user mistake