Transfer error can fail unnoticed

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/e42ac05b3b740292422a725e9b57687e62d32c67/contracts/lowLevelCallers/LowLevelERC1155Transfer.sol#L30-L34 https://github.com/code-423n4/2022-11-looksrare/blob/e42ac05b3b740292422a725e9b57687e62d32c67/contracts/lowLevelCallers/LowLevelERC1155Transfer.sol#L52-L56 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelERC721Transfer.sol#L27-L28 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelERC20Transfer.sol#L30-L37 https://github.com/code-423n4/2022-11-looksrare/blob/e42ac05b3b740292422a725e9b57687e62d32c67/contracts/lowLevelCallers/LowLevelERC20Transfer.sol#L51-L56

Vulnerability details

Impact

Quoting Solidity docs:

The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

If the address of the low level call is either wrong, or uninitialized (zero), or pointing to an old contract that was deleted, the low level call response status will be success/true, therefore the calling transaction can successfully complete while one or more internal transfers failed. As a result this offers a surface for loss and/or attack.

Proof of Concept

https://github.com/code-423n4/2022-11-looksrare/blob/e42ac05b3b740292422a725e9b57687e62d32c67/contracts/lowLevelCallers/LowLevelERC1155Transfer.sol#L30-L34 https://github.com/code-423n4/2022-11-looksrare/blob/e42ac05b3b740292422a725e9b57687e62d32c67/contracts/lowLevelCallers/LowLevelERC1155Transfer.sol#L52-L56 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelERC721Transfer.sol#L27-L28 https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelERC20Transfer.sol#L30-L37 https://github.com/code-423n4/2022-11-looksrare/blob/e42ac05b3b740292422a725e9b57687e62d32c67/contracts/lowLevelCallers/LowLevelERC20Transfer.sol#L51-L56

eventually delegatecall https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L88-L101

and approve https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelERC20Approve.sol#L25-L30

Tools Used

n/a

Recommended Mitigation Steps

In general apply the following pattern

(bool success, bytes memory resultData) = theAddress.call(callData);
if(!success) revert Failed();
if(resultData.length > 0) { // address must be a contract
    abi.decode(resultData, ...);
} else if(theAddress.code.length == 0) revert Failed(); // was success, but address is not a contract!

same concept should be applied here: eventually delegatecall https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L88-L101

and probably can be ignored here: and approve https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/lowLevelCallers/LowLevelERC20Approve.sol#L25-L30

[Picodes]

Quoting your quote of the solidity docs: 'Account existence must be checked prior to calling if needed.'. But why is it needed here ? A valid finding must demonstrate why we could reasonably expect that "the address of the low level call is either wrong, or uninitialized (zero), or pointing to an old contract that was deleted".

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid