Giveth / minime

Minimi Token. ERC20 compatible clonable token
GNU General Public License v3.0
670 stars 289 forks source link

[Alert] Overflow in generateTokens(), doTransfer() and Reentrancy of Controller. #60

Closed leekt closed 3 years ago

leekt commented 4 years ago

I know that this contract seems to lost maintenance and there is no way to exploit the contract without permission. But since it has gained a lot of reputation about the special feature of "Checkpoint", some projects still seems to use this repository and keeps downloading. So I thought that it is better noticed about overflow exploits and Reentrancy Attacks in this minimetoken.

Overflow

generateTokens() https://github.com/Giveth/minime/blob/ea04d950eea153a04c51fa510b068b9dded390cb/contracts/MiniMeToken.sol#L376-L386 in generateTokens(), there is a require statement in line 379 to prevent overflow. unfortunately, curTotalSupply is declared as uint(which is uint256 as default), but ALL checkpoint related values are stored as uint128.

For exploit scenario, let assume totalSupply() is big enough (let's assume it's 2^128 - 1 for convenience) and wants to generate 10 more tokens. On require statement, lefthand side will be 2^128 + 9 since curTotalsupply and _amount is both capable of 256bit data and on righthand side will be 2^128 - 1.

But after updateValueAt(), totalSupply() will be just 9. Since stored value (curTotalSupply + _amount) is bigger than what Checkpoint can handle.

Same thing happens for doTransfer()

I wrote an simple testcase for generateTokens() overflow(not making a PR because i don't want to mess up your environment). https://github.com/leekt216/minime/blob/master/test/minimetoken_normal.js#L221-L236

And also, it should be noticed that since doTransfer() function calls onTransfer() of controller after loading sender balance and before loading receiver balance, it can be easily used for Reentrancy Attacks by Controller.

clesaege commented 4 years ago

The controller is a trusted smart contract. So this does not create an attack vector. The appropriate fix is just add a comment in the generateToken() function specifying that no more than 2^128 - 1 basic token units should ever be created.