Sparkn

Summary

Failed transfer with low level call could be overlooked

Vulnerability Details

Impact

The ProxyFactory.sol contract has the function _distribute():

This function is used in distributeByOwner(), deployProxyAndDistribute(), deployProxyAndDistributeBySignature(), deployProxyAndDistributeByOwner(), According to the Solidity documentation,

Reference 1:

"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."

Reference 2:

Warning: Due to the fact that the EVM considers a call to a non-existing contract to always succeed, Solidity
includes an extra check using the extcodesize opcode when performing external calls. This ensures that the
contract that is about to be called either actually exists (it contains code) or an exception is raised.
The low-level calls which operate on addresses rather than contract instances (i.e. .call(), .delegatecall(),
.staticcall(), .send() and .transfer()) do not include this check, which makes them cheaper in terms of
gas but also less safe.

As a result, it is possible that this call will not work but _distribute() will not notice anything went wrong. It could be possible that a proxy does not exist or proxy that has been deleted, but _distribute() will not notice that something has gone wrong and as a result the transaction will always return success. For this reason, it would be better to also check for the contract’s existence prior to executing _distribute().

A similar instance for low level delegatecall() can be checked here

Tools Used

Manual Review

Recommendations

check for the contract’s existence per solidity documentation.