Users who are approved, but do not own a particular NFT, are supposed to be eligible to call merge and withdraw from the NFT.
Currently, _burn(), used by merge() and withdraw() to remove the NFT from the system, will revert unless the sender is the owner of the NFT as the public approve called inside_burn requires the sender to be the owner or operator.
Vulnerability Details
The merge function and withdraw function is calling internal _burn
Now if we have a look at _burn it calls the public approve function to set the token approval to address(0)
function _burn(uint256 _tokenId, uint256 _value) internal {
address owner = ownerOf(_tokenId);
// Update the total supply of deposited tokens
uint256 supplyBefore = supply;
uint256 supplyAfter = supplyBefore - _value;
supply = supplyAfter;
// Clear approval
//@audit-issue This will revert for approved users
approve(address(0), _tokenId);
// Checkpoint for gov
_moveTokenDelegates(delegates(owner), address(0), _tokenId);
// Remove token
_removeTokenFrom(owner, _tokenId);
emit Transfer(owner, address(0), _tokenId);
emit Supply(supplyBefore, supplyAfter);
}
The main issue lies in this approve, which checks if the msg.sender is the owner and operator of the tokenId.
function approve(address _approved, uint256 _tokenId) public {
address owner = idToOwner[_tokenId];
// Throws if `_tokenId` is not a valid token
require(owner != address(0), "owner not found");
// Throws if `_approved` is the current owner
require(_approved != owner, "Approved is already owner");
// Check requirements
bool senderIsOwner = (owner == msg.sender);
bool senderIsApprovedForAll = (ownerToOperators[owner])[msg.sender];
//@audit-issue Check will fail for the approved user who calls merge and withdraw
->> require(senderIsOwner || senderIsApprovedForAll, "sender is not owner or approved");
// Set the approval
idToApprovals[_tokenId] = _approved;
emit Approval(owner, _approved, _tokenId);
}
The approve function implementation itself is correct if the external users call it but in this case the merge and withdraw is also using the same function which causes the issue.
Note
The same issue was also submitted in the Velodrome c4 audit back in 2022. In that case, the problem was the same but the cause was different. https://github.com/code-423n4/2022-05-velodrome-findings/issues/66
Recommendation
Instead of calling approve it is recommended to set the approval to address(0) or delete it directly.
