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 owner's 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.
Proof of Concept
PoC-1:
Alice grants Eve approval to transfer a particular NFT out of its vault using approveTransferERC721()
Eve, who has transfer rights to that NFT from Alice’s vault, transfers that NFT to Bob using transferERC721()
Alice decides to buy back that NFT (e.g. because it is now considered rare and more valuable) from Bob and transfers it back to its vault
Eve, who continues to have transfer rights to that NFT from Alice’s vault, can steal that NFT and transfer to anyone
PoC-2:
Alice grants Eve approval to transfer a particular NFT out of its vault using approveTransferERC721()
Alice learns that Eve’s keys are compromises or that Eve is malicious and wants to revoke the approval but there is no mechanism to do so
Eve (or whoever stole her credentials) has transfer rights to that NFT from Alice’s vault and can steal that NFT and transfer to anyone
Add 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.
ghost
commented
3 years ago 
ghoul-sol
commented
3 years ago 
ztcrypto
Handle
0xRajeev
Vulnerability details
Impact
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 owner's 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.
Proof of Concept
PoC-1: Alice grants Eve approval to transfer a particular NFT out of its vault using approveTransferERC721() Eve, who has transfer rights to that NFT from Alice’s vault, transfers that NFT to Bob using transferERC721() Alice decides to buy back that NFT (e.g. because it is now considered rare and more valuable) from Bob and transfers it back to its vault Eve, who continues to have transfer rights to that NFT from Alice’s vault, can steal that NFT and transfer to anyone
PoC-2: Alice grants Eve approval to transfer a particular NFT out of its vault using approveTransferERC721() Alice learns that Eve’s keys are compromises or that Eve is malicious and wants to revoke the approval but there is no mechanism to do so Eve (or whoever stole her credentials) has transfer rights to that NFT from Alice’s vault and can steal that NFT and transfer to anyone
https://github.com/code-423n4/2021-05-visorfinance/blob/e0f15162a017130aa66910d46c70ee074b64dd40/contracts/contracts/visor/Visor.sol#L477-L487
https://github.com/code-423n4/2021-05-visorfinance/blob/e0f15162a017130aa66910d46c70ee074b64dd40/contracts/contracts/visor/Visor.sol#L489-L522
Tools Used
Manual Analysis
Recommended Mitigation Steps
Add 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.