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)

[MihanixA]

Thank you for your comment!
Though I think the only place we have this function call is redundant because we check if to address is a registered vault right after.
https://github.com/mellow-finance/mellow-vaults/blob/main/contracts/Vault.sol#L198-L214

[0xleastwood]

!gw.hasSubvault(to) is used to check if the to address is a registered vault.