-1

I'm so happy I have finally written something useful(and most likely shitty).

  • It's an auction contract that lasts 10 days.
  • Minimum deposit is 1 ether.
  • Minumim outbid amount is also 1 ether.
  • Bidders can withdraw their ether any time they want
  • After the auction ends the winner claims the prize(haven't figured out yet what's it going to be)

You all guys are the best people on Earth! You've helped me so much here.The Ethereum community is one of the few places I feel I belong to. If you have any thoughts or ideas on improving security or other contract-design issues please share!!!

contract Auction {
    uint public startTime;
    uint public endTime;
    uint public highestBid;
    address highestBidder;
    mapping (address => uint) deposits;
    uint constant minVal = 1000000000000000000; // 1 ether

    modifier depositCheck {
        if(msg.value < minVal || msg.value % minVal != 0 || now > endTime){
            throw;
        }
        _
    }       
    modifier bidCheck(uint bid) {
        if(bid <= deposits[msg.sender] && bid % minVal == 0 && bid > highestBid && now < endTime) {
            _
        }
        else {
            throw;
        }
    }       
    modifier withdrawCheck {
        if(msg.sender == highestBidder) {
            throw;
        }
        else {
            _
        }
    }

    function Auction(uint duration){
        startTime = now;
        endTime = now + 864000000; // ten days
    }       

    function deposit() depositCheck {
        if(deposits[msg.sender] == 0) {
            deposits[msg.sender] = msg.value;
        }
        else {
            deposits[msg.sender] += msg.value;
        }
    }    
    function bid(uint bid) bidCheck(bid) {
        highestBid = bid;
        highestBidder = msg.sender;
    }       
    function withdraw() withdrawCheck {
        uint refund = deposits[msg.sender];
        deposits[msg.sender] = 0;
        if (!msg.sender.send(refund)) {
            deposits[msg.sender] = refund;
        }
    }
    function claimPrize() {
        if(now > endTime && msg.sender == highestBidder) {
            //Do something here...
        }
    }
}

closed as off-topic by Richard Horrocks, niksmac, Dawny33, Mikko Ohtamaa, Paul S Aug 21 '16 at 16:15

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "You should only ask practical, answerable questions based on actual problems that you face. Chatty, open-ended questions diminish the usefulness of our site and push other questions off the front page." – Richard Horrocks, niksmac, Dawny33, Mikko Ohtamaa, Paul S
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    You're understandably happy to have completed your contract, but by providing an audit we'd be setting a precedent that could open the floodgates to similar requests, and it doesn't really fit well with the format of this particular community. Perhaps one of the Reddit groups would be a better place? :) – Richard Horrocks Aug 20 '16 at 18:30
  • Check out: Code Review is a question and answer site for seeking peer review of your code. – Waqar Lim Aug 22 '16 at 8:33
2

A couple of things that I noticed:

  • The constructor takes a parameter duration, but it is unused.

  • endTime = now + 864000000; // ten days

    864000000 seconds is not 10 but 10000 days. To avoid mistakes like that Solidity supports a couple of units, including days and ether (which you might want to use at the initialization of minVal).

  • depositCheck, bidCheck, and withdrawCheck should work fine, but although they do similar things (checking a condition and throwing if it isn't met), they appear in three different forms. This is confusing and invites errors, e.g., in signs.

  • deposit can be simplified to one line (deposits[msg.sender] += msg.value)

  • There is no way of checking if a bid or deposit was successful or not. I suggest adding events such as Bid, Deposit, PrizeClaimed as well as making highestBid and deposits public.

  • As far as I can see bidding and depositing could be performed in one function. This reduces complexity not only in the contract code itself, but also for the user: She only needs to send one transaction, not two.

  • Depending on what happens in claimPrize there could be a security risk: The contract cannot suicide here as then the deposits of beaten bidders would be lost, so the winner can call the function twice. If this is problematic, a prizeClaimed flag could be introduced which is set and checked claimPrize.

As a side note, I believe the best way to convince yourself and the community that a contract really does what it should is writing extensive, readable unit tests.

  • good sir/lady, may the force of blockchain be always with you! The review is truly invaluable! Thank you very much! One quick question though. You wrote about checks in modifiers: "... invites errors, e.g. in signs". What are signs? Would you be so kind as to give a little example on what you meant there? I'm sure as hell I don't want to invite any errors. – manidos Aug 21 '16 at 14:19
  • 1
    I meant mathematical signs, as in positive/negative numbers or in this case true and false (I agree, probably not the correct term there). So in depositCheck you have to write now > endTime, but in bidCheck now < endTime. Since you basically check for the same thing it would be "logical" to write < in both clauses. This would be a mistake though, because one time you throw if the condition is met, another time if it is not met. – ComandanteCheth Aug 21 '16 at 16:16

Not the answer you're looking for? Browse other questions tagged or ask your own question.