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

feat: add wrapper function for low level calls #2264

Merged
Merged
Show file tree
Hide file tree
Changes from 5 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
4 changes: 4 additions & 0 deletions contracts/mocks/AddressImpl.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ contract AddressImpl {
Address.sendValue(receiver, amount);
}

function functionCall(address target, bytes calldata data) external returns (bytes memory) {
return Address.functionCall(target, data);
nventuro marked this conversation as resolved.
Show resolved Hide resolved
}

// sendValue's tests require the contract to hold Ether
receive () external payable { }
}
16 changes: 16 additions & 0 deletions contracts/mocks/CallReceiverMock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.6.0;

contract CallReceiverMock {

event MockFunctionCalled();

function mockFunction() public {
emit MockFunctionCalled();
}

function mockFunctionReverts() public pure {
revert();
}
}
15 changes: 3 additions & 12 deletions contracts/token/ERC20/SafeERC20.sol
Original file line number Diff line number Diff line change
Expand Up @@ -56,19 +56,10 @@ library SafeERC20 {
*/
function _callOptionalReturn(IERC20 token, bytes memory data) private {
// We need to perform a low level call here, to bypass Solidity's return data size checking mechanism, since
// we're implementing it ourselves.

// A Solidity high level call has three parts:
// 1. The target address is checked to verify it contains contract code
// 2. The call itself is made, and success asserted
// 3. The return value is decoded, which in turn checks the size of the returned data.
// solhint-disable-next-line max-line-length
require(address(token).isContract(), "SafeERC20: call to non-contract");

// solhint-disable-next-line avoid-low-level-calls
(bool success, bytes memory returndata) = address(token).call(data);
require(success, "SafeERC20: low-level call failed");
// we're implementing it ourselves. We use {Address.functionCall} to perform this call, which verifies that
// the target address contains contract code and also asserts for success in the low-level call.

bytes memory returndata = address(token).functionCall(data, "SafeERC20: low-level call failed");
if (returndata.length > 0) { // Return data is optional
// solhint-disable-next-line max-line-length
require(abi.decode(returndata, (bool)), "SafeERC20: ERC20 operation did not succeed");
Expand Down
21 changes: 4 additions & 17 deletions contracts/token/ERC721/ERC721.sol
Original file line number Diff line number Diff line change
Expand Up @@ -497,28 +497,15 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
if (!to.isContract()) {
return true;
}
// solhint-disable-next-line avoid-low-level-calls
(bool success, bytes memory returndata) = to.call(abi.encodeWithSelector(
bytes memory returndata = to.functionCall(abi.encodeWithSelector(
IERC721Receiver(to).onERC721Received.selector,
_msgSender(),
from,
tokenId,
_data
));
if (!success) {
if (returndata.length > 0) {
// solhint-disable-next-line no-inline-assembly
assembly {
let returndata_size := mload(returndata)
revert(add(32, returndata), returndata_size)
}
} else {
revert("ERC721: transfer to non ERC721Receiver implementer");
}
} else {
bytes4 retval = abi.decode(returndata, (bytes4));
return (retval == _ERC721_RECEIVED);
}
), "ERC721: transfer to non ERC721Receiver implementer");
bytes4 retval = abi.decode(returndata, (bytes4));
return (retval == _ERC721_RECEIVED);
}

function _approve(address to, uint256 tokenId) private {
Expand Down
38 changes: 38 additions & 0 deletions contracts/utils/Address.sol
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,42 @@ library Address {
(bool success, ) = recipient.call{ value: amount }("");
require(success, "Address: unable to send value, recipient may have reverted");
}

/**
* @dev Replacement for Solidity's low-level `call`: performs a low-level
julianmrodri marked this conversation as resolved.
Show resolved Hide resolved
* call with `data` to the target address `target`. Returns the `returndata`
* provided by the low-level call.
*
* The call is not executed if the target address is not a contract.
*/
function functionCall(address target, bytes memory data) internal returns (bytes memory) {
return functionCall(target, data, "Address: low-level call failed");
}

/**
* @dev Replacement for Solidity's low-level `call`: performs a low-level
* call with `data` to the target address `target`. Returns the `returndata`
* provided by the low-level call. Uses `errorMessage` as default revert message.

* The call is not executed if the target address is not a contract.
nventuro marked this conversation as resolved.
Show resolved Hide resolved
*/
function functionCall(address target, bytes memory data, string memory errorMessage) internal returns (bytes memory) {
require(isContract(target), "Address: call to non-contract");

// solhint-disable-next-line avoid-low-level-calls
(bool success, bytes memory returndata) = target.call(data);
if (!success) {
nventuro marked this conversation as resolved.
Show resolved Hide resolved
if (returndata.length > 0) {
// solhint-disable-next-line no-inline-assembly
assembly {
let returndata_size := mload(returndata)
revert(add(32, returndata), returndata_size)
}
} else {
revert(errorMessage);
}
} else {
return returndata;
}
}
}
2 changes: 1 addition & 1 deletion test/token/ERC20/SafeERC20.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe('SafeERC20', function () {
this.wrapper = await SafeERC20Wrapper.new(hasNoCode);
});

shouldRevertOnAllCalls('SafeERC20: call to non-contract');
shouldRevertOnAllCalls('Address: call to non-contract');
nventuro marked this conversation as resolved.
Show resolved Hide resolved
});

describe('with token that returns false on all calls', function () {
Expand Down
59 changes: 57 additions & 2 deletions test/utils/Address.test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
const { accounts, contract } = require('@openzeppelin/test-environment');
const { accounts, contract, web3 } = require('@openzeppelin/test-environment');

const { balance, ether, expectRevert, send } = require('@openzeppelin/test-helpers');
const { balance, ether, expectRevert, send, expectEvent } = require('@openzeppelin/test-helpers');
const { expect } = require('chai');

const AddressImpl = contract.fromArtifact('AddressImpl');
const EtherReceiver = contract.fromArtifact('EtherReceiverMock');
const CallReceiver = contract.fromArtifact('CallReceiverMock');

describe('Address', function () {
const [ recipient, other ] = accounts;
Expand Down Expand Up @@ -90,4 +91,58 @@ describe('Address', function () {
});
});
});

describe('functionCall', function () {
beforeEach(async function () {
this.contractRecipient = await CallReceiver.new();
});

context('with valid contract receiver', function () {
it('calls the requested function', async function () {
const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
name: 'mockFunction',
type: 'function',
inputs: [],
}, []);
const { tx } = await this.mock.functionCall(this.contractRecipient.address, abiEncodedCall);
await expectEvent.inTransaction(tx, CallReceiver, 'MockFunctionCalled');
});

it('reverts when the called function reverts', async function () {
const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
name: 'mockFunctionReverts',
type: 'function',
inputs: [],
}, []);
await expectRevert(
this.mock.functionCall(this.contractRecipient.address, abiEncodedCall),
'Address: low-level call failed'
);
});

it('reverts when function does not exist', async function () {
const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
name: 'mockFunctionDoesNotExist',
type: 'function',
inputs: [],
}, []);
await expectRevert(
this.mock.functionCall(this.contractRecipient.address, abiEncodedCall),
'Address: low-level call failed'
);
});
});

context('with non-contract receiver', function () {
it('reverts when address is not a contract', async function () {
const [ recipient ] = accounts;
const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
name: 'mockFunction',
type: 'function',
inputs: [],
}, []);
await expectRevert(this.mock.functionCall(recipient, abiEncodedCall), 'Address: call to non-contract');
});
});
});
});