Bauer - The removeFromOffice() function is implemented incorrectly

[sherlock-admin]

Bauer

high

The removeFromOffice() function is implemented incorrectly

Summary

In the removeFromOffice() function,it does not clear the approval, allowing the approved user to retain permission for operations.

Vulnerability Detail

In the removeFromOffice() function, the protocol transfers the NFT from the from address to the to address.

 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);
    }

However, if this NFT was previously approved to another user, this function does not clear the approval, allowing the approved user to retain permission for operations.

 function approve(
        address to,
        uint256 tokenId
    )
        public
        override(ERC721Upgradeable, IERC721)
        onlyRole(GOVERNANCE_COUNCIL_ROLE)
    {
        _tokenApproval[tokenId] = to;
        emit Approval(ERC721Upgradeable.ownerOf(tokenId), to, tokenId);
    }

Impact

After the NFT has changed ownership, the person who was previously approved still retains permission to operate on it.

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

Clear previous approvals after transferring NFT ownership.

Duplicate of #35

[amshirif]

The desired affect is that approvals are maintained. This is because the addresses with allowances have a level of ownership that they get to maintain for their council seat

[sherlock-admin2]

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

takarez commented:

invalid because { i consider this invalid because the approve() fucntion has an only governance modifier and according to sherlock rules they are trusted entities; so the governance can make sure to revoke any approval that was made; and i can't see where its explicitly used in the function in question : removeFromOffice()}