search

code-423n4 / 2022-03-joyn-findings

4 stars 1 forks source link

Transfer return value is ignored #135

Closed deluca-mike closed 2 years ago

deluca-mike commented 2 years ago

Impact

Some ERC20 tokens, such as USDT, don't revert when transfer/transferFrom fails. The transfer return value has to be checked (as there are some other tokens that returns false instead revert). safeTransfer should be used instead of transfer

Proof of Concept

safeTransferFrom should be used instead of transferFrom on this line https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L175

Tools Used

Manual analysis

Recommended Mitigation Steps

Use safeTransfer instead of transfer or check the return value of transfer. The return value of transfer is checked properly in these locations https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L44 https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L52 https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L237

deluca-mike commented 2 years ago

Duplicate of #52

deluca-mike commented 2 years ago

Folded back in #45.

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