m4ttm - Potentially incorrect access control in _isAuthorized

[sherlock-admin]

m4ttm

medium

Potentially incorrect access control in _isAuthorized

Summary

The @notice states different access requirements to the @dev and code implementation.

Vulnerability Detail

The @notice comment states that the function determines if an address is approved or the owner, whereas the @dev comment and the code implementation state the address should have GOVERNANCE_COUNCIL_ROLE or be the approved address. Since this function is not used, it is assumed this will be used in a future upgraded contract implemation. It is unclear what is purpose is or what the desired access requirements are.

Impact

Potentially incorrect access control.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/0954297f4fefac82d45a79c73f3a4b8eb25f10e9/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L297-L311

/**
     * @notice Determines if an address is approved or is the owner for a specific token ID
     * @dev This function checks if the spender has GOVERNANCE_COUNCIL_ROLE or is the approved address for the token.
     * @param spender Address to check approval or ownership for.
     * @param tokenId Token ID to check against.
     * @return True if the address is approved or is the owner, false otherwise.
     */
    function _isAuthorized(
        address,
        address spender,
        uint256 tokenId
    ) internal view override returns (bool) {
        return (hasRole(GOVERNANCE_COUNCIL_ROLE, spender) ||
            _tokenApproval[tokenId] == spender);
    }

Tool used

Manual Review

Recommendation

Clarify what the desired access requirements are for this function and update the comments/code accordingly.

Duplicate of #206

[sherlock-admin2]

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

takarez commented:

invalid because {this is invalid; no clear explanation of whats wrong}