code423n4 commented 2 years ago

Handle

JMukesh

Vulnerability details

Impact

Low-level calls call/delegatecall/staticcall return true even if the account called is non-existent. Account existence must be checked prior to calling.

see finding 01 in https://raw.githubusercontent.com/trailofbits/publications/master/reviews/hermez.pdf

Proof of Concept

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/Identity.sol#L74

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/Identity.sol#L127

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/IdentityFactory.sol#L30

Tools Used

manual review

Recommended Mitigation Steps

add condition to check the address

Ivshti commented 2 years ago

intended behavior, as the same methods could be used for sending ETH as well - in which case, if you send to an EOA, it should be a success

GalloDaSballo commented 2 years ago

I had to dig deep to understand what the warden was saying The finding on the hermez report is high severity because there was no check for transferring a token This potentially allowed the transferring of non-existing tokens, effectively rugging funds

In this context, the check is not necessary as the wallet is trying to mimick the behaviour of an EOA

I can send ETH or ERC to any address, they don't need to "exist" for me to transfer tokens

Invalid finding, but interesting perspective from the warden

code-423n4 / 2021-10-ambire-findings

No account existence check for low-level call #48