Unsafe usage of transfer and transferFrom functions

[code423n4]

Lines of code

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

Vulnerability details

Impact

Use transfer instead of safeTransfer - in CoreCollection you don't check if the transfer has ben successful or not, and in RoyaltyVault you assume it returns bool. It is more safe to use safeTransfer instead of using the transfer functions, because some transfer function implementations don't return a boolean, and safeTransfer checks that for you.

Tools Used

VS Code and Remix

Recommended Mitigation Steps

use the safeTransfer and safeTransferFrom functions instead of the transfer and transferFrom functions

[sofianeOuafir]

In my opinion, the severity level should be 3 (High Risk) instead of 2 (Med Risk) duplicate of #52

[deluca-mike]

Lacks nuance: it does not explain why this is specifically an issue in Joyn's contracts or provide any valid attack vectors.