[WP-H12] `CoreCollection.sol` The owner won't be able to withdraw the funds due to the wrong ERC20 method being used in `withdraw()`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/CoreCollection.sol#L173-L177

Vulnerability details

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/CoreCollection.sol#L173-L177

function withdraw() external onlyOwner {
    uint256 amount = payableToken.balanceOf(address(this));
    payableToken.transferFrom(address(this), msg.sender, amount);
    emit NewWithdrawal(msg.sender, amount);
}

Unlike ERC721, most ERC20 tokens require an allowance when calling transferFrom(), even when the from == msg.sedner.

Therefore, in the withdraw() function, it should be using payableToken.trasnfer(msg.sender, amount); instead of payableToken.transferFrom().

The current implementation will most certainly revert with the error: "ERC20: insufficient allowance" for almost all ERC20 tokens.

See: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L164

Recommendation

Change to: payableToken.trasnfer(msg.sender, amount);

[sofianeOuafir]

duplicate of #52

[deluca-mike]

Actually a duplicate of #80