Is this timelock escrow smart contract safe?

Question

I am making a time-based escrow smart contract in which anyone can put in money, and only take that money out after a minute has passed. They can also increase the amount of money that is held in escrow or the time that it is locked for. I am not sure, however, about the security of this contract. It is based on code from here.

pragma solidity ^0.8.0;


// overflow and underflow examples and preventions
// one can deposit ether into this contract but you must wait 1 week before you can withdraw your funds

 
// use safe math to prevent underflow and overflow
import "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/math/SafeMath.sol";
 

contract Timelock {
    

    // calling SafeMath will add extra functions to the uint data type
    using SafeMath for uint; // you can make a call like myUint.add(123)
    

    // amount of ether you deposited is saved in balances
    mapping(address => uint) public balances;
  

    // when you can withdraw is saved in lockTime
    mapping(address => uint) public lockTime;
   

    

    function deposit() external payable {
        // if you have already deposited you should not be able to deposit
        require(balances[msg.sender] == 0, "there must be no existing money in deposit");
        //update balance
        balances[msg.sender] = msg.value;

        //updates locktime 1 minute from now
        lockTime[msg.sender] = block.timestamp + 1 minutes;

    }

    function addMoney() external payable {
        require(balances[msg.sender] != 0, "there must be existing money");
        balances[msg.sender]+=msg.value;
    }

       

    // the function that is commented out is vulnerable to overflow by updating the function below with a very large number
    // to prevent this use safe math to prevent overflow
    // function increaseLockTime(uint _secondsToIncrease) public {
    //     lockTime[msg.sender] += _secondsToIncrease;
    // }

     

    function increaseLockTime(uint _secondsToIncrease) public {

        // the add function below is from safemath and will take care of uint overflow
        // if a call to add causes an error an error will be thrown and the call to the function will fail
         lockTime[msg.sender] = lockTime[msg.sender].add(_secondsToIncrease);

    }

      

    function withdraw() public {

        // check that the sender has ether deposited in this contract in the mapping and the balance is >0
        require(balances[msg.sender] > 0, "insufficient funds");

        // check that the now time is > the time saved in the lock time mapping
        require(block.timestamp > lockTime[msg.sender], "lock time has not expired");
      

        // update the balance
        uint amount = balances[msg.sender];
        balances[msg.sender] = 0;

       
        // send the ether back to the sender
        (bool sent, ) = msg.sender.call{value: amount}("");
        require(sent, "Failed to send ether");

    }
}

Answer 1

It looks safe, yes, but it's messy.

No need to import safeMath since you use a solidity version that's > to 0.8, since safeMath is included in 0.8 and up.

Also, addMoney and deposit could be merged in one function that sets the timelock only if it's the first deposit.

Are you gonna use this for production? You need to write tests using foundry to be sure there isn't a bug.

Hope this helps