Platform: Code4rena
Start Date: 06/05/2021
Pot Size: $66,000 USDC
Total HM: 16
Participants: 11
Period: 6 days
Judge: cemozer
Total Solo HM: 9
Id: 8
League: ETH
Rank: 1/11
Findings: 9
Award: $14,938.39
🌟 Selected for report: 11
🚀 Solo Findings: 3
1388.8806 USDC - $1,388.88
@cmichelio
ERC20FlashMintUpgradeable.flashLoan does not check for an overflow when adding the fees to the flashloan amount.
The functionality might have been copied from https://eips.ethereum.org/EIPS/eip-3156 but this one already has overflow checks as it uses solidity 0.8.0.
This leads to an issue where the attacker does not need to pay back the flashloan as they will burn 0 tokens:
_burn(address(receiver), amount + fee);
They end up with a huge profit.
Luckily, this is currently not exploitable as the fee is set to 0 so there's no possibility to overflow. However, if governance decides to change the flashloan fee, flashloans can be taken without having to repay them.
Use SafeMath.
#0 - 0xKiwi
2021-05-21T00:24:09Z
Upgraded to 0.8.x.
🌟 Selected for report: cmichel
@cmichelio
NFTXEligiblityManager._sendForReceiver should check returnData.length == 1 before decoding, otherwise if it returns no return data, the abi.decode call and with it the whole distribute function will revert.
A single badly implemented feeReceiver can break the whole distribute function and do a denial of service by reverting the transaction.
Change to: bool tokensReceived = returnData.length == 1 && abi.decode(returnData, (bool));.
#0 - cemozerr
2021-05-25T20:58:36Z
Marking this as high risk because one nefarious feeReceiver can in fact deny other users to receive their fees
3429.3348 USDC - $3,429.33
@cmichelio
NFTXVaultUpgradeable.getRandomTokenIdFromFund does not work with ERC1155 as it does not take the deposited quantity1155 into account.
Assume tokenId0 has a count of 100, and tokenId1 has a count of 1.
Then getRandomId would have a pseudo-random 1:1 chance for token 0 and 1 when in reality it should be 100:1.
This might make it easier for an attacker to redeem more valuable NFTs as the probabilities are off.
Take the quantities of each token into account (quantity1155) which probably requires a design change as it's currently hard to do without iterating over all tokens.
#0 - cemozerr
2021-05-25T21:10:45Z
Marking this as high risk as an attacker can weed out high-value NFTs from a vault putting other users funds at risk
#1 - 0xKiwi
2021-06-30T01:14:46Z
Hello. The rationale for assigning this as a high risk issue doesn't make much sense. If NFTs are in a pool its because they are all the same value. If anyone wants a rarer they can either just game it or perform a target redeem.
224.9987 USDC - $225.00
@cmichelio
The NFTXVaultUpgradeable.getPseudoRand is not really random and can be predicted.
It's also easy to make sure that one gets the correct token by having a smart contract simulate the randomness logic before the call to redeem / swap.
When redeeming one should only be able to specify the NFTs when enableDirectRedeem is true which allows specifying specificIds.
With randomness that is resolved in the same transaction, one can circumvent this restriction and get the same behaviour as if enableDirectRedeem is true even though it's false.
A smart contract can redeem and check the result of the redeem and if it's not the desired result, the transaction is reverted and retried.
This predictability can only be circumvented with a two-step process that doesn't resolve the randomness in the same step as the tokens are transferred.
🌟 Selected for report: cmichel
2286.2232 USDC - $2,286.22
@cmichelio
NFTXEligiblityManager.distribute iterates over all _feeReceivers.
If the number of _feeReceivers gets too big, the transaction's gas cost could exceed the block gas limit and make it impossible to call distribute at all.
Keep the number of _feeReceivers small.
🌟 Selected for report: cmichel
@cmichelio
The fees in NFTXVaultUpgradeable can be set arbitrarily high (no restriction in setFees).
The manager can frontrun mints and set a huge fee (for example fee = base) which transfers user's NFTs to the vault but doesn't mint any pool share tokens in return for the user.
Similar griefing attacks are also possible with other functions besides mint.
Check for a max fee as a percentage of base (like 10%) whenever setting fees.
@cmichelio
When dealing with ERC721 (instead of 1155) the amounts array is ignored, which leads to an issue.
User can call NFTXMintRequestEligibility.requestMint for an ERC721 with amounts[i] = 0.
The ERC721.transferFrom is still executed but user cannot reclaimRequestedMint later and the NFT is stuck as it checks (amounts[i] > 0).
Tokens can get stuck.
Also, subscribers to Request event could be tricked by specifying amounts[i] > 1 in the ERC721 case, as only one token was transferred but the amount multiple quantities get logged.
requestMint: Check amounts[i] == 1 in ERC721 case, amounts[i] > 0 in 1155 case.
🌟 Selected for report: cmichel
@cmichelio
The emergencyExit/emergencyExitAndClaim functions take the staking and reward tokens as parameters and trust them for the withdrawal.
This does not lead to a critical issue (like being able to withdraw all funds) as one cannot deploy a fake reward smart contract to a _rewardDistributionTokenAddr and a random address without a smart contract will fail because of the dist.balanceOf(msg.sender) call not returning any data.
However, checking if the distribution token exists is still recommended.
Require isContract(dist).
🌟 Selected for report: cmichel
@cmichelio
Missing parameter validation for functions:
NFTXEligiblityManager.addModule, updateModuleNFTXFeeDistributor all setter functions (setTreasuryAddress, ...)NFTXVaultUpgradeable.setManagerSome wallets still default to zero addresses for a missing input which can lead to breaking critical functionality like setting the manager to the zero address and being locked out.
Validate the parameters.
0xsomeone
The impact of this finding is difficult to estimate as the contract system within scope is limited in how the various components are meant to be utilized.
A definitive side-effect of this re-entrancy is the delayed application of the afterRedeemHook which, in some implementations, renders NFTs illegible which would not be the case during the re-entrancy's execution. Another side-effect is that the quantity1155 or holdings would be out-of-sync and would indicate the NFT / EIP-1155 token amount to still be "in the system" when it is not.
The safeTransferFrom implementations of both ERC1155 and ERC721 in withdrawNFTsTo contain a callback hook on the recipient of the transfer in case they are a contract as the standard dictates that smart contract recipients should be aware of the transfer.
While re-entrancies are prevented via the nonReentrant modifier for most system functions, they are not done so for swapTo (and consequently swap) invocations meaning that it is still possible to re-enter the system at this stage. Additionally, re-entrancy is still possible in other segments of the codebase i.e. ones that rely on the eligibility contracts.
Manual Review.
The afterRedeemHook paradigm should be changed to a beforeRedeemHook paradigm to ensure that all state changes are applied prior to external calls according to the Checks-Effects-Interactions pattern. Additionally, the state changes within withdrawNFTsTo should occur prior to the safeTransferFrom invocations.
#0 - 0xKiwi
2021-05-20T21:53:17Z
We have made swap and swapTo reentrant safe.
@cmichelio
The following code does not use SafeMath and can potentially lead to overflows:
NFTXFeeDistributor.distributeNFTXFeeDistributor._sendForReceiverWhile looping through all _feeReceivers it could be that a broken vault was whitelisted that allows an attacker to perform an external call and break the invariant that always 1000 tokens are left in the contract.
Add SafeMath to _sendForReceiver even though one would expect the math to be safe.
#0 - 0xKiwi
2021-05-13T22:32:39Z
Confirmed and updated all code to 0.8.x.
@cmichelio
The NFTXVaultUpgradeable.flashLoan is not correctly implemented according to EIP-3156 (but it tries to implement it as it inherits from IERC3156FlashLenderUpgradeable).
"If successful, flashLoan MUST return true." - https://eips.ethereum.org/EIPS/eip-3156
It misses the return and currently always returns false.
Always returning false indicates that the flash loan was unsuccessful when in reality it could have been successful.
This breaks any contract trying to integrate with it.
Add the return statement: return super.flashLoan(...)
#0 - cemozerr
2021-05-25T21:05:55Z
Keeping this as low-risk as flash loan returning the project does not pose a security threat for the NFTX project itself
🌟 Selected for report: cmichel
323.8167 USDC - $323.82
@cmichelio
StakingTokenProvider.nameForStakingToken: if (keccak256(abi.encode(_pairedPrefix)) == keccak256(abi.encode(address(0)))) can be simplified to if(bytes(_pairedPrefix).length== 0)