sherlock-audit / 2024-01-telcoin-judging

6 stars 5 forks source link

zzykxx - `removeFromOffice()` does not reset `_tokenApproval` #225

Closed sherlock-admin closed 8 months ago

sherlock-admin commented 8 months ago

zzykxx

medium

removeFromOffice() does not reset _tokenApproval

Summary

The approved address for a given council member NFT is not resetted when an NFT is transferred.

Vulnerability Detail

The function removeFromOffice() in CouncilMember.sol transfers an NFT to a different address but does not reset the approval given via the _tokenApproval variable.

Impact

When a council member NFT is transferred the token will be approved to the same address as before the transfer.

Code Snippet

Tool used

Manual Review

Recommendation

Reset the approval after the transfer:

function removeFromOffice(address from,address to,uint256 tokenId,address rewardRecipient) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
    _retrieve();
    _withdrawAll(rewardRecipient, tokenId);
    _transfer(from, to, tokenId);
    _tokenApproval[tokenId] = address(0); ✅ 
}

Duplicate of #35

sherlock-admin2 commented 8 months ago

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

takarez commented:

invalid because { the approval is only for governace to allow them to have the ability to transfer(removerfromOffice) at any moment and not for users to do something; so its imposible for anyone to have the approval as its governace modifier protected.}