Create exploits for sb-curated reentrancy vulnerabilities

[sofiabobadilla]

Inspiration: https://coinsbench.com/the-correct-way-to-write-tests-for-your-smart-contracts-using-hardhat-and-ethers-js-reentrancy-5438006e90a0

[sofiabobadilla]

https://github.com/LearnWeb3DAO/Re-Entrancy
Another example

[sofiabobadilla]

FYI: console.log for solidity
https://www.alchemy.com/overviews/solidity-console-log

[sofiabobadilla]

I found the contract 0x627fa62ccbb1c1b04ffaecd72a53e37fc0e17839.sol to be not exploitable for reentrancy.

It does have a reentrancy vulnerability on line 94, but the function has a onlyOwner tag. Which means external contracts can not withdraw money and therefore it is not exploitable.

@mokita-j Could you confirm this analysis?

    function WithdrawToHolder(address _addr, uint _wei) 
    public
    onlyOwner
    payable
    {
        if(Holders[_addr]>0)
        {
            // <yes> <report> REENTRANCY
            if(_addr.call.value(_wei)())
            {
                Holders[_addr]-=_wei;
            }
        }
    }

This case is particularly interesting because this is the “duplicated” contract (both in reentrancy and unchecked_low_levels)

[mokita-j]

I found the contract 0x627fa62ccbb1c1b04ffaecd72a53e37fc0e17839.sol to be not exploitable for reentrancy.

It does have a reentrancy vulnerability on line 94, but the function has a onlyOwner tag. Which means external contracts can not withdraw money and therefore it is not exploitable.

@mokita-j Could you confirm this analysis?

@sofiabobadilla
Notice that the "constructor" of TokenBank is wrongly named initTokenBank.
Thus anyone can change the owner variable of TokenBank. However to exploit the contract we need to change the owner variable of Ownable.
Can you check if by changing TokenBank's owner, you also change Ownable's owner?

My bet is that Ownable's owner doesn't change, which makes the contract not exploitable.

[sofiabobadilla]

Contract etherbank.sol is label with reentrancy, but the methods for adding balance and withdraw do not have the modifier payable and therefore no correct function calls can be made. Thus, the contract is not exploitable.

It could be interesting though to see how tools can handle this error while creating a patch.

[sofiabobadilla]

I found the contract 0x627fa62ccbb1c1b04ffaecd72a53e37fc0e17839.sol to be not exploitable for reentrancy.
It does have a reentrancy vulnerability on line 94, but the function has a onlyOwner tag. Which means external contracts can not withdraw money and therefore it is not exploitable.
@mokita-j Could you confirm this analysis?

@sofiabobadilla Notice that the "constructor" of TokenBank is wrongly named initTokenBank. Thus anyone can change the owner variable of TokenBank. However to exploit the contract we need to change the owner variable of Ownable. Can you check if by changing TokenBank's owner, you also change Ownable's owner?

My bet is that Ownable's owner doesn't change, which makes the contract not exploitable.

By doing the experiment and after our discussion on Tuesday 13th, the contract is not exploitable since the variable owner that can be modified is not the same as the one used by the modifier which does not allow an external actor to perform an attack.
The hypothesis is also confirmed by looking at the history of the contract on Etherscan where a failed attempt is found.

[sofiabobadilla]

Contract etherbank.sol is label with reentrancy, but the methods for adding balance and withdraw do not have the modifier payable and therefore no correct function calls can be made. Thus, the contract is not exploitable.

It could be interesting though to see how tools can handle this error while creating a patch.

This can be fix, on the contract side, by using a different pragma version where modifiers was not require to send ether, but for the version that Hardhat can handle, this won't be possible. Then the contract falls under the category of contracts that are exploitable on version that are not compatible with the testing framework.

[sofiabobadilla]

A note for future reviews:
Contract modifier_reentrancy.sol follows a slightly different pattern.
This is the SWC reference for the contract: link

Basically, the reentrancy is performed through the modifier by having an initial call to airDrop() and then override supportsToken on the malicious contract to keep calling airDrop().

The test are different compare to the other patterns, here we test the balance of the hacker inside the victimbalance.

[sofiabobadilla]

reentrancy_cross_function.sol

Another not exploitable contract due to the lack of a deposit function where the user can have funds.
Since we are not aiming to change the source code, this contract will not have an exploit

[sofiabobadilla]

reentrancy_insecure.sol

similar to the previous one , does not have a deposito function so the vulnerable function (withdrawBalance) is not exploitable due to the design of the contract.

Major reason: is an example contract and therefore authors did not make it functional

[sofiabobadilla]

Total reentrancy contracts 31
Non exploitable 5
List of non exploitable contracts:

name	(reason)
0x627fa62ccbb1c1b04ffaecd72a53e37fc0e17839.sol	(has a OnlyOwner tag that can not be modified)
etherbank.sol	(pragma version)
reentrancy_cross_function.sol	(missing deposit function)
reentrancy_insecure.sol	(missing deposit function)
spank_chain_payment.sol	(exceeded time box for exploits)