sherlock-audit / 2023-09-Gitcoin-judging

11 stars 7 forks source link

0xnirlin - When registery ownership is transferred, previous owner can add him as member before the ownership transfer #933

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

0xnirlin

medium

When registery ownership is transferred, previous owner can add him as member before the ownership transfer

Previous owner can have escalated privileges if he adds himself as member right before the transfer ownership and can have escalated privileges in the system.

Vulnerability Detail

Owner can add members to the register by calling the addMembers() function.

But owner can also be changed, owner may not appear malicous but may escalte the privilieges if right before ownership if transferred he also adds himself as member and members have alot of privileges in the system, which he can than exploit if went rogue.

  function addMembers(bytes32 _profileId, address[] memory _members) external onlyProfileOwner(_profileId) {
        uint256 memberLength = _members.length;

        // Loop through the members and add them to the profile by granting the role
        for (uint256 i; i < memberLength;) {
            address member = _members[i];

            // Will revert if any of the addresses are a zero address
            if (member == address(0)) revert ZERO_ADDRESS();

            // Grant the role to the member and emit the event for each member
            _grantRole(_profileId, member);
            unchecked {
                ++i;
            }
        }
    }

Impact

Previous owner can add him as member and can have escalated privileges even when he is removed from the system as owner.

Code Snippet

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/core/Registry.sol#L284-L300

Tool used

Manual Review

Recommendation

Delete the owner from members too if he is added there, if want to add him as member new owner can add him later.

sherlock-admin commented 1 year ago

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

n33k commented:

low