Consensys / Tokens

Ethereum Token Contracts
MIT License
2.06k stars 1.19k forks source link

Fix #119: verifyEIP20 broken in EIP20Factory.sol #124

Closed skmgoldin closed 6 years ago

skmgoldin commented 6 years ago

The verifyEIP20 function does two checks to assert whether the code at a given address is known EIP20 code. In the first check, it simply compares the length (in bytes) of the code at the provided address to the length of the known EIP20 code. If that check passes, it would then iterate though the code at the provided address byte by byte and compare each byte to the known EIP20 code, returning false if any byte did not match.

In a run of the linter initially committed at a7837478bf60c2ab4332442e4849f2851fc51a31, the return true statement which had come after the byte-comparing for loop was moved inside the for loop such that if the first byte compared was matching, this function would return true even if the rest of the bytes did not match.

This PR moves verifyEIP20's return true statement to after both checks have completed.

GNSPS commented 6 years ago

How do you feel about changing this to use bytes.equal() instead? (v. https://github.com/GNSPS/solidity-bytes-utils/blob/master/contracts/BytesLib.sol#L292)

I can see how creating a dependency to this contract may not be optimal but what about copy/inlining the function from the bytes lib (perhaps with a link to the tests of that function in the original repo)?

skmgoldin commented 6 years ago

I would be fully willing to entertain that idea in a separate discussion. But I want to take this bug out of production as fast as possible, and it will take me time to understand that BytesLib code.

maurelian commented 6 years ago

LGTM.