XPAND - Commnity - Security Review

XPAND_Security_Review.md

XPAND - Security Review

Table of contest

• Introduction
• CommunityFactory.sol
• Community.sol

Introduction

XPAND team requested the review of the contracts under the private repository xg-smart-contracts referenced by the commit dc937eb9ba17e1d4886826fd0579febbe3ecb3ad: CommunityFactory.sol and Community.sol.

The rest of the contracts in the repositories are assumed to be audited.

The documentation of the project has been provided privately as well. The team has requested a light audit due to time constraints, so the report has not considered optimizations and some "informationals".

CommunityFactory.sol

Low

• L1 - line 15 - Override Community - A sender can create two or more communities by calling multiple times createCommunity. The communityMappings variable only keep the latest community created and therefore, previously created communities will be lost from the storage. getDeployedCommunity will return only the latest created. There are a lot of things to consider like preventing a user to create more than one community and/or emitting an event every time a community is created to index every community created off-chain.

Informational

• I1 - line - Consider using a multisig as the contract manager and add a transferManager method so you won't need to re-deploy the factory if the manager needs to be changed.

Community.sol

Critical

• C1 - line 81 - The contract is prepared to receive ETH but there is no way to transfer the ETH out of the contract. That ETH will be stuck forever in the smart contract. Consider preventing the receive of ETH

High

• H1 - lines 35, 41, 50, 85, and 98 - Possible token drain - As the tokenAddress can change times, users can create a withdrawal request for the current token: token1. But if the current token changes to token2, the owner of the contract does not have a way to check the token used to create the withdrawal request and therefore, can end up approving withdrawal requests of the previous token but using the new one causing the token2's drain and/or users receiving a different token from what they requested. Consider adding the tokenAddress to the withdrawal request.

• H2 - lines 78 - Owner can't remove users' withdrawal requests - The function removeMember can only be called by the onwer of the contract. msg.sender instead of the memberAddress is being used to delete the withdrawal request associated to an address. This means that the user will be removed as a community member but not their pending withdrawal. In consequence, the owner's pending withdrawal will be deleted. Consider using memberAddress instead of msg.sender.

Medium

• M1 - line 35 - Create multiple community currencies - The owner of the contract can set the tokenAddress multiple times by calling createCommunityCurrency. Consider using a mapping of available currencies or prevent the currency to be set only once.

• M2 - line 181 - Re-Buy listed NFTs - The buyNt function transfers an ERC721 or ERC1155 token using the safeTransferFrom. On each safeTransferFrom if _to is a smart contract, the function onERC721Received or onERC1155Received is called on _to context giving _to the control of the transaction flow. This smart contract can trigger another valid recipient to call the buyNft function performing a reentrancy attack. The nftItems[itemId] mapping is valid as it has not been deleted yet; therefore, the caller can buy the NFT again. The attacker will pay for the NFT but the vulnerability comes if the owner (seller) puts for sale more than one ERC1155 token with the same id but at different prices. The attacker can end up buying ERC1155 tokens by paying the previous price. Consider moving the delete nftItems[itemId] (line 198) before the safeTransferFrom interactions following the check-effects-interaction pattern. Also, consider adding reentrancy protection for each method that does external calls and can be called by anyone.

• M3 - line 247 - Reuse airdrop when claiming an NFT - The claim function transfers an ERC721 or ERC1155 token using the safeTransferFrom. On each safeTransferFrom if _to is a smart contract, the function onERC721Received or onERC1155Received is called on _to context giving _to the control of the transaction flow. This smart contract can trigger another valid recipient to call the claim function performing a reentrancy attack. The nextClaimId will be valid as it has not been incremented yet and therefore the caller can get the same airdrop as the first sender. This attack window can only happen for ERC1155 tokens where the token id is the same. Consider moving the nextClaimId++ (line 261) before the safeTransferFrom interactions following the check-effects-interaction pattern. Also, consider adding reentrancy protection for each method that does external calls and can be called by anyone.

Low

• L1 - Clear storage - Consider adding functions to clear orders (nftItems) if the NFT associated has been transferred out of the contract by using withdrawNFTS.

Informational

• I1 - line 17 - manager is not being used anywhere. Update: in the doc the team is aware of this.
• I2 - lines 32, 44, and 120 - Amount format - It is a good practice to receive amounts on wei. The right amount can be calculated in the front end used to interact with the contract. That will reduce future errors and the gas consumed in every transaction.
• I3 - line 57 - Obsolete method getWithdrawalRequest - The mapping withdrawalRequests is public and therefore everyone can access it. Moreover every data that is stored on-chain can be accessed by looking at historic transactions.
• I4 - lines 93 and 107 - CEI pattern - Switch lines 93/95 and 107/109 to follow the CEI pattern to avoid further issues https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html.
• I5 - Consider adding a transferOwnership function.

Ignacio Mazzara - October 2022.