Is this contract correct?

Question

Hi i write simple contract to test. Does he have any known mistakes or bugs? What would you change in it for the better? Thanks for answer.

pragma solidity ^0.4.21;

contract Lock {
    uint256 public value;
    address public owner;
    uint256 public lockedUntil;

    modifier onlyOwner() {
        require(msg.sender == owner) ;
        _;
    }

    function construct() public payable  {
        value = msg.value;
        owner = msg.sender;
        lockedUntil = now + 5 days;
    }

    function withdraw() public onlyOwner {
        require(block.timestamp > lockedUntil);
        msg.sender.transfer(value);
    }
}

Answer 1

Your construct function is callable by everyone, which means anyone can take over your contract. The constructor function for your contract should have the same name as the contract, so the construct function should be named Lock.

The contract itself looks fine, but can only be used by one address, to lock up funds it seems. You could change this so that the contract can be used by everyone, and store everyone's lockedUntil and value variables into a mapping(address => uint256[2]) variable, where you can get these values from the array. (Or create a struct to handle this in a more elegant way).

Also add a functionality to let users decide their own lock up time?

Answer 2

The only problem with your contract is the constructor. You are using solidity 0.4.21, so your constructor should be a function with the same name of the Contract, so you should replace your construct() function with this:

function Lock() public payable  {
    value = msg.value;
    owner = msg.sender;
    lockedUntil = now + 5 days;
}

Starting in solidity 0.4.22, your constructor should be a function (but without the function word) called constructor:

constructor() public payable  {
    value = msg.value;
    owner = msg.sender;
    lockedUntil = now + 5 days;
}