smart-contract-security-examples
smart-contract-security-examples copied to clipboard
PeterBorah
Possible drain vector in TokenWithInvariants.sol?
I think 7d7ef27b12f15318871c44512b70737176d23c5f might have introduced an attack vector. Also not sure what is meant by "griefing" in that commit.
As I understand, the contract demonstrates how recursive calls to withdraw() and fraudulent calls to deposit(...) would be stopped by a balance check and a throw. I assume it's thought that this.balance >= totalSupply is an acceptable state. It could happen, say, if someone sent more ether than the number of tokens they claim with deposit(amount). Then anyone could withraw the excess, but not so much that the function modifier checkInvariants() would fail.
However, if it was possible to force the contract into thinking that totalSupply has reduced without a reduction in this.balance, then it would be possible to subsequently withdraw the excess without triggering the modifier.
Just learning Solidity ATM, so can't provide actual working exploit code - sorry about that. Consider this two-stage attack:
First, recursively call withdraw() from contract A that has a balance in TWI as balanceOf[A]. Proceed as in the known race-to-empty attack, but at the bottom call, return the stolen ether, and transfer owned tokens to B. (The latter is similar to what TheDAO exploit did.)
After this execution, this.balance of TWI won't have changed, totalSupply will be reduced, and token balance will be transferred from A to B. (Faux stack in next comment.)
Second, execute a "classic" race-to-empty from contract B, up to the same number of times as in the first case. (No faux stack.)
This allows for the withdrawal of up to half of the ether in TWI, and rendering it useless due to totalSupply being reduced to zero. That is, half the tokens are used to extact the ether, and half are burned.
Faux stack for first stage (EDIT: long lines, scroll right for comments if needed):
totalSupply this.balance balanceOf[A] balanceOf[B] contract: call
---------------------------------------------------------------------
100 100 10 0 A: TWI.withdraw()
100 100 10 0 TWI: balance = balanceOf[A];
100 100 10 0 TWI: A.call.value(balance)()
100 90 10 0 A: TWI.withdraw()
100 90 10 0 TWI: balance = balanceOf[A];
100 90 10 0 TWI: A.call.value(balance)()
100 80 10 0 A: TWI.call(20)("transfer", "B", "10") // 20 == initial balanceOf[A] * number of desired recursive calls, both known pre-execution
100 100 10 0 TWI: <transfer(B,10) body run> // 10 == initial balanceOf[A]
100 100 0 10 TWI: checkInvariants() // passes
100 100 0 10 A: return() // depth limit reached, mission accomplished
100 100 0 10 TWI: totalSupply -= balance;
90 100 0 10 TWI: balanceOf[A] = 0; // useless
90 100 0 10 TWI: checkInvariants() // passes
90 100 0 10 A: return() // not deep enough, didn't send anything back
90 100 0 10 TWI: totalSupply -= balance;
80 100 0 10 TWI: balanceOf[A] = 0; // useless
80 100 0 10 TWI: checkInvariants() // passes
80 100 0 10 A: return() // initial call
(EDIT: s/msg.sender/A/)
Sorry if this is an inappropriate venue for such guesses. Also if TWI.call(20)("transfer", "B", "10") is not actually possible - again, I'm still learning.
Yeah, I think this works. Well spotted!
To rephrase for my own benefit, and anyone else following along: If you return the ether stolen by a race-to-empty attack before any of the invariants are checked, then the totalSupply will be reduced by the amount you fraudulently withdraw, but the invariants will pass. Then you can perform a second race-to-empty, and the fraudulently lowered totalSupply will mean that the invariants don't see any problem with your large withdrawal.
The original motivation for loosening the invariants was to prevent people "donating" Ether to the contract, and thereby locking all actions.
With the current design of Ether, a possible solution would be to go back to checking strict equality, and reject any Ether transfer that isn't a deposit. This wouldn't work for other tokens, though, where you can't reject transfers. Alternatively, you can have your guards be more intelligent and specific: check that the totalSupply changes by the expected amount at the end of the function. (Function-specific guards aren't discussed in the article, but may be more generally useful than permanent invariants.)
This easily qualifies for the bounty mentioned in the article, so feel free to send me an Ethereum address.
Sent! (Since this was the first bug bounty claimed, and since it was a major issue and not just a typo, I sent a bit more than the nominal amount mentioned in the article.)
Ooh this is nice.
Token systems are in general not nearly 'paranoid' enough, it's cool to see this documented and fixed.
Since this issue seems to be getting a bit of attention, I want to make sure it's clear that this code was demonstrating a single security technique, and intentionally left in the race-to-empty vulnerability. Real code should avoid known attacks like race-to-empty, and should have more security checks than a single invariant. This was an excellent attack that shows the necessity of defense in depth!
Would like thoughts on checking the invariant as a precondition too. Would this fix it? I've been sitting on this and did I miss something?
modifier checkInvariants {
if (this.balance < totalSupply) throw;
_
if (this.balance < totalSupply) throw;
}
@ethers: I don't think it fixes it, unfortunately. The attacker can return the withdrawn Ether each time before calling the next withdrawal.
@PeterBorah I'm not so sure. I just recently wrote out a full worked example following @veox 's notes above and have run it successfully on a private node. There are two phases to the attack, one where you increase the difference between the contract's ether balance and totalSupply (transferring the tokens away at the last moment) and one where you perform the multiple withdraws.
In the first part of the attack the attacking contract gives back all its ether to TokenWithInvariants in the final transfer call but it's important (for @veox's attack) that it is only done at this point. The key part to their attack is that at the end this.balance is higher than totalSupply. If we simply gave back the value with each call to withdraw then we wouldn't be able to create this differential which is really important for the second part of the attack. I think @ethers' suggestion of putting the check before the _ in checkInvariants will actually do the trick. At least it does for my worked example but there may be some other way to use reentrancy to attack this modified version.
I have written up @veox's attack in a self-contained repo which is private for the moment. I've been wanting to ask the community whether it is okay to publish things like this. My feeling is that it's okay to talk about the generalities of attacks but not necessarily to give step-by-step instructions, which my repo absolutely does. (I'm new to Ethereum and needed to spell out every step for myself.)
If you're interested I could make you a collaborator on my repo so that you could have a look.
In the first part of the attack the attacking contract gives back all its ether to TokenWithInvariants in the final transfer call but it's important (for @veox's attack) that it is only done at this point.
@sseefried: Perhaps I'm missing something, but I don't think this is true. The key part of the attack is repeatedly invoking the decrease of totalSupply. That decrease (on line 29) is guaranteed to happen unless the transfer fails. In this phase of the attack, where the Ether ends up is irrelevant, except to the invariants.
In the second phase, it is of course necessary to keep the money, but at that point the totalSupply is lowered, and this can be done safely.
I'd be happy to take a look at your private repo. Personally, I see no problem in publishing code that illustrates certain attacks, especially against code that isn't actually being used. Hackers won't be discouraged by having to write their own code, and sharing knowledge and examples to test against is valuable.
@PeterBorah I was wrong and you were absolutely right. If you pass back the value on the recursive withdraw calls and then on the final transfer call you pass back the remaining value then you have successfully done the first phase of the attack. I implemented it in my repo and it worked like a charm. Thanks for letting me know. I've also given you collaborator access to my repo now.
@ethers: OT in relation to this issue, but that modifier currently won't work as expected for functions that return, so careful.
For an example see this article by Dennis Peterson (search for mutex).
EDIT: ah, I see you've added it, heh. :)
@veox Indeed, I submitted this PR before https://github.com/PeterBorah/smart-contract-security-examples/pull/5/files
For posterity's sake: the v0.4 changes in Solidity render the described attack impossible. Specifically, introduction of the built-in payable modifier.
A minimal straightforward update of TokenWithInvariants to Solidity v0.4 would be adding payable to deposit(), and leaving transfer() unpayable (by default). Therefore, one wouldn't be able to "return the stolen ether with transfer()", which was a key point.
In a way, this issue is OK to close. :)