Token approvals are not revoked upon transfer which can cause position theft

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/OwnershipNFTs.sol#L109

Vulnerability details

Impact

Transferring the tokens from one user to the other doesn't revoke the token's approval which opens up the NFT to various attacks by the approved party. For example, the NFT/position can be stolen from receivers by owners by simply approving themselves as a spender of the token, transferring the tokens to the victims, and then transferring it back to themselves. Also, due to the flawed implementation of the _requireAuthorised function, they can also appprove external users/contracts to act as approved parties to the tokens.

Proof of Concept

1. Context

NFT tokens hold the ablity to be approved to users on an individual basis. This is seen in the OwnershipNFT, which holds the approve function that allows any of the approved users (owner, operator or token approved user) to approve a new user to spend the token. These users will always pass the _requireAuthorised check and as such can always transfer/approve the 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];

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

This approval, though, for security reasons, must be revoked when the NFTs are transferred to the new users. This concern can also be noted in various ERC721 implementations which clear the token approvals upon transfer to prevent unintended consequences.

2. Bug Location

OwnershipNFT.sol holds the internal _transfer function, which is called by the transferFrom1, transferFrom2, safeTransferFrom1 and safeTransferFrom2 functions, which is used by the owner/approved parties to transfer the NFT to move the NFT from the owner to the receiver.

    function _transfer(
        address _from,
        address _to,
        uint256 _tokenId
    ) internal {
        _requireAuthorised(_from, _tokenId);
        SEAWATER.transferPositionEEC7A3CD(_tokenId, _from, _to);
    }

Looking at the function, we can see that it performs the necessary steps of checking for authorization and transferring position data, but fails to revoke any exisitng approvals that the NFT might hold. The positon data is handled through the ``transfer_position_E_E_C7_A3_C_D` function, which removes positon from the posotion owner and adds it to the receiver

    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(())
    }

3. Final Effect

What this means is that any user that had been previously approved to spend the token can simply steal the NFT and its corresponding positon data from the new owner. A malicious owner can simply approve himself to spend the NFT, transfer it to the victim, potentially for a price on NFT marketplaces, then transfer it back to himself. Since the function transfers users positons through the transferPositionEEC7A3CD function, the positon which had been initially removed from the malicious owner and granted to the victim will now be removed from the victim and granted back to the malicious owner. If the postion had racked up any fees within that moment, the fees can also be stolen.
Also due to the flawed implementation of the _requireAuthorised function, it allows for a token approved user to approve another user to the same token (although at the cost of losing his approval, but goes against standard implementations), the malicious owner can simply approve another user to steal the NFT and its corresponding position data. In short, loss of funds for the victim due to an unwarranted backdoor in his NFT.

Tools Used

Manual code review

Recommended Mitigation Steps

Delete all token approvals immediately after transfer.

    function _transfer(
        address _from,
        address _to,
        uint256 _tokenId
    ) internal {
        _requireAuthorised(_from, _tokenId);
        SEAWATER.transferPositionEEC7A3CD(_tokenId, _from, _to);
    }
+    delete getApproved[_tokenId];

Assessed type

Token-Transfer

[c4-judge]

alex-ppg marked the issue as satisfactory