Approvals are not revoked on transfer

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/OwnershipNFTs.sol#L98-L116

Vulnerability details

Currently, approvals allow other users to transfer tokens on behalf of the owner. However, the approval is not revoked after a transfer, leaving the approved user with continued authority over the tokens, even after they are transferred to a new owner. This could enable the approved user to retain control over the tokens, even when they no longer belong to the original owner.

Impact

• Unauthorized Token Control: Previously approved addresses retain permissions to operate on the token even after it has been transferred to a new owner. This could lead to unauthorized transfers or actions on the new owner’s token.

• Non-Compliance with ERC721 Standard: By failing to clear approvals post-transfer, the contract deviates from the ERC721 standard, impacting both its security and functionality.

Proof of Concept

• OwnershipNFTs.sol

    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!");
    }

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

• pkg/seawater/src/lib.rs

    #[allow(non_snake_case)]
    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(())
    }

Tools Used

Manual Review

Recommended Mitigation Steps

    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 issue #160 as primary and marked this issue as a duplicate of 160

[c4-judge]

alex-ppg marked the issue as satisfactory