non-vulnerable contract in SB curated dataset

[sunbeomso]

The contract below is not vulnerable to reentrancy: https://github.com/smartbugs/smartbugs/blob/master/dataset/reentrancy/reentrancy_insecure.sol

/*
 * @source: https://consensys.github.io/smart-contract-best-practices/known_attacks/
 * @author: consensys
 * @vulnerable_at_lines: 17
 */

pragma solidity ^0.4.0;

contract Reentrancy_insecure {

    // INSECURE
    mapping (address => uint) private userBalances;

    function withdrawBalance() public {
        uint amountToWithdraw = userBalances[msg.sender];
        // <yes> <report> REENTRANCY
        (bool success, ) = msg.sender.call.value(amountToWithdraw)(""); // At this point, the caller's code is executed, and can call withdrawBalance again
        require(success);
        userBalances[msg.sender] = 0;
    }
}

Note that the condition \forall i. userBalances[i] = 0 holds over the entire contract. Therefore, even though the reentrancy itself is possible, no ethers can be stolen from the contract (i.e., no vulnerabilities exist).

To make this contract vulnerable, some functions should be added to make userBalances[i] > 0 for some i.

One example of potentially fixed versions can be (addToBalance function is added):

/*                                                                                                              
 * @source: https://consensys.github.io/smart-contract-best-practices/known_attacks/                            
 * @author: consensys                                                                                           
 * @vulnerable_at_lines: 17                                                                                     
 */                                                                                                             
                                                                                                                
pragma solidity ^0.4.0;                                                                                         
                                                                                                                
contract Reentrancy_insecure {                                                                                  
                                                                                                                
    // INSECURE                                                                                                 
    mapping (address => uint) private userBalances;                                                             
                                                                                                                
    function addToBalance(address _to) public payable {                                                         
        userBalances[_to] += msg.value;                                                                         
    }                                                                                                           
                                                                                                                
    function withdrawBalance() public {                                                                         
        uint amountToWithdraw = userBalances[msg.sender];                                                       
        // <yes> <report> REENTRANCY                                                                            
        (bool success, ) = msg.sender.call.value(amountToWithdraw)(""); // At this point, the caller's code is executed, and can call withdrawBalance again
        require(success);
        userBalances[msg.sender] = 0;                                                                           
    }   
}   

However, this fixed version can be seen as a semantically duplicated one, compared with the contract below: https://github.com/smartbugs/smartbugs/blob/master/dataset/reentrancy/etherbank.sol

In summary, I think reentrancy_insecure.sol should be removed from the SB curated dataset. Or, some meaningful modifications should be applied to avoid duplicates.

[jff]

Thanks @SunBeomSo

It's a valid point, but still, the problem here is that there's a call to an external function before updating the balance.

I'm in favour of changing this example as you suggest (i.e. adding the function addToBalance). What do you think @pedrocrvz @tdurieux ?