Skip to content

Commit

Permalink
refactor: improve internal functions of ERC725XCore (#162)
Browse files Browse the repository at this point in the history 
* refactor: re-write ERC725XCore internal functions

Co-authored-by: Yamen Merhi <yamennmerhi@gmail.com>

* refacotr: remove `gasleft()` in low level calls

* style: 🎨 apply prettier on `ERC725XCore`

* chore: add prettier configs for Solidity + reduce comment size

* docs: add suggested changes in Natspec comments

* refactor: move balance check at the top of the function

Co-authored-by: Yamen Merhi <yamennmerhi@gmail.com>
  • Loading branch information
@CJ42 @YamenMerhi
CJ42 and YamenMerhi committed Sep 2, 2022
1 parent e49bd45 commit a2a10b0
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 129 deletions.
8 changes: 8 additions & 0 deletions implementations/.prettierrc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@
"trailingComma": "all",
"singleQuote": true
}
},
{
"files": "*.sol",
"options": {
"tabWidth": 4,
"printWidth": 100,
"compiler": "0.8.10"
}
}
]
}
241 changes: 118 additions & 123 deletions implementations/contracts/ERC725XCore.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,184 +33,179 @@ import {
* This is the basis for a smart contract based account system, but could also be used as a proxy account system
*/
abstract contract ERC725XCore is OwnableUnset, ERC165, IERC725X {
/* Public functions */

/**
* @inheritdoc IERC725X
*/
function execute(
uint256 operation,
address to,
uint256 value,
bytes calldata data
) public payable virtual override onlyOwner returns (bytes memory result) {
uint256 txGas = gasleft();
bytes memory data
) public payable virtual override onlyOwner returns (bytes memory) {

require(address(this).balance >= value, "ERC725X: insufficient balance");

// CALL
if (operation == OPERATION_CALL) {
require(address(this).balance >= value, "ERC725X: insufficient balance for call");

result = executeCall(to, value, data, txGas);

emit Executed(operation, to, value, bytes4(data));

// STATICCALL
} else if (operation == OPERATION_STATICCALL) {
require(value == 0, "ERC725X: cannot transfer value with operation STATICCALL");

result = executeStaticCall(to, data, txGas);

emit Executed(operation, to, value, bytes4(data));

// DELEGATECALL
// WARNING!
// delegatecall is a dangerous operation type!
//
// delegate allows to call another deployed contract and use its functions
// to update the state of the current calling contract
//
// this can lead to unexpected behaviour on the contract storage, such as:
//
// - updating any state variables (even if these are protected)
// - update the contract owner
// - run selfdestruct in the context of this contract
//
// use with EXTRA CAUTION
} else if (operation == OPERATION_DELEGATECALL) {
require(value == 0, "ERC725X: cannot transfer value with operation DELEGATECALL");

result = executeDelegateCall(to, data, txGas);

emit Executed(operation, to, value, bytes4(data));

// CREATE
} else if (operation == OPERATION_CREATE) {
require(
to == address(0),
"ERC725X: CREATE operations require the receiver address to be empty"
);
require(address(this).balance >= value, "ERC725X: insufficient balance for call");

address contractAddress = performCreate(value, data);
result = abi.encodePacked(contractAddress);

emit ContractCreated(operation, contractAddress, value);

// CREATE2
} else if (operation == OPERATION_CREATE2) {
require(
to == address(0),
"ERC725X: CREATE operations require the receiver address to be empty"
);
require(address(this).balance >= value, "ERC725X: insufficient balance for call");

bytes32 salt = BytesLib.toBytes32(data, data.length - 32);
bytes memory bytecode = BytesLib.slice(data, 0, data.length - 32);

address contractAddress = Create2.deploy(value, salt, bytecode);
result = abi.encodePacked(contractAddress);

emit ContractCreated(operation, contractAddress, value);
} else {
revert("Wrong operation type");
}
if (operation == OPERATION_CALL) return _executeCall(to, value, data);

// Deploy with CREATE
if (operation == OPERATION_CREATE) return _deployCreate(to, value, data);

// Deploy with CREATE2
if (operation == OPERATION_CREATE2) return _deployCreate2(to, value, data);

// STATICCALL
if (operation == OPERATION_STATICCALL) return _executeStaticCall(to, value, data);

// DELEGATECALL
//
// WARNING! delegatecall is a dangerous operation type! use with EXTRA CAUTION
//
// delegate allows to call another deployed contract and use its functions
// to update the state of the current calling contract.
//
// this can lead to unexpected behaviour on the contract storage, such as:
// - updating any state variables (even if these are protected)
// - update the contract owner
// - run selfdestruct in the context of this contract
//
if (operation == OPERATION_DELEGATECALL) return _executeDelegateCall(to, value, data);

revert("ERC725X: Unknown operation type");
}

/* Overrides functions */

/**
* @inheritdoc ERC165
*/
function supportsInterface(bytes4 interfaceId)
public
view
virtual
override(IERC165, ERC165)
returns (bool)
{
return interfaceId == _INTERFACEID_ERC725X || super.supportsInterface(interfaceId);
}

/* Internal functions */

/**
* @dev perform call using operation 0
* Taken from GnosisSafe: https://github.com/gnosis/safe-contracts/blob/main/contracts/base/Executor.sol
*
* @param to The address on which call is executed
* @param value The value to be sent with the call
* @param data The data to be sent with the call
* @param txGas The amount of gas for performing call
* @return result The data from the call
*/
function executeCall(
function _executeCall(
address to,
uint256 value,
bytes memory data,
uint256 txGas
) internal returns (bytes memory result) {
// solhint-disable avoid-low-level-calls
(bool success, bytes memory returnData) = to.call{gas: txGas, value: value}(data);
bytes memory data
) internal virtual returns (bytes memory result) {

// solhint-disable avoid-low-level-calls
(bool success, bytes memory returnData) = to.call{value: value}(data);
result = Address.verifyCallResult(success, returnData, "ERC725X: Unknown Error");

emit Executed(OPERATION_CALL, to, value, bytes4(data));
}

/**
* @dev perform staticcall using operation 3
* @param to The address on which staticcall is executed
* @param data The data to be sent with the call
* @param txGas The amount of gas for performing staticcall
* @return result The data from the call
* @param value The value passed to the execute(...) function (MUST be 0)
* @param data The data to be sent with the staticcall
* @return result The data returned from the staticcall
*/
function executeStaticCall(
function _executeStaticCall(
address to,
bytes memory data,
uint256 txGas
) internal view returns (bytes memory result) {
(bool success, bytes memory returnData) = to.staticcall{gas: txGas}(data);
uint256 value,
bytes memory data
) internal virtual returns (bytes memory result) {
require(value == 0, "ERC725X: cannot transfer value with operation STATICCALL");

// solhint-disable avoid-low-level-calls
(bool success, bytes memory returnData) = to.staticcall(data);
result = Address.verifyCallResult(success, returnData, "ERC725X: Unknown Error");

emit Executed(OPERATION_STATICCALL, to, value, bytes4(data));
}

/**
* @dev perform delegatecall using operation 4
* Taken from GnosisSafe: https://github.com/gnosis/safe-contracts/blob/main/contracts/base/Executor.sol
*
* @param to The address on which delegatecall is executed
* @param data The data to be sent with the call
* @param txGas The amount of gas for performing delegatecall
* @return result The data from the call
* @param value The value passed to the execute(...) function (MUST be 0)
* @param data The data to be sent with the delegatecall
* @return result The data returned from the delegatecall
*/
function executeDelegateCall(
function _executeDelegateCall(
address to,
bytes memory data,
uint256 txGas
) internal returns (bytes memory result) {
// solhint-disable avoid-low-level-calls
(bool success, bytes memory returnData) = to.delegatecall{gas: txGas}(data);
uint256 value,
bytes memory data
) internal virtual returns (bytes memory result) {
require(value == 0, "ERC725X: cannot transfer value with operation DELEGATECALL");

// solhint-disable avoid-low-level-calls
(bool success, bytes memory returnData) = to.delegatecall(data);
result = Address.verifyCallResult(success, returnData, "ERC725X: Unknown Error");

emit Executed(OPERATION_DELEGATECALL, to, value, bytes4(data));
}

/**
* @dev perform contract creation using operation 1
* Taken from GnosisSafe: https://github.com/gnosis/safe-contracts/blob/main/contracts/libraries/CreateCall.sol
*
* @param to The recipient address passed to execute(...) (MUST be address(0) for CREATE)
* @param value The value to be sent to the contract created
* @param deploymentData The contract bytecode to deploy
* @return newContract The address of the contract created
* @param data The contract bytecode to deploy
* @return newContract The address of the contract created as bytes
*/
function performCreate(uint256 value, bytes memory deploymentData)
internal
returns (address newContract)
{
require(deploymentData.length != 0, "no contract bytecode provided");

function _deployCreate(
address to,
uint256 value,
bytes memory data
) internal virtual returns (bytes memory newContract) {
require(
to == address(0),
"ERC725X: CREATE operations require the receiver address to be empty"
);
require(data.length != 0, "ERC725X: No contract bytecode provided");

address contractAddress;
// solhint-disable no-inline-assembly
assembly {
newContract := create(value, add(deploymentData, 0x20), mload(deploymentData))
contractAddress := create(value, add(data, 0x20), mload(data))
}

require(newContract != address(0), "Could not deploy contract");
}
require(contractAddress != address(0), "ERC725X: Could not deploy contract");

/* Overrides functions */
newContract = abi.encodePacked(contractAddress);
emit ContractCreated(OPERATION_CREATE, contractAddress, value);
}

/**
* @inheritdoc ERC165
* @dev perform contract creation using operation 2
* @param to The recipient address passed to execute(...) (MUST be address(0) for CREATE2)
* @param value The value to be sent to the contract created
* @param data The contract bytecode to deploy appended with a bytes32 salt
* @return newContract The address of the contract created as bytes
*/
function supportsInterface(bytes4 interfaceId)
public
view
virtual
override(IERC165, ERC165)
returns (bool)
{
return interfaceId == _INTERFACEID_ERC725X || super.supportsInterface(interfaceId);
function _deployCreate2(
address to,
uint256 value,
bytes memory data
) internal virtual returns (bytes memory newContract) {
require(
to == address(0),
"ERC725X: CREATE operations require the receiver address to be empty"
);
require(data.length != 0, "ERC725X: No contract bytecode provided");

bytes32 salt = BytesLib.toBytes32(data, data.length - 32);
bytes memory bytecode = BytesLib.slice(data, 0, data.length - 32);
address contractAddress = Create2.deploy(value, salt, bytecode);

newContract = abi.encodePacked(contractAddress);
emit ContractCreated(OPERATION_CREATE2, contractAddress, value);
}
}
13 changes: 7 additions & 6 deletions implementations/test/ERC725X.behaviour.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ export const shouldBehaveLikeERC725X = (
context = await buildContext();
provider = ethers.provider;
abiCoder = new ethers.utils.AbiCoder();

// fund erc725X contract
valueToSend = ethers.utils.parseEther("50");
await context.erc725X
Expand Down Expand Up @@ -509,7 +510,7 @@ export const shouldBehaveLikeERC725X = (
txParams.value,
txParams.data
)
).to.be.revertedWith("ERC725X: insufficient balance for call");
).to.be.revertedWith("ERC725X: insufficient balance");
});
});
});
Expand Down Expand Up @@ -952,7 +953,7 @@ export const shouldBehaveLikeERC725X = (
txParams.value,
txParams.data
)
).to.be.revertedWith("Could not deploy contract");
).to.be.revertedWith("ERC725X: Could not deploy contract");
});
});

Expand Down Expand Up @@ -1051,7 +1052,7 @@ export const shouldBehaveLikeERC725X = (
txParams.value,
txParams.data
)
).to.be.revertedWith("ERC725X: insufficient balance for call");
).to.be.revertedWith("ERC725X: insufficient balance");
});
});

Expand All @@ -1073,7 +1074,7 @@ export const shouldBehaveLikeERC725X = (
txParams.value,
txParams.data
)
).to.be.revertedWith("Could not deploy contract");
).to.be.revertedWith("ERC725X: Could not deploy contract");
});
});
});
Expand Down Expand Up @@ -1405,7 +1406,7 @@ export const shouldBehaveLikeERC725X = (
txParams.value,
txParams.data
)
).to.be.revertedWith("ERC725X: insufficient balance for call");
).to.be.revertedWith("ERC725X: insufficient balance");
});
});

Expand Down Expand Up @@ -1917,7 +1918,7 @@ export const shouldBehaveLikeERC725X = (
txParams.value,
txParams.data
)
).to.be.revertedWith("Wrong operation type");
).to.be.revertedWith("ERC725X: Unknown operation type");
});
});
});
Expand Down

0 comments on commit a2a10b0

Please sign in to comment.