Incorrect assertions: reentrancy

[mokita-j]

Why are these assertions with constants like 7 and 3?
If the attacker has deposited 5 ether. He should be able to collect 5 ether.
@sofiabobadilla Could you give me some insights on this question?

https://github.com/ASSERT-KTH/solidity-hack-labs/blob/b41877052e41ace88aaa21e72531c25de80fe900/smartbugs-curated/0.4.x/test/reentrancy/0x4320e6f8c05b27ab4707cd1f6d5ce6f3e4b3a5a1_test.js#L86

https://github.com/ASSERT-KTH/solidity-hack-labs/blob/b41877052e41ace88aaa21e72531c25de80fe900/smartbugs-curated/0.4.x/test/reentrancy/0x4320e6f8c05b27ab4707cd1f6d5ce6f3e4b3a5a1_test.js#L89

[sofiabobadilla]

Hi @mokita-j

In that case the function Collect asks you for an amount to collect, the oracle I follow is that in a normal behavior you should only get the amount you used as input for the function.

Considering what you wrote now, I understand that it can be the case that a reentrant called won't fail if the hacker has enough money. But in this case the second call should fail as the hacker had originally 5ether, so for the second call it will also need the state to not be updated, otherwise it will revert from lack of funds.

Is it more clear now?

[mokita-j]

In that case the function Collect asks you for an amount to collect, the oracle I follow is that in a normal behavior you should only get the amount you used as input for the function.

I have to disagree with you on this point. Here's why:

The assertions are supposed to detect exploitation caused by reentrancy, like an attacker withdrawing more ether than they deposited.
However, they currently check for reentrancy itself, not whether it leads to an exploited state.
This causes effective patches to be incorrectly flagged as ineffective. For example:

function Collect(uint _am) public payable {
    if (balances[msg.sender] >= MinSum && balances[msg.sender] >= _am) {
        balances[msg.sender] = balances[msg.sender] - _am; // <FIX> Move
        if (msg.sender.call.value(_am)()) {
            // <FIX> Move: balances[msg.sender] -= _am;
            Log.AddMessage(msg.sender, _am, "Collect");
        } else {
            revert(); // <FIX> Add revert
        }
    }
}

This patch prevents exploitation by updating the balance before the external call and adding a revert. Yet, the current assertions wrongly label it as exploitable.

This same assertion issue happens for the following exploits:

1. reentrancy/0x23a91059fdc9579a9fbd0edc5f2ea0bfdb70deb4_test.js
2. reentrancy/0x4320e6f8c05b27ab4707cd1f6d5ce6f3e4b3a5a1_test.js
3. reentrancy/0x561eac93c92360949ab1f1403323e6db345cbf31_test.js
4. reentrancy/0x7541b76cb60f4c60af330c208b0623b7f54bf615_test.js
5. reentrancy/0x7b368c4e805c3870b6c49a3f1f49f69af8662cf3_test.js
6. reentrancy/0x8c7777c45481dba411450c228cb692ac3d550344_test.js
7. reentrancy/0xaae1f51cf3339f18b6d3f3bdc75a5facd744b0b8_test.js
8. reentrancy/0xb93430ce38ac4a6bb47fb1fc085ea669353fd89e_test.js
9. reentrancy/0xbaf51e761510c1a11bf48dd87c0307ac8a8c8a4f_test.js
10. reentrancy/0xbe4041d55db380c5ae9d4a9b9703f1ed4e7e3888_test.js
11. reentrancy/0xcead721ef5b11f1a7b530171aab69b16c5e66b6e_test.js
12. reentrancy/0xf015c35649c82f5467c9c74b7f28ee67665aad68_test.js

@sofiabobadilla Does this explanation clarify what I mean?

[sofiabobadilla]

Hi Monica

The assertions are supposed to detect exploitation caused by reentrancy, like an attacker withdrawing more ether than they
deposited.

I understand your point, I'll correct the oracle for those cases. Good catch!

[mokita-j]

Ack. I created the PR with the fixes.
Let me know what you think.