search

code-423n4 / 2022-05-factorydao-findings

1 stars 1 forks source link

Upgraded Q -> M from 220 [1654424909577] #287

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Judge has assessed an item in Issue #220 as Medium risk. The relevant finding follows:

gititGoro commented 2 years ago

Item L2 from the original issue follows:

[L-02] isContract logic can be bypassed

The logic in isContract can be bypassed. It uses the same approached described in https://solidity-by-example.org/hacks/contract-size

Proof of Concept

The logic that can be bypassed is for isContract https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/VoterID.sol#L343

Recommended Mitigation Steps

One solution that works for now is to check tx.origin == msg.sender. This is true for EOAs and false for contracts, but this can change with later EIPs.

ksk2345 commented 2 years ago

M01 is wrongly upgraded from Low to Medium. This is a good find by the warden, but with the current codebase, I dont find any impact of this issue to the protocol. The isContract function is only used in function checkOnERC721Received() of voterID.sol contract, and if any user crafted or uses this to hack, then the user is only at loss of not receiving the ERC721.

gititGoro commented 2 years ago

You're correct. Actually, the only way this can go wrong is if the receiving wallet is misrepresented. There's no incentive for receiving address to pretend to not be a contract. That would just transfer the NFT without incident. Marking invalid.