dependablesystemslab / solidifi-benchmark Goto Github PK

Various of the reentrancy bugs injected in this dataset are also detectable as a different bug class. For example, several of the reentrancy bugs are detected by MAIAN, an analysis tool that does not attempt to identify reentrancy. This can be misleading when using this dataset for tool evaluation. Tools might detect certain bugs due to different reasons, complicating a comparison of the tools.

For example, the following pattern is injected into the reentrancy dataset:

address payable lastPlayer_re_ent9;
uint jackpot_re_ent9;
function buyTicket_re_ent9() public {
  (bool success,) = lastPlayer_re_ent9.call.value(jackpot_re_ent9)("");
  if (!success)
    revert();
  lastPlayer_re_ent9 = msg.sender;
  jackpot_re_ent9    = address(this).balance;
}

However, there is absolutely no need to perform reentrancy here. In the first invocation lastPlayer_re_ent9 is 0 initialized. So the external call will simply succeed without causing any reentrancy. However, also no ether will be transferred. Then lastPlayer_re_ent9 is set to the attacker and jackpot_re_ent9 is set to all the ether available. The next call to this function will transfer the total ether balance of the victim contract. But where is the reentrancy here? In theory the code can cause reentrancy, but it would not make sense as part of an attack.

In the inspection.py script it seems that for Manticore, 'Reachable ether leak to sender' is considered as a bug code for reentrancy (see here). Why was this done? In the example above Manticore, and other analysis tools like MAIAN or teEther, will identify a leaking Ether issue, not a reentrancy bug. In my opinion this is also the correct bug class for the code above.

I believe the following bug pattern has a similar issue, although here the attacker can actually utilize reentrancy to steal more than 1 ether.

bool not_called_re_ent27 = true;
function bug_re_ent27() public{
    require(not_called_re_ent27);
    if( ! (msg.sender.send(1 ether) ) ){
        revert();
    }
    not_called_re_ent27 = false;
}

In general it seems a bit problematic to inject bug patterns that can be identified as two different bug classes, as it complicates comparisons between tools. This might invalidate some of the results presented in the paper. I suggest to double check the results and maybe publish an addendum to the paper, if necessary.

Most of the injected bug patterns for reentrancy are not exploitable, or are even effectively dead code. This seems like a major drawback of this dataset when trying to evaluate reentrancy detection tools: the more precise a tool becomes, the worse it will perform on this dataset. In my opinion this is quite misleading.

The following pattern is injected into many of the contracts in the reentrancy category:

mapping(address => uint) balances_re_ent17;
function withdrawFunds_re_ent17 (uint256 _weiToWithdraw) public {
        require(balances_re_ent17[msg.sender] >= _weiToWithdraw);
        // limit the withdrawal
        (bool success,)=msg.sender.call.value(_weiToWithdraw)("");
        require(success);  //bug
        balances_re_ent17[msg.sender] -= _weiToWithdraw;
}

Now here balances_re_ent17 is 0 initialized as all datatypes in Solidity/Ethereum. There is no way to change the values in balances_re_ent17. As such, the only valid call that does pass the require statement in the beginning of withdrawFunds_re_ent17 is to pass _weiToWithdraw == 0 as a parameter. This will transfer 0 ether. So one can reenter as much as one likes by always transferring 0 ether and subtracting 0 from 0. Not very useful and definitely not exploitable.

The next reentrancy pattern is broken in two ways:

• Effectively dead code
• Uses transfer, where reentrancy is not possible.

// 0 initialized and no function to update the mapping
mapping(address => uint) redeemableEther_re_ent25;
function claimReward_re_ent25() public {        
      // can never be satisfied
      require(redeemableEther_re_ent25[msg.sender] > 0);
      // unreachable
      uint transferValue_re_ent25 = redeemableEther_re_ent25[msg.sender];
      // msg.sender.transfer does not allow for reentrancy due to gas limits
      msg.sender.transfer(transferValue_re_ent25);   //bug
      redeemableEther_re_ent25[msg.sender] = 0;
}

I suggest to put a big disclaimer somewhere that this dataset should not be used to evaluate reentrancy detection.

Hi, I have a question on validity of injected overflow bugs.

It seems that, some parts that are marked as injected overflow bugs are not actually bugs (i.e., they are safe).

Could you please confirm whether they are indeed bugs or not?

For example, in a code snippet

function bug_intou20(uint8 p_intou20) public{
    uint8 vundflw1=0;
    vundflw1 = vundflw1 + p_intou20;   // overflow bug
}

which comes from
https://github.com/DependableSystemsLab/SolidiFI-benchmark/blob/master/buggy_contracts/Overflow-Underflow/buggy_11.sol#L98

the expression vundflw1 + p_intou20 will not overflow because vulndflw1 is initialized as 0 and it is a local variable (hence effects by transactions will not be accumulated).

To introduce overflow bugs in the function bug_intou20, for example, vulndlfw1 should be initialized with non-zero values.