Joyn contest - ych18's results

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L175

Vulnerability details

The CoreCollection.withdraw function uses payableToken.transferFrom(address(this), msg.sender, amount) to transfer tokens from the CoreCollection contract to the msg.sender ( who is the owner of the contract). The usage of transferFrom can result in serious issues. In fact, many ERC20 always require that in transferFrom allowance[from][msg.sender] >= amount, so in this case the call to the withdraw function will revert as the allowance[CoreCollection][CoreCollection] == 0 and therefore the funds cannot ben withdrawn and will be locked forever in the contract.

Recommendation : replace transferFrom with transfer

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L43-L46 https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L51-L57

Vulnerability details

Although the ERC20 standard suggests that a transfer should return true on success, many tokens are non-compliant in this regard (including high profile, like USDT) . In that case, the .transfer() call here will revert even if the transfer is successful, because solidity will check that the RETURNDATASIZE matches the ERC20 interface.

Recommendation: Consider using OpenZeppelin’s SafeERC20

• incorrect revert msg and comment in Splitter.transferSplitAsset as the function is transfering ERC20 token and not ETH.
• All contracts use an unlocked pragma ^0.8.4.

• CoreCollection: onlyUnInitialized and tokenExists modifier are declared but never used.
• CoreCollection: the internal function _baseURI is declared but never used.
• CoreCollection.setStartingIndex() function can be priavte. (no need to be public)
• ERC721Payable: onlyVaultInitialized modifier is declared but never used.
• Splitter : attemptETHTransfer and amountFromPercent functions are declared but never used.
• • The unchecked block can be used for all the for loops. Consider adding an internal function _increment with an unchecked block and use it to increment the counter inside the for loop.