# 31281 - \[SC - Low] Approved spender cannot withdraw or merge Submitted on May 16th 2024 at 07:15:47 UTC by @OxAnmol for [Boost | Alchemix](https://immunefi.com/bounty/alchemix-boost/) Report ID: #31281 Report type: Smart Contract Report severity: Low Target: Impacts: * Temporary freezing of NFTs ## Description ## Brief/Intro 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` ```solidity function merge(uint256 _from, uint256 _to) external { ...SNIP... _burn(_from, value0); _depositFor(_to, value0, end, _locked1.maxLockEnabled, _locked1, DepositType.MERGE_TYPE); } ``` Now if we have a look at `_burn` it calls the public `approve` function to set the token approval to `address(0)` ```solidity 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. ```solidity 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. ### Recommendation Instead of calling approve it is recommended to set the approval to address(0) or delete it directly. ```diff 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 - approve(address(0), _tokenId); + idToApprovals[tokenId] = address(0); // Checkpoint for gov _moveTokenDelegates(delegates(owner), address(0), _tokenId); // Remove token _removeTokenFrom(owner, _tokenId); emit Transfer(owner, address(0), _tokenId); emit Supply(supplyBefore, supplyAfter); } ``` ## Impact Details approved user is unable to execute ordinary operations due to a logic flaw which can freeze the NFT for them temporarily. As per this impact i belive the high is appropriate according to severity guidelines which accounts `Temporary freezing of NFT` as High. ## References ## Proof of Concept Paste this test inside `VotingEscrow.t.sol`. The test will pass on revert expectation as the merge is called by approved user. ```solidity function testMergeTokensRevertEvenWhenCallerIsApproved() public { uint256 tokenId1 = createVeAlcx(admin, TOKEN_1, MAXTIME, false); uint256 tokenId2 = createVeAlcx(admin, TOKEN_100K, MAXTIME / 2, false); // Approve both token to Beef hevm.startPrank(admin); veALCX.approve(beef, tokenId1); veALCX.approve(beef, tokenId2); hevm.stopPrank(); hevm.startPrank(beef); uint256 lockEnd1 = veALCX.lockEnd(tokenId1); assertEq(lockEnd1, ((block.timestamp + MAXTIME) / ONE_WEEK) * ONE_WEEK); assertEq(veALCX.lockedAmount(tokenId1), TOKEN_1); // Vote to trigger flux accrual hevm.warp(newEpoch()); address[] memory pools = new address[](1); pools[0] = alETHPool; uint256[] memory weights = new uint256[](1); weights[0] = 5000; voter.vote(tokenId1, pools, weights, 0); voter.vote(tokenId2, pools, weights, 0); voter.distribute(); hevm.warp(newEpoch()); // Reset to allow merging of tokens voter.reset(tokenId1); voter.reset(tokenId2); uint256 unclaimedFluxBefore1 = flux.getUnclaimedFlux(tokenId1); uint256 unclaimedFluxBefore2 = flux.getUnclaimedFlux(tokenId2); hevm.expectRevert(abi.encodePacked("sender is not owner or approved")); veALCX.merge(tokenId1, tokenId2); // This will revert but it shouldn't hevm.stopPrank(); } ``` --- # Agent Instructions: Querying This Documentation If you need additional information that is not directly available in this page, you can query the documentation dynamically by asking a question. Perform an HTTP GET request on the current page URL with the `ask` query parameter: ``` GET https://reports.immunefi.com/alchemix/31281-sc-low-approved-spender-cannot-withdraw-or-merge.md?ask= ``` The question should be specific, self-contained, and written in natural language. The response will contain a direct answer to the question and relevant excerpts and sources from the documentation. Use this mechanism when the answer is not explicitly present in the current page, you need clarification or additional context, or you want to retrieve related documentation sections.