search

sherlock-audit / 2024-01-telcoin-judging

6 stars 5 forks source link

bitsurfer - Lack of revoke or clear token approval when removed from office #208

Closed sherlock-admin closed 5 months ago

sherlock-admin commented 5 months ago

bitsurfer

medium

Lack of revoke or clear token approval when removed from office

Summary

Lack of revoke token approval when removed from office

Vulnerability Detail

In CouncilMember there is removeFromOffice function to replace an existing council member with a new one.

The function basically withdraw all rewards to rewardRecipient and then transfer the tokenId to the new council member (or existing council member).

In the function, there is missing one element of removal, that is clearing the approval of that tokenId (_tokenApproval). There is a possible condition where the tokenId previously was approve to some other address via approve function. This _tokenApproval is not cleared in this removeFromOffice operation.

If we check again, the _isAuthorized is being used in _isAuthorized override function in ERC721, and can be check whether approved address is allowed to manage owner's tokens, or tokenId in particular (ignoring whether it is owned by owner). Thus, this should be cleared too when removing the tokenId from original owner.

The approved address (or person) can front-run or act when there is removeFromOffice, and to perform any action which they should not have the capability anymore after removed from office.

File: CouncilMember.sol
122:     function removeFromOffice(
123:         address from,
124:         address to,
125:         uint256 tokenId,
126:         address rewardRecipient
127:     ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
128:         // Retrieve and distribute any pending TELCOIN for all council members
129:         _retrieve();
130:         // Withdraw all the TELCOIN rewards for the specified token to the rewardRecipient
131:         _withdrawAll(rewardRecipient, tokenId);
132:         // Transfer the token (representing the council membership) from one address to another
133:         _transfer(from, to, tokenId);
134:     }

Impact

Approved user of tokenId can still execute operation as authorized user of tokenId since the _tokenApproval is not cleared

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

Consider to clear up the token approval for that corresponding tokenId off removeFromOffice operation

Duplicate of #35

sherlock-admin2 commented 5 months ago

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

takarez commented:

invalid because { this is invalid because accroding to what i undertand; the governor is the approved user only and thats what allowed him to transfer the token even if he isnt the owner; in case of the transfer; the only isAuthorized; its used for the governace incase he tries to transfer(removeFromOffice) again; but for user to pass there is need of hiim to be the owner; so invalid.}