Is it okay to define the following smart contract that a accepts funds from two parties?

Question

I want to deploy a smart contract that receive funds from two parties. The contract is initialized with some ethers from person (call it revealer), and another Recipient (name it fineRecipient) who should aslo deposit an amount of ethers. If the revealer call the contract with the correct preimage of a certain hash value, he gets his funds back and also the Recipient deposit. Otherwise, the Recipient can get all funds in the contract. Is the following contract is okay from security point of view ? or does it needs any modifications before testing it in real production environment?

pragma solidity 0.5.2;

contract TimedCommitment {
    address payable revealer;
    address payable fineRecipient;
    bytes32 public hash;
    uint256 public deadline;

    constructor(address payable _fineRecipient, bytes32 _hash, uint256 timeout) public payable {
        revealer = msg.sender;
        fineRecipient = _fineRecipient;
        hash = _hash;
        deadline = now + timeout;
    }

  function  deposit(uint256 amount) payable public{
         require(msg.value == amount);
         require(msg.value == fineRecipient);

    }

    // If this is called with the correct preimage, the revealer gets their funds back.
    function providePreimage(bytes calldata preimage) external {
        require(keccak256(preimage) == hash);
        revealer.transfer(address(this).balance);
       revealer.transfer(address(this).amount);
    }

    // Pay the fine to the fineRecipient if the timeout has expired.
    function refund() external {
        require(msg.sender == fineRecipient);
        require(now >= deadline);

        msg.sender.transfer(address(this).balance);
    }
}

This contract is edited from this nice answer :timed commitment using ethereum blockchain

Answer 1

In function providePreimage(), you are trying to transfer the ether twice. This will cause a revert. As a result, revealer will never be able to provide preimage and transfer the ether back to itself. The timeout will eventually be reached and fineRecipient will be able to transfer funds to itself using refund().

To fix the issue, change providePreimage() function to

function providePreimage(bytes calldata preimage) external {
    require(keccak256(preimage) == hash);
    revealer.transfer(address(this).balance);
}

As an added measure of security, you can add a condition that fineRecipient must deposit their share at least X time before timeout is reached so that revealer has enough time to reveal the preimage. If fineRecipient fails to do so, the refund() function should refund the revealer and not fineRecipient.