Open code423n4 opened 1 year ago
https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/NFTBoostVault.sol#L247 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/NFTBoostVault.sol#L317 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/NFTBoostVault.sol#L632 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/NFTBoostVault.sol#L472 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/NFTBoostVault.sol#L659
Users can't use NFTs with tokenID = 0 even if they have multipliers.
In NFTBoostVault.sol:setMultiplier(), we didn't force the tokenId not to be zero:
function setMultiplier(address tokenAddress, uint128 tokenId, uint128 multiplierValue) public override onlyManager { if (multiplierValue > MAX_MULTIPLIER) revert NBV_MultiplierLimit(); NFTBoostVaultStorage.AddressUintUint storage multiplierData = _getMultipliers()[tokenAddress][tokenId]; // set multiplier value multiplierData.multiplier = multiplierValue; emit MultiplierSet(tokenAddress, tokenId, multiplierValue); }
More important, in ERC-1155 spec and openzeppelin's ERC-1155 implementation, no restriction on ID=0 is applied, so tokenID=0 is not an abnormal case.
Manual Review.
We should remove tokenId != 0 restrictions in NFTBoostVault.sol.
Context
141345 marked the issue as duplicate of #235
0xean marked the issue as satisfactory
0xean changed the severity to QA (Quality Assurance)
0xean marked the issue as grade-a
Lines of code
https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/NFTBoostVault.sol#L247 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/NFTBoostVault.sol#L317 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/NFTBoostVault.sol#L632 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/NFTBoostVault.sol#L472 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/NFTBoostVault.sol#L659
Vulnerability details
Impact
Users can't use NFTs with tokenID = 0 even if they have multipliers.
Proof of Concept
In NFTBoostVault.sol:setMultiplier(), we didn't force the tokenId not to be zero:
More important, in ERC-1155 spec and openzeppelin's ERC-1155 implementation, no restriction on ID=0 is applied, so tokenID=0 is not an abnormal case.
Tools Used
Manual Review.
Recommended Mitigation Steps
We should remove tokenId != 0 restrictions in NFTBoostVault.sol.
Assessed type
Context