Closed code423n4 closed 2 years ago
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L183
Users can have more nfts than expected
Variable limitPerAccount is used to indicate the max number of NFTs an account may have while minting. https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L111
limitPerAccount
Unfortunately, user can easily bypass this feature by using multiple accounts. For example: A drop has limitPerAccount = 10.
limitPerAccount = 10
mintFromFixedPriceSale()
count = 10
/// link: https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L183 if (IERC721(nftContract).balanceOf(msg.sender) + count > saleConfig.limitPerAccount) {
=> Alice get 10 nfts.
In Example above, Alice can get 20 nfts > limitPerAccount = 10 nfts which was declared by creator.
Manual review
I think there is no good solution to mitigate this issue. Can delete this variable if not needed.
Dupe of https://github.com/code-423n4/2022-08-foundation-findings/issues/59
Lines of code
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L183
Vulnerability details
Impact
Users can have more nfts than expected
Proof of concept
Variable
limitPerAccount
is used to indicate the max number of NFTs an account may have while minting. https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L111Unfortunately, user can easily bypass this feature by using multiple accounts. For example: A drop has
limitPerAccount = 10
.mintFromFixedPriceSale()
with parametercount = 10
. => Alice has 10 nfts in balancemintFromFixedPriceSale()
with parametercount = 10
again. Since Alice transfers all of her nfts to Bob, so it pass the check=> Alice get 10 nfts.
In Example above, Alice can get 20 nfts >
limitPerAccount
= 10 nfts which was declared by creator.Tools Used
Manual review
Recommended Mitigation Steps
I think there is no good solution to mitigate this issue. Can delete this variable if not needed.