when NFT position is transferred, approvals are not cleared

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L554-L569
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/OwnershipNFTs.sol#L109-L116

Vulnerability details

Impact

Approvals are not being cleared when OwnershipNFT is being transferred by owner. NFT can be sold and later on can be stolen back along with all the deposited tokens.

Proof of Concept

Looking at the OZ ERC721 implementation we will see the following function being called in transferFrom and transfer:

ERC721.sol

function _update(address to, uint256 tokenId, address auth) internal virtual returns (address) {
    address from = _ownerOf(tokenId);

    // Perform (optional) operator check
    if (auth != address(0)) {
        _checkAuthorized(from, auth, tokenId);
    }

    // Execute the update
    if (from != address(0)) {
        // Clear approval. No need to re-authorize or emit the Approval event
        _approve(address(0), tokenId, address(0), false);

        unchecked {
            _balances[from] -= 1;
        }
    }

    if (to != address(0)) {
        unchecked {
            _balances[to] += 1;
        }
    }

    _owners[tokenId] = to;

    emit Transfer(from, to, tokenId);

    return from;
}

As we can see approve with address(0) is called each time, which clears the per token and operator approvals of the previous owner, exactly to prevent the old approvers to steal back the NFT.

However, in OwnershipNFT approvals are not being cleared and will remain even after the position is transferred:

OwnershipNFT.sol

function _transfer(
      address _from,
      address _to,
      uint256 _tokenId
  ) internal {
      _requireAuthorised(_from, _tokenId);
      SEAWATER.transferPositionEEC7A3CD(_tokenId, _from, _to);
  }
  
function _requireAuthorised(address _from, uint256 _tokenId) internal view {
    // revert if the sender is not authorised or the owner
    bool isAllowed =
        msg.sender == _from ||
        isApprovedForAll[_from][msg.sender] ||
        msg.sender == getApproved[_tokenId];

    require(isAllowed, "not allowed");
    require(ownerOf(_tokenId) == _from, "_from is not the owner!");
}

Note that the Rust code, lib::transfer_position_E_E_C7_A3_C_D in particular, doesn’t manage the approvals, it cares only about adding/removing the position owner:

lib.rs

pub fn transfer_position_E_E_C7_A3_C_D(
    &mut self,
    id: U256,
    from: Address,
    to: Address,
) -> Result<(), Revert> {
    assert_eq_or!(msg::sender(), self.nft_manager.get(), Error::NftManagerOnly);

    self.remove_position(from, id);
    self.grant_position(to, id);

    #[cfg(feature = "log-events")]
    evm::log(events::TransferPosition { from, to, id });

    Ok(())
}

fn remove_position(&mut self, owner: Address, id: U256) {
    // remove owner
    self.position_owners.setter(id).erase();

    // decrement count
    let owned_positions_count = self.owned_positions.get(owner) - U256::one();
    self.owned_positions
        .setter(owner)
        .set(owned_positions_count);
}

fn grant_position(&mut self, owner: Address, id: U256) {
    // set owner
    self.position_owners.setter(id).set(owner);

    // increment count
    let owned_positions_count = self.owned_positions.get(owner) + U256::one();
    self.owned_positions
        .setter(owner)
        .set(owned_positions_count);
}

So approvals are managed only in OwnershipNFT, which is also the nft_manager in lib.rs, and everyone approved by the old owner can do anything with the new owner’s position- to increase, decrease, burn or even transfer it back to himself.

For example, when malicious owner wants to steal from other users he can do the following:

1. Lists his position for sale, but before that approves himself, or another address that also belongs to him.
2. Unsuspecting user buys the position and start providing liquidity from it.
3. The buyer accumulates uncollected fees as well as great amount of token_owed_0 and token_owed_0.
4. When the old owner is satisfied with the amount that he will receive he executes OwnershipNFT::transferFrom from the approved address.
5. _requireAuthorised passes because msg.sender == getApproved[_tokenId]
6. lib::transfer_position_E_E_C7_A3_C_D wipes the buyer as position owner and grants the ownership back to the old owner, who can choose whether to continue using the stolen position or collect it and close it stealing all the liquidity from it.

Tools Used

Manual Review

Recommended Mitigation Steps

In _transfer make sure to clear the approvals for the given token:

function _requireAuthorised(address _from, uint256 _tokenId) internal view {
    // revert if the sender is not authorised or the owner
    bool isAllowed =
        msg.sender == _from ||
        isApprovedForAll[_from][msg.sender] ||
        msg.sender == getApproved[_tokenId];

+  approve(address(0), _tokenId);
+  setApprovalForAll(address(0), _tokenId);

    require(isAllowed, "not allowed");
    require(ownerOf(_tokenId) == _from, "_from is not the owner!");
}

Assessed type

ERC721

[c4-judge]

alex-ppg marked the issue as satisfactory