search

RequestNetwork / RequestTokenSale

MIT License
64 stars 20 forks source link

Bug bounty: lack of short address attack protections #5

Closed makinj closed 7 years ago

makinj commented 7 years ago

https://blog.golemproject.net/how-to-find-10m-by-just-reading-blockchain-6ae9d39fcd95 This attack is relevant here. You should be checking the payload length so that an exchange who forgets to verify the length of addresses cannot be coerced into transferring too many tokens. The idea of the attack is that an attacker can generate wallet addresses until one ends with "00". They then tell an exchange, or some other application that transfers tokens to the user, to send some tokens to their address without the trailing "00". When the exchange sends the transfer call, the EVM will fill in the missing bytes in the address argument with the beginning bytes of the value argument and then shifts the next argument over by the same number of bytes. This is dangerous when the next argument is a value and the exchange can be tricked into transferring far more tokens than intended. This should be on the token's approve, transferFrom, and transfer functions.

vrolland commented 7 years ago

Hello Makinj, Thank you for your concern.

As described here "smart contracts are not the best place to check parameter sizes." And because Open zeppelin decided to remove it, we also do not apply it.

makinj commented 7 years ago

fair enough, just thought I'd give a heads up. I will say that I disagree though. While it could be fixed at many different levels, it's also within your power to fix it at your level.