8

If my contract has a payout function like this

address bossAddress;
address employeeAddress;
uint256 bossSalary;
uint256 employeeSalary;
function payout (){
    if (msg.sender==bossAddress){
            employeeAddress.send(employeeSalary);
            bossAddress.send(bossSalary);
            selfdestruct(bossAddress);
    }
}

Then, if I were a malicious boss, could I have exploited the payout such that only the boss gets paid?

A) If the employeeAddress is a contract, but bossAddress is a regular address, can I exploit the callstack attack so that the transaction fails on employeeAddress.send(employeeSalary), but continues to run bossAddress.send(bossSalary)? I guess what I'm asking is if both send calls reduces the callstack count, or just the send call that involves a contract address.

B) On the other hand, if the employee address is a contract that uses a lot of gas, I probably won't need to do any exploit, since the employeeAddress.send will fail from out of gass, then the boss should get paid both employeeSalary AND bossSalary, thanks to the selfdestruct() call.

C) And as a last point, is there a way to specify gas on the send call? This should prevent most out-of-gass errors in the case employeeAddress is actually a contract.

I just need to verify that my understanding of gas / callstack behavior is correct.

eth
  • 85,679
  • 53
  • 285
  • 406
shiso
  • 1,036
  • 1
  • 10
  • 12

1 Answers1

10

Call depth attack

A. No. If the call depth is at 1024, employeeAddress.send will fail. The depth remains at 1024 and bossAddress.send will also fail. The depth will only decrease to 1023 when payout (or its caller, depending on how payout is invoked) is finished.

B. Yes, that all sounds correct and no need for any call depth manipulation.

C. There is no way to specify gas for Solidity .send() and it forwards zero gas to the recipient. It is the responsibility of recipients of send() to do nothing/little in their fallback function if they want to avoid Out of Gas and receive Ether.


Reentrant attack

Solidity's send forwards zero gas so it can't be used for a reentrant attack.

If the contract has at least twice the bossSalary (example: bossSalary and employeeSalary are the same), and the boss is paid first using .call, then a reentrant attack is possible:

function payout (){
    if (msg.sender==bossAddress){
            bossAddress.call.value(bossSalary)();
            employeeAddress.call.value(employeeSalary)();
            selfdestruct(bossAddress);
    }
}

bossAddress could have a fallback function to invoke payout a second time before the employee is paid. Since none of the return values of .call are checked, employeeAddress.call will fail but there's no throw so the boss gets to keep both salaries.

The return value of all .send and .call (and related .callcode, .delegatecall) should be checked and handled carefully.

eth
  • 85,679
  • 53
  • 285
  • 406
  • Thank you for providing the additional call example. In the case of using call, it is possible then to specify a gas amount. However, if the bossAddress has a reentrant call to payout, it seems like the first call will still return true. Come to think of it, the recursive calls will eventually run out of gas, so is it true that all recursive calls gets rolled back if we simply checked for true in bossAddress.call()? Or does the bossAddress gets paid however many times before the gas runs out? – shiso Jun 20 '16 at 13:25
  • You need to do if (!address.call.gas(0).value(...)(...)) throw; to make sure the transaction gets rolled back. – eth Jun 20 '16 at 17:52
  • 1
    Send() actually forwards 2300 gas, but that's still not enough for a reentrant attack. It's enough so the recipient can log the transaction though. – Dennis Peterson Jun 20 '16 at 18:16
  • @DennisPeterson Upvoted since that's the effect. (But technically send() forwards 0 gas and the 2300 gas is a stipend: last point of https://github.com/ethereum/wiki/wiki/Subtleties) – eth Jun 20 '16 at 18:19
  • Interesting, I didn't know that. I was just thinking of the practical effect in solidity code, I need to get deeper into the EVM. ....Btw, is the disclaimer at the top of that page, about it being out of date in some ways, still true? It would probably help people write secure contracts if docs on subtleties about ether transfers were up to date. – Dennis Peterson Jun 20 '16 at 18:27
  • Unfortunately still out of date and it seems the people who know exactly haven't had time to update it. – eth Jun 20 '16 at 18:34
  • 1
    By the way, the last point says that 2300 is added on top of the gas supplied by the calling account. If it's true, calling for example address.call.gas(2300).value()() should assign 4600 gas to be spent by the recipient fallback function. – Giuseppe Bertone Jun 21 '16 at 00:35
  • I can't help but notice in the subtleties page that "If a transaction triggers an exception, then: the value transfer from sender to recipient still takes place" so does using the if... throw construct roll back the value transfer on exception, or no? Thank you for your comments. – shiso Jun 21 '16 at 13:20
  • @GiuseppeBertone Thanks, you're right! Upvoted and I updated the comment above. – eth Jun 23 '16 at 09:45
  • @shiso Take note of the all CAPS at the top of the subtleties page. Yes, throw does roll back the value. – eth Jun 23 '16 at 09:49