code423n4 commented 3 years ago

Handle

JMukesh

Vulnerability details

Impact

lack of checking uniqueness of address in some of the function ,same address can be added in the list or same address can be approved

Proof of Concept

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/wallet/Zapper.sol#L73

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/wallet/Zapper.sol#L84

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

Tools Used

manual review

Recommended Mitigation Steps

check whether given array have unique element or not

Ivshti commented 3 years ago

There are no adverse effects from this as far as I can see - let me know if you can think of anything

GalloDaSballo commented 3 years ago

I can assume not checking for unique addresses can end up having certain calls cost more gas, and potentially, for very specific tokens, setting allowance from non-zero to non-zero will cause a revert

That said, the gas savings of avoiding duplicates are outweighed by the gas costs of running the checks on chain