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

WIP - Have Escrow forward all gas on withdraw. #1714

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions contracts/mocks/FallbackGasReporter.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
pragma solidity ^0.5.2;

contract FallbackGasReporter {
event RemainingGas(uint256 amount);

function () external payable {
emit RemainingGas(gasleft());
}
}
4 changes: 3 additions & 1 deletion contracts/payment/escrow/Escrow.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
pragma solidity ^0.5.2;

import "../../math/SafeMath.sol";
import "../../utils/Address.sol";
import "../../ownership/Secondary.sol";

/**
Expand All @@ -17,6 +18,7 @@ import "../../ownership/Secondary.sol";
*/
contract Escrow is Secondary {
using SafeMath for uint256;
using Address for address payable;

event Deposited(address indexed payee, uint256 weiAmount);
event Withdrawn(address indexed payee, uint256 weiAmount);
Expand Down Expand Up @@ -47,7 +49,7 @@ contract Escrow is Secondary {

_deposits[payee] = 0;

payee.transfer(payment);
payee.transferWithGas(payment);

emit Withdrawn(payee, payment);
}
Expand Down
5 changes: 4 additions & 1 deletion contracts/payment/escrow/RefundEscrow.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
pragma solidity ^0.5.2;

import "./ConditionalEscrow.sol";
import "../../utils/Address.sol";

/**
* @title RefundEscrow
Expand All @@ -14,6 +15,8 @@ import "./ConditionalEscrow.sol";
* RefundableCrowdsale contract for an example of RefundEscrow’s use.
*/
contract RefundEscrow is ConditionalEscrow {
using Address for address payable;

enum State { Active, Refunding, Closed }

event RefundsClosed();
Expand Down Expand Up @@ -79,7 +82,7 @@ contract RefundEscrow is ConditionalEscrow {
*/
function beneficiaryWithdraw() public {
require(_state == State.Closed);
_beneficiary.transfer(address(this).balance);
_beneficiary.transferWithGas(address(this).balance);
}

/**
Expand Down
5 changes: 5 additions & 0 deletions contracts/utils/Address.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,9 @@ library Address {
assembly { size := extcodesize(account) }
return size > 0;
}

function transferWithGas(address payable account, uint256 amount) internal {
(bool success,) = account.call.value(amount)(""); // solhint-disable-line avoid-call-value
require(success);
}
}
12 changes: 12 additions & 0 deletions test/payment/escrow/Escrow.behavior.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
const { balance, ether, expectEvent, shouldFail } = require('openzeppelin-test-helpers');

const FallbackGasReporter = artifacts.require('FallbackGasReporter');

function shouldBehaveLikeEscrow (primary, [payee1, payee2]) {
const amount = ether('42');

Expand Down Expand Up @@ -67,6 +69,16 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) {
await this.escrow.withdraw(payee1, { from: primary });
});

it('forwards all gas', async function () {
const gasReporter = await FallbackGasReporter.new();

const { tx } = await this.escrow.withdraw(gasReporter.address, { from: primary });
const event = await expectEvent.inTransaction(tx, FallbackGasReporter, 'RemainingGas');

// This doesn't really test that _all_ gas is forwarded, but it does test that `transfer` wasn't used
event.args.amount.should.be.bignumber.greaterThan('2300');
});

it('only the primary account can withdraw', async function () {
await shouldFail.reverting(this.escrow.withdraw(payee1, { from: payee1 }));
});
Expand Down