Skip to content

Commit

Permalink
chore: [C4] Add first batch of Gas optimization (#695)
Browse files Browse the repository at this point in the history 
* chore: [C4] Add first batch of Gas optimization

* chore: replace `= 0` assignment in iterators of LSP7/8 Core

* chore: add more gas optimizations

* docs: generate docs

* chore: run prettier

* docs: add latest docs

* chore: resolve failing tesst

* chore: suggested changes

---------

Co-authored-by: CJ42 <[email protected]>
  • Loading branch information
@YamenMerhi @CJ42
YamenMerhi and CJ42 authored Sep 27, 2023
1 parent 40d3bb5 commit f1686d5
Show file tree
Hide file tree
Showing 16 changed files with 127 additions and 213 deletions.
8 changes: 4 additions & 4 deletions contracts/LSP0ERC725Account/LSP0ERC725AccountCore.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ abstract contract LSP0ERC725AccountCore is

if (!success) {
// Look for revert reason and bubble it up if present
if (result.length > 0) {
if (result.length != 0) {
// The easiest way to bubble the revert reason is using memory via assembly
// solhint-disable no-inline-assembly
/// @solidity memory-safe-assembly
Expand Down Expand Up @@ -341,7 +341,7 @@ abstract contract LSP0ERC725AccountCore is

// If the caller is the owner perform setData directly
if (msg.sender == _owner) {
for (uint256 i = 0; i < dataKeys.length; ) {
for (uint256 i; i < dataKeys.length; ) {
_setData(dataKeys[i], dataValues[i]);

unchecked {
Expand All @@ -356,7 +356,7 @@ abstract contract LSP0ERC725AccountCore is
// Depending on the magicValue returned, a second call is done after setting data
bool verifyAfter = _verifyCall(_owner);

for (uint256 i = 0; i < dataKeys.length; ) {
for (uint256 i; i < dataKeys.length; ) {
_setData(dataKeys[i], dataValues[i]);

unchecked {
Expand Down Expand Up @@ -671,7 +671,7 @@ abstract contract LSP0ERC725AccountCore is
address _owner = owner();

// If owner is a contract
if (_owner.code.length > 0) {
if (_owner.code.length != 0) {
(bool success, bytes memory result) = _owner.staticcall(
abi.encodeWithSelector(
IERC1271.isValidSignature.selector,
Expand Down
2 changes: 1 addition & 1 deletion ...LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP/LSP1UniversalReceiverDelegateUP.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ contract LSP1UniversalReceiverDelegateUP is ERC165, ILSP1UniversalReceiver {
) internal returns (bytes memory) {
// CHECK balance only when the Token contract is already deployed,
// not when tokens are being transferred on deployment through the `constructor`
if (notifier.code.length > 0) {
if (notifier.code.length != 0) {
// if the amount sent is 0, then do not update the keys
try ILSP7DigitalAsset(notifier).balanceOf(msg.sender) returns (
uint256 balance
Expand Down
2 changes: 1 addition & 1 deletion ...iversalReceiver/LSP1UniversalReceiverDelegateVault/LSP1UniversalReceiverDelegateVault.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ contract LSP1UniversalReceiverDelegateVault is ERC165, ILSP1UniversalReceiver {
) internal returns (bytes memory) {
// CHECK balance only when the Token contract is already deployed,
// not when tokens are being transferred on deployment through the `constructor`
if (notifier.code.length > 0) {
if (notifier.code.length != 0) {
// if the amount sent is 0, then do not update the keys
try ILSP7DigitalAsset(notifier).balanceOf(msg.sender) returns (
uint256 balance
Expand Down
2 changes: 1 addition & 1 deletion contracts/LSP20CallVerification/LSP20CallVerification.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ abstract contract LSP20CallVerification {
bytes memory returnedData
) internal pure virtual {
// Look for revert reason and bubble it up if present
if (returnedData.length > 0) {
if (returnedData.length != 0) {
// The easiest way to bubble the revert reason is using memory via assembly
// solhint-disable no-inline-assembly
/// @solidity memory-safe-assembly
Expand Down
3 changes: 1 addition & 2 deletions contracts/LSP25ExecuteRelayCall/LSP25MultiChannelNonce.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ abstract contract LSP25MultiChannelNonce {
address from,
uint128 channelId
) internal view virtual returns (uint256 idx) {
uint256 nonceInChannel = _nonceStore[from][channelId];
return (uint256(channelId) << 128) | nonceInChannel;
return (uint256(channelId) << 128) | _nonceStore[from][channelId];
}

/**
Expand Down
68 changes: 0 additions & 68 deletions contracts/LSP2ERC725YJSONSchema/LSP2Utils.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,74 +308,6 @@ library LSP2Utils {
return true;
}

/**
* @dev Verify if `data` is an abi-encoded array of addresses (`address[]`) encoded according to the ABI specs.
*
* @param data The bytes value to verify.
*
* @return `true` if the `data` represents an abi-encoded array of addresses, `false` otherwise.
*/
function isEncodedArrayOfAddresses(
bytes memory data
) internal pure returns (bool) {
if (!isEncodedArray(data)) return false;

uint256 offset = uint256(bytes32(data));
uint256 arrayLength = data.toUint256(offset);

uint256 pointer = offset + 32;

for (uint256 ii = 0; ii < arrayLength; ) {
bytes32 key = data.toBytes32(pointer);

// check that the leading bytes are zero bytes "00"
// NB: address type is padded on the left (unlike bytes20 type that is padded on the right)
if (bytes12(key) != bytes12(0)) return false;

// increment the pointer
pointer += 32;

unchecked {
++ii;
}
}

return true;
}

/**
* @dev Verify if `data` is an abi-array of `bytes4` values (`bytes4[]`) encoded according to the ABI specs.
*
* @param data The bytes value to verify.
*
* @return `true` if the `data` represents an abi-encoded array of `bytes4`, `false` otherwise.
*/
function isBytes4EncodedArray(
bytes memory data
) internal pure returns (bool) {
if (!isEncodedArray(data)) return false;

uint256 offset = uint256(bytes32(data));
uint256 arrayLength = data.toUint256(offset);
uint256 pointer = offset + 32;

for (uint256 ii = 0; ii < arrayLength; ) {
bytes32 key = data.toBytes32(pointer);

// check that the trailing bytes are zero bytes "00"
if (uint224(uint256(key)) != 0) return false;

// increment the pointer
pointer += 32;

unchecked {
++ii;
}
}

return true;
}

/**
* @dev Verify if `data` is a valid array of value encoded as a `CompactBytesArray` according to the LSP2 `CompactBytesArray` valueType specification.
*
Expand Down
95 changes: 63 additions & 32 deletions contracts/LSP6KeyManager/LSP6KeyManagerCore.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,9 @@ abstract contract LSP6KeyManagerCore is

// Variables, methods and modifier used for ReentrancyGuard are taken from the link below and modified accordingly.
// https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.8/contracts/security/ReentrancyGuard.sol
bool internal _reentrancyStatus;
uint8 internal _reentrancyStatus;
uint8 internal constant _NOT_ENTERED = 1;
uint8 internal constant _ENTERED = 2;

/**
* @inheritdoc ILSP6
Expand Down Expand Up @@ -195,7 +197,7 @@ abstract contract LSP6KeyManagerCore is
bytes[] memory results = new bytes[](payloads.length);
uint256 totalValues;

for (uint256 ii = 0; ii < payloads.length; ) {
for (uint256 ii; ii < payloads.length; ) {
if ((totalValues += values[ii]) > msg.value) {
revert LSP6BatchInsufficientValueSent(totalValues, msg.value);
}
Expand Down Expand Up @@ -278,7 +280,7 @@ abstract contract LSP6KeyManagerCore is
bytes[] memory results = new bytes[](payloads.length);
uint256 totalValues;

for (uint256 ii = 0; ii < payloads.length; ) {
for (uint256 ii; ii < payloads.length; ) {
if ((totalValues += values[ii]) > msg.value) {
revert LSP6BatchInsufficientValueSent(totalValues, msg.value);
}
Expand Down Expand Up @@ -329,37 +331,43 @@ abstract contract LSP6KeyManagerCore is
isSetData = true;
}

address targetContract = _target;

// If target is invoking the verification, emit the event and change the reentrancy guard
if (msg.sender == _target) {
bool isReentrantCall = _nonReentrantBefore(isSetData, caller);
if (msg.sender == targetContract) {
uint8 reentrancyStatus = _nonReentrantBefore(
targetContract,
isSetData,
caller
);

_verifyPermissions(caller, msgValue, data);
_verifyPermissions(targetContract, caller, msgValue, data);
emit PermissionsVerified(caller, msgValue, bytes4(data));

// if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function
return
isSetData || isReentrantCall
isSetData || (reentrancyStatus == _ENTERED)
? _LSP20_VERIFY_CALL_MAGIC_VALUE_WITHOUT_POST_VERIFICATION
: _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION;
}
/// @dev If a different address is invoking the verification,
/// do not change the state or emit the event to allow read-only verification
else {
bool isReentrantCall = _reentrancyStatus;
uint8 reentrancyStatus = _reentrancyStatus;

if (isReentrantCall) {
if (reentrancyStatus == _ENTERED) {
_requirePermissions(
caller,
ERC725Y(_target).getPermissionsFor(caller),
ERC725Y(targetContract).getPermissionsFor(caller),
_PERMISSION_REENTRANCY
);
}

_verifyPermissions(caller, msgValue, data);
_verifyPermissions(targetContract, caller, msgValue, data);

// if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function
return
isSetData || isReentrantCall
isSetData || (reentrancyStatus == _ENTERED)
? _LSP20_VERIFY_CALL_MAGIC_VALUE_WITHOUT_POST_VERIFICATION
: _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION;
}
Expand Down Expand Up @@ -396,14 +404,24 @@ abstract contract LSP6KeyManagerCore is
isSetData = true;
}

bool isReentrantCall = _nonReentrantBefore(isSetData, msg.sender);
address targetContract = _target;

uint8 reentrancyStatus = _nonReentrantBefore(
targetContract,
isSetData,
msg.sender
);

_verifyPermissions(msg.sender, msgValue, payload);
_verifyPermissions(targetContract, msg.sender, msgValue, payload);
emit PermissionsVerified(msg.sender, msgValue, bytes4(payload));

bytes memory result = _executePayload(msgValue, payload);
bytes memory result = _executePayload(
targetContract,
msgValue,
payload
);

if (!isReentrantCall && !isSetData) {
if (reentrancyStatus == _NOT_ENTERED && !isSetData) {
_nonReentrantAfter();
}

Expand Down Expand Up @@ -435,6 +453,8 @@ abstract contract LSP6KeyManagerCore is
revert InvalidPayload(payload);
}

address targetContract = _target;

address signer = LSP25MultiChannelNonce
._recoverSignerFromLSP25Signature(
signature,
Expand All @@ -461,14 +481,22 @@ abstract contract LSP6KeyManagerCore is
isSetData = true;
}

bool isReentrantCall = _nonReentrantBefore(isSetData, signer);
uint8 reentrancyStatus = _nonReentrantBefore(
targetContract,
isSetData,
signer
);

_verifyPermissions(signer, msgValue, payload);
_verifyPermissions(targetContract, signer, msgValue, payload);
emit PermissionsVerified(signer, msgValue, bytes4(payload));

bytes memory result = _executePayload(msgValue, payload);
bytes memory result = _executePayload(
targetContract,
msgValue,
payload
);

if (!isReentrantCall && !isSetData) {
if (reentrancyStatus == _NOT_ENTERED && !isSetData) {
_nonReentrantAfter();
}

Expand All @@ -481,10 +509,11 @@ abstract contract LSP6KeyManagerCore is
* @return bytes The data returned by the call made to the linked {target} contract.
*/
function _executePayload(
address targetContract,
uint256 msgValue,
bytes calldata payload
) internal virtual returns (bytes memory) {
(bool success, bytes memory returnData) = _target.call{
(bool success, bytes memory returnData) = targetContract.call{
value: msgValue,
gas: gasleft()
}(payload);
Expand All @@ -503,11 +532,12 @@ abstract contract LSP6KeyManagerCore is
* @param payload The abi-encoded function call to execute on the {target} contract.
*/
function _verifyPermissions(
address targetContract,
address from,
uint256 msgValue,
bytes calldata payload
) internal view virtual {
bytes32 permissions = ERC725Y(_target).getPermissionsFor(from);
bytes32 permissions = ERC725Y(targetContract).getPermissionsFor(from);
if (permissions == bytes32(0)) revert NoPermissionsSet(from);

bytes4 erc725Function = bytes4(payload);
Expand All @@ -521,7 +551,7 @@ abstract contract LSP6KeyManagerCore is
);

LSP6SetDataModule._verifyCanSetData(
_target,
targetContract,
from,
permissions,
inputKey,
Expand All @@ -535,7 +565,7 @@ abstract contract LSP6KeyManagerCore is
.decode(payload[4:], (bytes32[], bytes[]));

LSP6SetDataModule._verifyCanSetData(
_target,
targetContract,
from,
permissions,
inputKeys,
Expand All @@ -552,7 +582,7 @@ abstract contract LSP6KeyManagerCore is
) = abi.decode(payload[4:], (uint256, address, uint256, bytes));

LSP6ExecuteModule._verifyCanExecute(
_target,
targetContract,
from,
permissions,
operationType,
Expand All @@ -574,7 +604,7 @@ abstract contract LSP6KeyManagerCore is
* @dev Initialise _reentrancyStatus to _NOT_ENTERED.
*/
function _setupLSP6ReentrancyGuard() internal virtual {
_reentrancyStatus = false;
_reentrancyStatus = 1;
}

/**
Expand All @@ -583,20 +613,21 @@ abstract contract LSP6KeyManagerCore is
* Used in the beginning of the `nonReentrant` modifier, before the method execution starts.
*/
function _nonReentrantBefore(
address targetContract,
bool isSetData,
address from
) internal virtual returns (bool isReentrantCall) {
isReentrantCall = _reentrancyStatus;
if (isReentrantCall) {
) internal virtual returns (uint8 reentrancyStatus) {
reentrancyStatus = _reentrancyStatus;
if (reentrancyStatus == _ENTERED) {
// CHECK the caller has REENTRANCY permission
_requirePermissions(
from,
ERC725Y(_target).getPermissionsFor(from),
ERC725Y(targetContract).getPermissionsFor(from),
_PERMISSION_REENTRANCY
);
} else {
if (!isSetData) {
_reentrancyStatus = true;
_reentrancyStatus = _ENTERED;
}
}
}
Expand All @@ -608,7 +639,7 @@ abstract contract LSP6KeyManagerCore is
function _nonReentrantAfter() internal virtual {
// By storing the original value once again, a refund is triggered (see
// https://eips.ethereum.org/EIPS/eip-2200)
_reentrancyStatus = false;
_reentrancyStatus = _NOT_ENTERED;
}

/**
Expand Down
Loading

0 comments on commit f1686d5

Please sign in to comment.