grearlake - Malicious council member can't be removed

[sherlock-admin2]

grearlake

high

Malicious council member can't be removed

Summary

Malicious council member can't be removed from council member list by transfer ERC721 token to other address

Vulnerability Detail

Function removeFromOffice is called by governance to replace existing council member with a new one and withdraw old telecoin:

function removeFromOffice(
    address from,
    address to,
    uint256 tokenId,
    address rewardRecipient
) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
    // Retrieve and distribute any pending TELCOIN for all council members
    _retrieve();
    // Withdraw all the TELCOIN rewards for the specified token to the rewardRecipient
    _withdrawAll(rewardRecipient, tokenId);
    // Transfer the token (representing the council membership) from one address to another
    _transfer(from, to, tokenId);    // <---
}

it call _transfer() function in ERC721Upgradeable library to transfer ownership of token:

function _transfer(address from, address to, uint256 tokenId) internal {
    if (to == address(0)) {
        revert ERC721InvalidReceiver(address(0));
    }
    address previousOwner = _update(to, tokenId, address(0));
    if (previousOwner == address(0)) {
        revert ERC721NonexistentToken(tokenId);
    } else if (previousOwner != from) {
        revert ERC721IncorrectOwner(from, tokenId, previousOwner);
    }
}

But transferFrom() function in the ERC721Upgradeable is not overriden, which make user able to transfer token to other address, which make condition else if (previousOwner != from) is true, lead to contract become revert. transferFrom() function:

function transferFrom(address from, address to, uint256 tokenId) public virtual {
    if (to == address(0)) {
        revert ERC721InvalidReceiver(address(0));
    }
    // Setting an "auth" arguments enables the `_isAuthorized` check which verifies that the token exists
    // (from != 0). Therefore, it is not needed to verify that the return value is not 0 here.
    address previousOwner = _update(to, tokenId, _msgSender());
    if (previousOwner != from) {
        revert ERC721IncorrectOwner(from, tokenId, previousOwner);
    }
}

Council member can transfer token because they are owner of token that granted in the `mint() function:

function mint(
    address newMember
) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
    if (totalSupply() != 0) {
        _retrieve();
    }

    balances.push(0);
    _mint(newMember, totalSupply());
}

Impact

Council member can't be removed. Attacker can execute bad actions like cancel any transaction by challenging them.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L122-#L134

Tool used

Manual Review

Recommendation

transferFrom() function in the ERC721Upgradeable contract should be overriden to not allowing anyone to call them

Duplicate of #243

[sherlock-admin2]

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

takarez commented:

valid because { this is a valid finding as the watson was able to explain howa malicious user can transfer the token and denying the governace from removing him; its a dupp of 190 with better writeup and a highrt impact reported thus making it best!!!}