Ability to transfer burned tokens, and later blocking TimeLock contract

Summary

There is an ability to transfer burned tokens.

Vulnerability Detail

The main problem is with the _isAuthorized function, not enough validation.

Firstly, I will introduce the functions, and in the second part I will explain how the exploit can be performed

CouncilMember.sol has a custom approve function, which adds an approved spender to the _tokenApproval mapping.

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

Then custom _isAuthorized function will be called if an approved spender would like to "spend" a token by calling transferFrom from OZ library

function _isAuthorized(
        address,
        address spender,
        uint256 tokenId
    ) internal view override returns (bool) {
        return (hasRole(GOVERNANCE_COUNCIL_ROLE, spender) ||
            _tokenApproval[tokenId] == spender);
    }

Let's discuss how it can be used to perform an exploit.

Alice - ERC721 token original holder Bob - Ailce's friend

Step 1 Alice adds Bob to the _tokenApproval mapping, to allow Bob to transfer Alice's token. By calling approve function

Step 2 Alice to go GOVERNANCE_COUNCIL_ROLE role holder and asks to burn her token. Then GOVERNANCE_COUNCIL_ROLE role holder calls burn function to withdraw all TELCOINs related to the ERC721 token and send an ERC721 token to 0 address

Step 3 Since Bob is approved to spend token with tokenId (which was Alice's token in step1), Bob can call the transferFrom function and transfer just the burned token to himself from the 0 address.

Because transferFrom has no check the from address is no 0 address. and the only check it has is to check that _isAuthorized function returns True. Which will return True in this case, because approve is attached to tokenId not to token holder address.

Impact

The token that Bob transferred from 0 address, will not receive Telcoins during token distribution.

But the main impact is that there are no functions for admins to take this approval from Bob on that ERC721 token. This will allow Bob to pass the onlyCouncilMember check in the executeTransaction.sol contract to call functions such as executeTransaction or challengeTransaction. The challengeTransaction can change proposedTransactions[transactionId].challenged = true;

So no one will be able to proposeTransaction and executeTransaction, because during the challengePeriod Bob can call challengeTransaction and "reject" the proposeTransaction

Code Snippet

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

Tool used

Manual Review

Recommendation

But if admins would like to change the challengePeriod to 0, and proposeTransaction and executeTransaction in one block, mev bot could plug their proposeTransaction and execute it before admins, to drain the tokens

Clean the _tokenApproval related to tokenId on every owner change of an ERC721 token. OR add more check in _isAuthorized functoin

Duplicate of #35

sherlock-audit / 2024-01-telcoin-judging

Strausses - Ability to transfer burned tokens, and later blocking TimeLock contract #171