hats-finance / Accumulated-finance-0x75278bcc0fa7c9e3af98654bce195eaf3bb6a784

0 stars 0 forks source link

the `_transfer()` function is incompliant with eip-721 standards #53

Open hats-bug-reporter[bot] opened 2 months ago

hats-bug-reporter[bot] commented 2 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0xf7ef7eebf59eb9dc173b770fbd0444c110252ab937df2c97c741c19e49287309 Severity: low

Description: Description\ According to the EIP-721 documentation, the _transfer() function should check that the from and to addresses are not the same.

please check the documentation:\ https://eips.ethereum.org/EIPS/eip-721

We questioned if the operator parameter on onERC721Received was necessary. In all cases we could imagine, if the operator was important then that operator could transfer the token to themself and then send it – then they would be the from address. This seems contrived because we consider the operator to be a temporary owner of the token (and transferring to themself is redundant). When the operator sends the token, it is the operator acting on their own accord, NOT the operator acting on behalf of the token holder. This is why the operator and the previous token owner are both significant to the token recipient.

but there is no check in _transfer() function to avoid this issue:

function _transfer(address from, address to, uint256 tokenId) internal {
        if (to == address(0)) {
            revert ERC721InvalidReceiver(address(0));
        }
        address previousOwner = _update(to, tokenId, address(0));
        if (previousOwner == address(0)) {
            revert ERC721NonexistentToken(tokenId);
        } else if (previousOwner != from) {
            revert ERC721IncorrectOwner(from, tokenId, previousOwner);
        }
    }

Recommendation\ Consdier checking from and to addresses

function _transfer(address from, address to, uint256 tokenId) internal {
+        if (to == from) {
+           revert ERC721InvalidReceiver(address(0));
+       }

        if (to == address(0)) {
            revert ERC721InvalidReceiver(address(0));
        }
        address previousOwner = _update(to, tokenId, address(0));
        if (previousOwner == address(0)) {
            revert ERC721NonexistentToken(tokenId);
        } else if (previousOwner != from) {
            revert ERC721IncorrectOwner(from, tokenId, previousOwner);
        }
    }
0xRizwan commented 2 months ago

Its EIP721 compliant, the implementation is same as openzeppelin's ERC721.

0xdanial commented 2 months ago

@0xRizwan please check the link below it's validated on the codehawk platform

https://solodit.xyz/issues/no-check-for-transferring-to-self-codehawks-stakelink-git

ilzheev commented 2 months ago

@0xdanial https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L347-L357