edu3labs - audit

CertiK4 Jul 2023Files named in the report

Audit

CertiK reviewed Edu3Labs (edu3labs - audit) on 4 Jul 2023: 20 findings, 0 critical; 18 marked fixed, 2 open.

No open critical findings

Open the report Open Edu3Labs’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings20

Statuses are the security firm’s own, from its report.

Severity	Open	Fixed
Critical	0	0
High	1	1
Medium	1	3
Low	0	14

HighCentralized Control of Contract UpgradeFixed

`Edu3LabsMarketplace` are upgradeable contract, the owner can upgrade the contract without the community's commitment. If an attacker compromises the account, he can change the implementation of the contract and drain tokens from the contract.

MediumLack Of Access ControlFixed

The function `batchMint()` can be called by anyone as it has no access restriction. This enables anyone to call this and mint nfts for anyone.

MediumPotential Reentrancy AttackFixed

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects.

MediumVariables cannot be initialized in the constructoFixed

The function `_setupRole()` is called to set the role, but since the contract `Edu3LabsMarketplace` is upgradeable, the variable `_roles` is not initialized in the proxy contract. ```solidity mapping(bytes32 => RoleData) private _roles; ```

LowCase that will not occurFixed

If the variable `_startTime` is equal to 0, indicating that `_startTime` is less than `block.timestamp`, then on line 339, `_startTime` is updated to `block.timestamp`. Therefore, on line 342, the variable `_startTime` cannot be equal to 0.

LowEdu3LabsMarketplace Not Support ERC721Fixed

The contract `Edu3LabsMarketplace` is designed to facilitate the buying and selling of tokens in a marketplace. According to the code comments, the contract is intended to allow for the trading of ERC721 tokens. However, the contract does not implement the corresponding `onReceived` function, which means the contract is unable to receive ERC721 tokens.

LowLack of Input ValidationFixed

The function `createListing()` is designed to list tokens for sale and supports two types of selling options: Direct Listing or Auction. However, the function does not check the listing type, which may lead to unpredictable issues.

LowMint Token FailureFixed

In the function `verifiedMint()`, the nft token with indicated `_nftId` will be minted to the `msg.sender` address, and then the function `setApprovalForAll()`, which always fails, will be called. ```solidity=124 function setApprovalForAll( address, bool ) public pure override { revert("Soulbound"); } ```

LowMissing Zero Address ValidationFixed

Addresses are not validated before assignment or external calls, potentially allowing the use of zero addresses and leading to unexpected behavior or vulnerabilities. For example, transferring tokens to a zero address can result in a permanent loss of those tokens.

LowPartial Test FailuresFixed

Here are the results and failing test cases for reference. ```shell Compiled 3 Solidity files successfully Admin & Admin Proxy Test Deployment ✔ Should set the owner and admin address correctly (1003ms) ✔ should deploy and register a proxy to admin (70ms) ✔ should be able to add allowed market places ✔ should be able to remove allowed market places (51ms) ✔ should be able to change the multisig address ✔ should be set native Marketplace ✔ should be able to change the native Marketplace (50ms) ✔ should be able to set fee on marketplace (59ms) ✔ multisigners should be able to add or delete allowed nfts (58ms) ✔ proxy should be able to add allowed nfts (79ms) CollectionFactory Deployment ✔ should deploy (38ms) Create Collection ✔ should deploy collection return the address (55ms) 1) should set right owner to nft collection Multisig ✔ should deploy with correct owners ✔ should be able to set new owners ✔ should be able to remove owners ✔ should numConfirmationsRequired can be changed by multisign Edu3LabsVerifier Deployment ✔ Should set the right admin (38ms) Signature verifiying ✔ should verify a valid signature 2) modifier should revert if signature is invalid 3) modifier should continue if signature is valid 4) if nft id is not valid should revert 5) should revert if signature is not valid Marketplace contract test Deployment ✔ Should set the right owner (137ms) ✔ should give test erc to owner Listing 6) should list a new item ✔ should be listed if created from our proxy NFTCollection Deployment 7) Should set the right admin Mint 8) should prevent if signature is not valid 9) should prevent if nft id is not valid 10) should mint if signature is valid 11) should prevent minting nft with same id 12) should set uri after minted 13) should prevent if nft is not minted by owner 14) should prevent listing if not whitelisted 21 passing (2s) 14 failing ```

Show 8 more

LowPotential Cross-Chain Replay AttackFixed

Signed messages are not properly verified with the current chain ID, thus allowing attackers to perform replay attacks across chains. Hardcoded or cached chain ID values are also vulnerable since a hard fork may occur and change the chain ID in the future. ```solidity=113 return ecrecover(_ethsignedMessageHash, v, r, s); ``` - Calling `ecrecover` with a hash that does not properly include the chain ID.

LowPotential incorrect assignment logicFixed

The `createListing()` function is called to list the user's tokens. When the passed `startTime` is less than `block.timestamp`, the `startTime` is assigned to `block.timestamp` if it passes the 251 line check, so the `startTime` of all tokens is not less than `block.timestamp`. ```solidity=256 if (startTime < block.timestamp) { // if `startTime` less than `block.timestamp` // do not allow listing to start in the past (1 hour buffer) if(block.timestamp - startTime >= 1 hours) { revert ST(); } startTime = block.timestamp; // `startTime` assigned to `block.timestamp` } ```

LowPotential Payer Pay More They ShouldFixed

The `payout()` function is called to pay for the purchase of tokens, including `platformFeeCut` and `secondhandFeeCut`, but the tokens seller only receives `_totalPayoutAmount - (platformFeeCut + MAX_SELLER_FEE)` amount of tokens, if `MAX_SELLER_FEE` is greater than `secondhandFeeCut`, then the seller will receive less than it should. ```solidity=885 uint256 platformFeeCut = (_totalPayoutAmount * platformFeeBps) / MAX_BPS; ``` ```solidity=893 secondhandFeeCut = (_totalPayoutAmount * secondhandFee) / MAX_BPS; ``` ```solidity=921 CurrencyTransferLib.transferCurrencyWithWrapper( _currencyToUse, _payer, _payee, _totalPayoutAmount - (platformFeeCut + MAX_SELLER_FEE), _nativeTokenWrapper ); ``` uint256 platformFeeCut = (_totalPayoutAmount * platformFeeBps) / MAX_BPS; transferred to platformFeeRecipient\ uint256 secondhandFeeCut = (_totalPayoutAmount * secondhandFee) / MAX_BPS; transferred to originalCreator\ the remaining amount : _totalPayoutAmount - (platformFeeCut + secondhandFeeCut); uint256 max_seller_fee_cut = (_totalPayoutAmount * MAX_SELLER_FEE) / MAX_BPS;\ _totalPayoutAmount - (platformFeeCut + max_seller_fee_cut); transferred to _payee If secondhandFeeCut > max_seller_fee_cut, the payer will pay more tokens than they should.\ If secondhandFeeCut < max_seller_fee_cut, the payer will pay fewer tokens than they should.

LowRedundant ComparisonsFixed

Comparisons that are always true or always false may be incorrect or unnecessary. ```solidity=173 if (_lower < 0) { ```

LowUnknown Trusted Forwarder ImplementationFixed

The variable `_trustedForwarder` serves as a 3rd party forwarder to help it identify the address of the Transaction Signer. The contract `Edu3LabsMarketplace` override the `_msgSender()` function to get the `msg.sender` and override the `_msgData()` function to get the `msg.data`. The scope of the audit treats these 3rd party forwarders as black boxes and assumes their functional correctness. However, in the real world, 3rd parties can be compromised and this may lead to lost or stolen assets. In addition, upgrades of 3rd parties can possibly create severe impacts. Reference: [EIP-2771](https://eips.ethereum.org/EIPS/eip-2771)

LowUnsafe Integer CastFixed

Type casting refers to changing a variable of one data type into another. The code contains an unsafe cast between integer types, which may result in unexpected truncation or sign flipping of the value.

LowUsage of `transfer`/`send` for sending EtherFixed

It is not recommended to use Solidity's `transfer()` and `send()` functions for transferring Ether, since some contracts may not be able to receive the funds. Those functions forward only a fixed amount of gas (2300 specifically) and the receiving contracts may run out of gas before finishing the transfer. Also, EVM instructions' gas costs may increase in the future. Thus, some contracts that can receive now may stop working in the future due to the gas limitation. ```solidity=30 payable(_to).transfer(address(this).balance); ``` - `AdminProxy.withdraw` uses `transfer()`.

LowVote not removed when removing the ownerFixed

When a owner is removed, the previous vote is not removed. This could lead to the proposal being executed without sufficient vote.