Skip to content
This repository has been archived by the owner on Jan 12, 2025. It is now read-only.

scammed - Approved users with approvedForAll cannot act over lsNFT #519

Closed
sherlock-admin4 opened this issue Jul 15, 2024 · 0 comments
Closed
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A Medium severity issue. Reward A payout will be made for this issue

Comments

@sherlock-admin4
Copy link

sherlock-admin4 commented Jul 15, 2024

scammed

Medium

Approved users with approvedForAll cannot act over lsNFT

Summary

NFT approved users with setApprovalForAll cannot act as a single approved user.

Vulnerability Detail

NFT can be approved in 2 ways, by approve and by setApprovalForAll. _requireOnlyApprovedOrOwnerOf should allow approved users to call privilege functions of lsNFT, but since NFT can only have one approved user with approve and multiple with setApprovalForAll, if the caller is approved with setApprovalForAll, he cannot act over NFT because the setApprovalForAll check is missing.

MlumStaking.sol#L148-L151

function _requireOnlyApprovedOrOwnerOf(uint256 tokenId) internal view {
    require(_ownerOf(tokenId) != address(0), "ERC721: operator query for nonexistent token");
    require(_isOwnerOf(msg.sender, tokenId) || getApproved(tokenId) == msg.sender, "FORBIDDEN");
}

Impact

A user who is approved with setApprovalForAll cannot call functions in MlumStaking.sol.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MlumStaking.sol#L148-L151

Tool used

Manual Review

Recommendation

Change _requireOnlyApprovedOrOwnerOf to check for ERC721::isApprovedForAll as well, just like in ERC721::_isAuthorized.

Duplicate of #378

@github-actions github-actions bot added the Excluded Excluded by the judge without consulting the protocol or the senior label Jul 21, 2024
@0xSmartContract 0xSmartContract added Medium A Medium severity issue. Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label and removed Excluded Excluded by the judge without consulting the protocol or the senior labels Jul 27, 2024
@sherlock-admin4 sherlock-admin4 changed the title Soft Mint Lizard - Approved users with approvedForAll cannot act over lsNFT scammed - Approved users with approvedForAll cannot act over lsNFT Jul 29, 2024
@sherlock-admin4 sherlock-admin4 added the Reward A payout will be made for this issue label Jul 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A Medium severity issue. Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

2 participants