IsContract Function Usage

[code423n4]

Handle

defsec

Vulnerability details

Impact

the isContract function that uses EXTCODESIZE was discovered to be hackable. The function will return false if it is invoked from a contract's constructor (because the contract has not been deployed yet).

The code should be used very carefully, if at all, to avoid security hacks such as:

https://www.reddit.com/r/ethereum/comments/916xni/how_to_pwn_fomo3d_a_beginners_guide (archive)

Proof of Concept

Navigate to "https://github.com/maple-labs/proxy-factory/blob/main/contracts/ProxyFactory.sol#L64" & "https://github.com/maple-labs/proxy-factory/blob/main/contracts/ProxyFactory.sol#L56" & "https://github.com/maple-labs/proxy-factory/blob/main/contracts/ProxyFactory.sol#L47" & "https://github.com/maple-labs/proxy-factory/blob/main/contracts/ProxyFactory.sol#L20"

The function is allowed with isContract modifier.
However this is vulnerable implementation.
The sample code can be seen below.

Tools Used

None

Recommended Mitigation Steps

If you want to make sure that an EOA is calling your contract, a simple way is require(msg.sender == tx.origin). However, preventing a contract is an anti-pattern with security and interoperability considerations. (ethereum/solidity#683 - https://ethereum.stackexchange.com/questions/1891/whats-the-difference-between-msg-sender-and-tx-origin)

[lucas-manuel]

We are aware of this, do not think its an issue, and wont address.

[deluca-mike]

Also, there are no mitigation steps, and the ones proposed are much worse than the "problem". The contract existence checks used are not there to blacklist EOAs, but actually to prevent low-level calls and delegatecalls to functions at addresses without contract code, which would result in erroneous successes.

[pauliax]

Agree with sponsors, the avoidance of EXTCODESIZE is a pretty well-known problem, but here I don't think it poses serious security issues, the warden hasn't provided any real attack path, only theoretical considerations. I am marking this issue as a non-critical recommendation.