Submitted by 0xRajeev, also found by shw
NFT transfer approvals that are set to true in approveTransferERC721() are never set to false and there is no way to remove such an nft approval.
Impact 1: The approval is not removed (set to false) after a transfer in transferERC721(). So if the NFT is ever moved back into the owners vault again, then the previous/compromised delegate can again transfer it to any address of choice without requiring a new approval.
Impact 2: If a delegate becomes compromised/untrustworthy after granting approval but before transfer then the owner will lose its NFT because there is no mechanism to revoke the approval that was granted earlier.
PoC-1:
PoC-2:
Recommend adding a boolean parameter to approveTransferERC721() and set the nftApprovals  to that parameter which can be true for giving approval and false for removing/revoking approval
If msg.sender != _getOwner(), call approveTransferERC721() with the boolean false to remove approval before making a transfer in transferERC721() on L515.
xyz-ctrl (Visor) commented:
ghoul-sol (Judge) commented:
ztcrypto (Visor) patched:
