The Protocol breaks the Allowance Mechanism of the NFTs

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/Common.sol#L392 https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/V3Utils.sol#L76-L85 https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/V3Utils.sol#L171 https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/V3Automation.sol#L92

Vulnerability details

Impact

The user loses their approved entities

Proof of Concept

The protocol transfers the Position NFT from the position owner to the protocol and back to the position owner to execute the actions.

List of occurences

```solidity Contract: Common.sol 402: params.nfpm.transferFrom(address(this), params.recipient, result.tokenId); ``` ```solidity Contract: V3Utils.sol 171: nfpm.transferFrom(address(this), from, tokenId); ``` ```solidity Contract: V3Utils.sol 76: function execute(INonfungiblePositionManager _nfpm, uint256 tokenId, Instructions calldata instructions) whenNotPaused() external 77: { 78: // must be approved beforehand 79: _nfpm.safeTransferFrom( 80: msg.sender, 81: address(this), 82: tokenId, 83: abi.encode(instructions) 84: ); 85: } ``` ```solidity Contract: V3Automation.sol 92: params.nfpm.transferFrom(positionOwner, address(this), params.tokenId); ``` ```solidity Contract: V3Automation.sol 167: params.nfpm.transferFrom(address(this), positionOwner, params.tokenId); ```

This breaks the allowance mechanism of ERC721 for every action taken on the platform and it omits the allowances given from the token owners to the approved entities. For every NFT transfer, the allowances are zeroed out:

Contract: ERC721.sol

333:     function _transfer(address from, address to, uint256 tokenId) internal virtual {
334:         require(ERC721.ownerOf(tokenId) == from, "ERC721: transfer from incorrect owner");
335:         require(to != address(0), "ERC721: transfer to the zero address");
336: 
337:         _beforeTokenTransfer(from, to, tokenId, 1);
338: 
339:         // Check that tokenId was not transferred by `_beforeTokenTransfer` hook
340:         require(ERC721.ownerOf(tokenId) == from, "ERC721: transfer from incorrect owner");
341: 
342:         // Clear approvals from the previous owner
343:   >>    delete _tokenApprovals[tokenId];
344: 
345:         unchecked {
346:             // `_balances[from]` cannot overflow for the same reason as described in `_burn`:
347:             // `from`'s balance is the number of token held, which is at least one before the current
348:             // transfer.
349:             // `_balances[to]` could overflow in the conditions described in `_mint`. That would require
350:             // all 2**256 token ids to be minted, which in practice is impossible.
351:             _balances[from] -= 1;
352:             _balances[to] += 1;
353:         }
354:         _owners[tokenId] = to;
355: 
356:         emit Transfer(from, to, tokenId);
357: 
358:         _afterTokenTransfer(from, to, tokenId, 1);
359:     }

E.g.:

1. Alice is on her flight to attend DSS Bangkok
2. She already approved Bob to take care of her positions until she arrives during the long flight
3. Bob interacts with Krystal to collect the LP fees.
4. Bob loses his approval
5. Alice´s position remains unattended and one of the pool tokens takes a nose dive before Bob can withdraw the liquidity and sell it.

Tools Used

Manual Review

Recommended Mitigation Steps

NFPM uses isAuthorizedForToken modifier as below:

Contract: NonfungiblePositionManager.sol

183:     modifier isAuthorizedForToken(uint256 tokenId) {
184:         require(_isApprovedOrOwner(msg.sender, tokenId), 'Not approved');
185:         _;
186:     }

Take approval of token owners rather than using onERC721Received hook.

Assessed type

Other

[3docSec]

Report is of sufficient quality

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as selected for report

[Haupc]

There are 2 ways to take approval from user

1. via approve function => this function also discard previous approval. So we can not keep other's approval.
2. via setApprovalForAll fuction => this function seems too risky for user to use

[quanghuy219]

ERC721 implementation allows only one spender on a token at a time, therefore taking user's approval for our contract will also clear other approval on the same token

mapping(uint256 tokenId => address) private _tokenApprovals;

Another approach is to use setApprovalForAll function in ERC721, which poses another concern of allowing our contract to use all user's positions.

With that in mind, we think that using onERC721Received hook is still the safest option for our users and Krystal is able to provide convenient user experience.