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

Implementation of TimelockedEscrow, ConditionalTokenEscrow, RefundTokenEscrow & TimelockedTokenEscrow #1262

Merged
20 changes: 20 additions & 0 deletions contracts/mocks/ConditionalTokenEscrowMock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
pragma solidity ^0.4.24;

import "../payment/ConditionalTokenEscrow.sol";
import "../token/ERC20/ERC20.sol";


// mock class using ConditionalTokenEscrow
contract ConditionalTokenEscrowMock is ConditionalTokenEscrow {
mapping(address => bool) public allowed;

constructor (ERC20 _token) public TokenEscrow(_token) { }

function setAllowed(address _payee, bool _allowed) public {
allowed[_payee] = _allowed;
}

function withdrawalAllowed(address _payee) public view returns (bool) {
return allowed[_payee];
}
}
23 changes: 23 additions & 0 deletions contracts/payment/ConditionalTokenEscrow.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
pragma solidity ^0.4.24;

import "./TokenEscrow.sol";


/**
* @title ConditionalTokenEscrow
* @dev Base abstract escrow to only allow withdrawal of tokens
* if a condition is met.
*/
contract ConditionalTokenEscrow is TokenEscrow {
/**
* @dev Returns whether an address is allowed to withdraw their tokens.
* To be implemented by derived contracts.
* @param _payee The destination address of the tokens.
*/
function withdrawalAllowed(address _payee) public view returns (bool);

function withdraw(address _payee) public {
require(withdrawalAllowed(_payee));
super.withdraw(_payee);
}
}
77 changes: 77 additions & 0 deletions contracts/payment/RefundTokenEscrow.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
pragma solidity ^0.4.24;

import "./ConditionalTokenEscrow.sol";
import "../token/ERC20/ERC20.sol";


/**
* @title RefundTokenEscrow
* @dev Escrow that holds tokens for a beneficiary, deposited from multiple parties.
* The contract owner may close the deposit period, and allow for either withdrawal
* by the beneficiary, or refunds to the depositors.
*/
contract RefundTokenEscrow is ConditionalTokenEscrow {
enum State { Active, Refunding, Closed }

event Closed();
event RefundsEnabled();

State public state;
address public beneficiary;

/**
* @dev Constructor.
* @param _token Address of the ERC20 token that will be put in escrow.
* @param _beneficiary The beneficiary of the deposits.
*/
constructor(ERC20 _token, address _beneficiary) public TokenEscrow(_token) {
require(_beneficiary != address(0));
beneficiary = _beneficiary;
state = State.Active;
}

/**
* @dev Stores tokens that may later be refunded.
* @param _refundee The address tokens will be sent to if a refund occurs.
* @param _amount The amount of tokens to store.
*/
function deposit(address _refundee, uint256 _amount) public {
require(state == State.Active);
super.deposit(_refundee, _amount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to think this one a bit more thoroughly: as it is, the owner will make a payment (e.g. transfer approved tokens) to the beneficiary, unless a refund occurs, in which case they will be sent to the refundee. This looks a bit weird at first glance, since one would expect the refundee to be making the payment, not the owner.

For this to make sense, the refundee has probably previously transferred tokens to the owner (which is likely a contract: see RefundableCrowdsale), via approve and transferFrom. Maybe this would be better if we could have the payer approve the escrow directly? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me see if get your point.
As it is, in pseudocode, a simplified happy path would be as follows.
Note that owner always refers to the owner of escrow and Payer is the same as refundee

  1. Payer calls token.approve(owner, N) to allow owner transfer N tokens
  2. Payer tells the owner she wants to deposit N tokens to beneficiary
  3. owner calls token.transferFrom(payer, owner, N) to get hold of N tokens (transfer was approved in 1)
  4. owner approves escrow to move N tokens and calls escrow.deposit(beneficiary, N) to put N tokens in escrow
  5. escrow calls token.transferFrom(owner, escrow, N) to get hold of N tokens (transfer was approved in 4).
  6. Supposing all conditions are met, beneficiary tells the owner she wants to withdraw the tokens.
  7. owner calls escrow.withdraw(beneficiary)
  8. escrow calls token.transfer(beneficiary, N)

If I understand, you suggest to do something like this:

  1. Payer calls token.approve(escrow, N) to allow escrow transfer N tokens
  2. Payer tells the owner she wants to deposit N tokens to beneficiary
  3. owner calls escrow.deposit(beneficiary, N) to put N tokens in escrow
  4. escrow calls token.transferFrom(payer, escrow, N) to get hold of N tokens (transfer was approved in 1).
  5. Supposing all conditions are met, beneficiary tells the owner she wants to withdraw the tokens.
  6. owner calls escrow.withdraw(beneficiary)
  7. escrow calls token.transfer(beneficiary, N)

My concerns about this last flow:

  1. Payer calls token.approve(escrow, N) to allow escrow transfer N tokens

I'm not sure about this step in terms of 'payer experience'. First, this would force the payer to know the address of the escrow in addition to the address of the owner. Furthermore, it would also require the payer to exactly understand how the payment method works behind the scenes (otherwise, the question would be: who's escrow? Why do I have to allow him to take my tokens if I just wanted to deposit to beneficiary through owner?). Some abstraction from how the payment method is actually implemented might be better for certain payers.

  1. escrow calls token.transferFrom(payer, escrow, N) to get hold of N tokens (transfer was approved in 1).

How would escrow know the payer's address in this case? Should it be added as a param to the deposit function in step 3? In that case, we would be adding yet another difference between TokenEscrow and Escrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identifying potential differences between both escrows is exactly what I intend to do :)

I agree with your assessment however: exposing the escrow address, while bad, was not too bad, but having to tell the escrow who to transfer from (or simply using the refundee as this address) feels way off. I'm still not thrilled with two transfers taking place when only one is needed, but I guess it'll do for now, I can't think of a better way of doing this.

Note btw that, during step 4 in the first scenario, owner need not approve the escrow, since this can be done at construction time for an amount of MAX_UINT256.

}

/**
* @dev Allows for the beneficiary to withdraw their tokens, rejecting
* further deposits.
*/
function close() public onlyOwner {
require(state == State.Active);
state = State.Closed;
emit Closed();
}

/**
* @dev Allows for refunds to take place, rejecting further deposits.
*/
function enableRefunds() public onlyOwner {
require(state == State.Active);
state = State.Refunding;
emit RefundsEnabled();
}

/**
* @dev Withdraws the beneficiary's tokens.
*/
function beneficiaryWithdraw() public {
require(state == State.Closed);
uint256 amount = token.balanceOf(address(this));
token.safeTransfer(beneficiary, amount);
}

/**
* @dev Returns whether refundees can withdraw their deposits (be refunded).
*/
function withdrawalAllowed(address _payee) public view returns (bool) {
return state == State.Refunding;
}
}
32 changes: 32 additions & 0 deletions contracts/payment/TimelockedEscrow.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
pragma solidity ^0.4.24;

import "./ConditionalEscrow.sol";


/**
* @title TimelockedEscrow
* @dev Escrow that holds funds for given amount of time,
* preventing their withdrawal until the time has passed.
*/
contract TimelockedEscrow is ConditionalEscrow {
// In seconds since unix epoch
uint256 public releaseTime;
tinchoabbate marked this conversation as resolved.
Show resolved Hide resolved

/**
* @dev Constructor.
* @param _releaseTime Time when the funds will be available for withdrawal.
*/
constructor(uint256 _releaseTime) public {
// solium-disable-next-line security/no-block-members
require(_releaseTime > block.timestamp);
releaseTime = _releaseTime;
}

/**
* @dev Returns whether an address is allowed to withdraw their funds.
*/
function withdrawalAllowed(address _payee) public view returns (bool) {
// solium-disable-next-line security/no-block-members
return block.timestamp >= releaseTime;
}
}
34 changes: 34 additions & 0 deletions contracts/payment/TimelockedTokenEscrow.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
pragma solidity ^0.4.24;

import "./ConditionalTokenEscrow.sol";


/**
* @title TimelockedTokenEscrow
* @dev Escrow that holds tokens for given amount of time,
* preventing their whithdrawal until the time has passed.
*/
contract TimelockedTokenEscrow is ConditionalTokenEscrow {
// In seconds since unix epoch
uint256 public releaseTime;

/**
* @dev Constructor.
* @param _token Address of the ERC20 token that will be put in escrow.
* @param _releaseTime Time when the tokens will be available for withdrawal.
*/
constructor (ERC20 _token, uint256 _releaseTime) public TokenEscrow(_token) {
// solium-disable-next-line security/no-block-members
require(_releaseTime > block.timestamp);
releaseTime = _releaseTime;
}

/**
* @dev Returns whether an address is allowed to withdraw their funds.
*/
function withdrawalAllowed(address _payee) public view returns (bool) {
// solium-disable-next-line security/no-block-members
require(block.timestamp >= releaseTime);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to disallow payments after this time has elapsed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the one hand, if we do not disallow payments (i.e. leaving things as they are), once the time has elapsed, the escrow can at least continue working as a regular TokenEscrow. The owner must bare this in mind though. Should we go for this approach, we ought to explicitly describe this behavior somewhere in the contract.

On the other hand, if we do disallow payments, the contract would become useless once all tokens are withdrawn from the escrow.

Even though not entirely convinced yet, I'd go for the second option. It may be stricter, but I find it more predictable. If the owner wants a regular TokenEscrow after the time has elapsed, he/she should go ahead and deploy a new contract.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the contract be useless after the time has elapsed doesn't worry me too much right now: this is part of a bigger issue in the ecosystem, and we haven't yet taken a stance (e.g. providing self-destruct mechanisms).

I also like the second option more, though I guess it all depends on what this sort of contract would be used for. A time-locked payment doesn't make too much sense IMO without the ability to cancel, and in all cancellable scenarios I can think of, I don't see a reason why you'd want to pay after the time has expired.

return true;
}
}
55 changes: 55 additions & 0 deletions test/payment/ConditionalTokenEscrow.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
const { shouldBehaveLikeTokenEscrow } = require('./TokenEscrow.behavior');
const { expectThrow } = require('../helpers/expectThrow');
const { EVMRevert } = require('../helpers/EVMRevert');

const BigNumber = web3.BigNumber;

require('chai')
.use(require('chai-bignumber')(BigNumber))
.should();

const ConditionalTokenEscrowMock = artifacts.require('ConditionalTokenEscrowMock');
const StandardToken = artifacts.require('StandardTokenMock');

contract('ConditionalTokenEscrow', function ([_, owner, payee, ...otherAccounts]) {
const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000';
const MAX_UINT256 = new BigNumber(2).pow(256).minus(1);
nventuro marked this conversation as resolved.
Show resolved Hide resolved

it('reverts when deployed with a null token address', async function () {
await expectThrow(
ConditionalTokenEscrowMock.new(ZERO_ADDRESS, { from: owner }), EVMRevert
);
});

context('with token', function () {
beforeEach(async function () {
this.token = await StandardToken.new(owner, MAX_UINT256);
this.escrow = await ConditionalTokenEscrowMock.new(this.token.address, { from: owner });
});

context('when withdrawal is allowed', function () {
beforeEach(async function () {
await Promise.all(otherAccounts.map(
payee => this.escrow.setAllowed(payee, true))
);
});

shouldBehaveLikeTokenEscrow(owner, otherAccounts);
});

context('when withdrawal is disallowed', function () {
const amount = web3.toWei(23.0, 'ether');

beforeEach(async function () {
await this.token.approve(this.escrow.address, MAX_UINT256, { from: owner });
await this.escrow.setAllowed(payee, false);
});

it('reverts on withdrawals', async function () {
await this.escrow.deposit(payee, amount, { from: owner });

await expectThrow(this.escrow.withdraw(payee, { from: owner }), EVMRevert);
});
});
});
});
Loading