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

chore: [C4] Add first batch of QA fixes #693

Merged
merged 8 commits into from
Sep 27, 2023
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
57 changes: 34 additions & 23 deletions contracts/LSP0ERC725Account/LSP0ERC725AccountCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ import {
_TYPEID_LSP0_OwnershipTransferStarted,
_TYPEID_LSP0_OwnershipTransferred_SenderNotification,
_TYPEID_LSP0_OwnershipTransferred_RecipientNotification
} from "../LSP0ERC725Account/LSP0Constants.sol";
} from "./LSP0Constants.sol";
import {
_INTERFACEID_LSP1,
_LSP1_UNIVERSAL_RECEIVER_DELEGATE_PREFIX,
Expand All @@ -56,7 +56,8 @@ import {

// errors
import {
ERC725Y_DataKeysValuesLengthMismatch
ERC725Y_DataKeysValuesLengthMismatch,
ERC725Y_DataKeysValuesEmptyArray
} from "@erc725/smart-contracts/contracts/errors.sol";
import {
NoExtensionFoundForFunctionSelector
Expand Down Expand Up @@ -191,16 +192,16 @@ abstract contract LSP0ERC725AccountCore is
emit ValueReceived(msg.sender, msg.value);
}

address _owner = owner();
address accountOwner = owner();
YamenMerhi marked this conversation as resolved.
Show resolved Hide resolved

// If the caller is the owner perform execute directly
if (msg.sender == _owner) {
if (msg.sender == accountOwner) {
return ERC725XCore._execute(operationType, target, value, data);
}

// If the caller is not the owner, call {lsp20VerifyCall} on the owner
// Depending on the magicValue returned, a second call is done after execution
bool verifyAfter = LSP20CallVerification._verifyCall(_owner);
bool verifyAfter = LSP20CallVerification._verifyCall(accountOwner);

// Perform the execution
bytes memory result = ERC725XCore._execute(
Expand All @@ -212,7 +213,10 @@ abstract contract LSP0ERC725AccountCore is

// if verifyAfter is true, Call {lsp20VerifyCallResult} on the owner
if (verifyAfter) {
LSP20CallVerification._verifyCallResult(_owner, abi.encode(result));
LSP20CallVerification._verifyCallResult(
accountOwner,
abi.encode(result)
);
}

return result;
Expand All @@ -228,6 +232,9 @@ abstract contract LSP0ERC725AccountCore is
* - If the operation type is `CREATE` (1) or `CREATE2` (2), `target` must be `address(0)`.
* - If the operation type is `STATICCALL` (3) or `DELEGATECALL` (4), `value` transfer is disallowed and must be 0.
*
* @custom:warning
* - The `msg.value` should not be trusted for any method called with `operationType`: `DELEGATECALL` (4).
YamenMerhi marked this conversation as resolved.
Show resolved Hide resolved
*
* @custom:events
* - {Executed} event for each call that uses under `operationType`: `CALL` (0), `STATICCALL` (3) and `DELEGATECALL` (4). (each iteration)
* - {ContractCreated} event, when a contract is created under `operationType`: `CREATE` (1) and `CREATE2` (2) (each iteration)
Expand All @@ -243,10 +250,10 @@ abstract contract LSP0ERC725AccountCore is
emit ValueReceived(msg.sender, msg.value);
}

address _owner = owner();
address accountOwner = owner();

// If the caller is the owner perform execute directly
if (msg.sender == _owner) {
if (msg.sender == accountOwner) {
return
ERC725XCore._executeBatch(
operationsType,
Expand All @@ -258,7 +265,7 @@ abstract contract LSP0ERC725AccountCore is

// If the caller is not the owner, call {lsp20VerifyCall} on the owner
// Depending on the magicValue returned, a second call is done after execution
bool verifyAfter = LSP20CallVerification._verifyCall(_owner);
bool verifyAfter = LSP20CallVerification._verifyCall(accountOwner);

// Perform the execution
bytes[] memory results = ERC725XCore._executeBatch(
Expand All @@ -271,7 +278,7 @@ abstract contract LSP0ERC725AccountCore is
// if verifyAfter is true, Call {lsp20VerifyCallResult} on the owner
if (verifyAfter) {
LSP20CallVerification._verifyCallResult(
_owner,
accountOwner,
abi.encode(results)
);
}
Expand All @@ -296,23 +303,23 @@ abstract contract LSP0ERC725AccountCore is
emit ValueReceived(msg.sender, msg.value);
}

address _owner = owner();
address accountOwner = owner();

// If the caller is the owner perform setData directly
if (msg.sender == _owner) {
if (msg.sender == accountOwner) {
return _setData(dataKey, dataValue);
}

// If the caller is not the owner, call {lsp20VerifyCall} on the owner
// Depending on the magicValue returned, a second call is done after setting data
bool verifyAfter = _verifyCall(_owner);
bool verifyAfter = _verifyCall(accountOwner);

_setData(dataKey, dataValue);

// If verifyAfter is true, Call {lsp20VerifyCallResult} on the owner
// The setData function does not return, second parameter of {_verifyCallResult} will be empty
if (verifyAfter) {
_verifyCallResult(_owner, "");
_verifyCallResult(accountOwner, "");
}
}

Expand All @@ -337,10 +344,14 @@ abstract contract LSP0ERC725AccountCore is
revert ERC725Y_DataKeysValuesLengthMismatch();
}

address _owner = owner();
if (dataKeys.length == 0) {
revert ERC725Y_DataKeysValuesEmptyArray();
}

address accountOwner = owner();

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

Expand All @@ -354,7 +365,7 @@ abstract contract LSP0ERC725AccountCore is

// If the caller is not the owner, call {lsp20VerifyCall} on the owner
// Depending on the magicValue returned, a second call is done after setting data
bool verifyAfter = _verifyCall(_owner);
bool verifyAfter = _verifyCall(accountOwner);

for (uint256 i = 0; i < dataKeys.length; ) {
_setData(dataKeys[i], dataValues[i]);
Expand All @@ -367,7 +378,7 @@ abstract contract LSP0ERC725AccountCore is
// If verifyAfter is true, Call {lsp20VerifyCallResult} on the owner
// The setData function does not return, second parameter of {_verifyCallResult} will be empty
if (verifyAfter) {
_verifyCallResult(_owner, "");
_verifyCallResult(accountOwner, "");
}
}

Expand Down Expand Up @@ -572,24 +583,24 @@ abstract contract LSP0ERC725AccountCore is
*
* @custom:requirements Can be only called by the {owner} or by an authorised address that pass the verification check performed on the owner.
*
* @custom:danger Leaves the contract without an owner. Once ownership of the contract has been renounced, any functions that are restricted to be called by the owner will be permanently inaccessible, making these functions not callable anymore and unusable.
* @custom:danger Leaves the contract without an owner. Once ownership of the contract has been renounced, any functions that are restricted to be called by the owner or an address allowed by the owner will be permanently inaccessible, making these functions not callable anymore and unusable.
YamenMerhi marked this conversation as resolved.
Show resolved Hide resolved
*
*/
function renounceOwnership()
public
virtual
override(LSP14Ownable2Step, OwnableUnset)
{
address _owner = owner();
address accountOwner = owner();

// If the caller is the owner perform renounceOwnership directly
if (msg.sender == _owner) {
if (msg.sender == accountOwner) {
return LSP14Ownable2Step._renounceOwnership();
}

// If the caller is not the owner, call {lsp20VerifyCall} on the owner
// Depending on the magicValue returned, a second call is done after transferring ownership
bool verifyAfter = _verifyCall(_owner);
bool verifyAfter = _verifyCall(accountOwner);

address previousOwner = owner();
LSP14Ownable2Step._renounceOwnership();
Expand All @@ -604,7 +615,7 @@ abstract contract LSP0ERC725AccountCore is
// If verifyAfter is true, Call {lsp20VerifyCallResult} on the owner
// The transferOwnership function does not return, second parameter of {_verifyCallResult} will be empty
if (verifyAfter) {
_verifyCallResult(_owner, "");
_verifyCallResult(accountOwner, "");
}
}

Expand Down
2 changes: 1 addition & 1 deletion contracts/LSP14Ownable2Step/ILSP14Ownable2Step.sol
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ interface ILSP14Ownable2Step {
* @dev Transfer ownership of the contract from the current {owner()} to the {pendingOwner()}.
*
* Once this function is called:
* - The current {owner()} will loose access to the functions restricted to the {owner()} only.
* - The current {owner()} will lose access to the functions restricted to the {owner()} only.
* - The {pendingOwner()} will gain access to the functions restricted to the {owner()} only.
*
* @notice `msg.sender` is accepting ownership of contract: `address(this)`.
Expand Down
8 changes: 6 additions & 2 deletions contracts/LSP20CallVerification/LSP20CallVerification.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import {ILSP20CallVerifier as ILSP20} from "./ILSP20CallVerifier.sol";
// errors
import {
LSP20InvalidMagicValue,
LSP20CallingVerifierFailed
LSP20CallingVerifierFailed,
EOACannotVerifyCall
} from "./LSP20Errors.sol";

/**
Expand All @@ -26,6 +27,9 @@ abstract contract LSP20CallVerification {
function _verifyCall(
address logicVerifier
) internal virtual returns (bool verifyAfter) {
if (logicVerifier.code.length == 0)
revert EOACannotVerifyCall(logicVerifier);

(bool success, bytes memory returnedData) = logicVerifier.call(
abi.encodeWithSelector(
ILSP20.lsp20VerifyCall.selector,
Expand All @@ -42,7 +46,7 @@ abstract contract LSP20CallVerification {
if (bytes3(magicValue) != bytes3(ILSP20.lsp20VerifyCall.selector))
revert LSP20InvalidMagicValue(false, returnedData);

return magicValue[3] == 0x01 ? true : false;
return bytes1(magicValue[3]) == 0x01;
YamenMerhi marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
6 changes: 6 additions & 0 deletions contracts/LSP20CallVerification/LSP20Errors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,9 @@ error LSP20CallingVerifierFailed(bool postCall);
* @param returnedData The data returned by the call to the logic verifier
*/
error LSP20InvalidMagicValue(bool postCall, bytes returnedData);

/**
* @dev reverts when the logicVerifier is an EOA that cannot return magicValue
YamenMerhi marked this conversation as resolved.
Show resolved Hide resolved
* @param logicVerifier The address of the logic verifier
*/
error EOACannotVerifyCall(address logicVerifier);
YamenMerhi marked this conversation as resolved.
Show resolved Hide resolved
27 changes: 6 additions & 21 deletions contracts/LSP6KeyManager/LSP6KeyManagerCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -321,13 +321,8 @@ abstract contract LSP6KeyManagerCore is
uint256 msgValue,
bytes calldata data
) external virtual returns (bytes4) {
bool isSetData = false;
if (
bytes4(data) == IERC725Y.setData.selector ||
bytes4(data) == IERC725Y.setDataBatch.selector
) {
isSetData = true;
}
bool isSetData = bytes4(data) == IERC725Y.setData.selector ||
YamenMerhi marked this conversation as resolved.
Show resolved Hide resolved
bytes4(data) == IERC725Y.setDataBatch.selector;

// If target is invoking the verification, emit the event and change the reentrancy guard
if (msg.sender == _target) {
Expand Down Expand Up @@ -388,13 +383,8 @@ abstract contract LSP6KeyManagerCore is
revert InvalidPayload(payload);
}

bool isSetData = false;
if (
bytes4(payload) == IERC725Y.setData.selector ||
bytes4(payload) == IERC725Y.setDataBatch.selector
) {
isSetData = true;
}
bool isSetData = bytes4(payload) == IERC725Y.setData.selector ||
bytes4(payload) == IERC725Y.setDataBatch.selector;

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

Expand Down Expand Up @@ -453,13 +443,8 @@ abstract contract LSP6KeyManagerCore is

LSP25MultiChannelNonce._verifyValidityTimestamps(validityTimestamps);

bool isSetData = false;
if (
bytes4(payload) == IERC725Y.setData.selector ||
bytes4(payload) == IERC725Y.setDataBatch.selector
) {
isSetData = true;
}
bool isSetData = bytes4(payload) == IERC725Y.setData.selector ||
bytes4(payload) == IERC725Y.setDataBatch.selector;

bool isReentrantCall = _nonReentrantBefore(isSetData, signer);

Expand Down
14 changes: 7 additions & 7 deletions contracts/LSP7DigitalAsset/LSP7DigitalAssetCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -271,37 +271,37 @@ abstract contract LSP7DigitalAssetCore is ILSP7DigitalAsset {
* It has been added in the LSP7 contract implementation so that it can be used as a prevention mechanism
* against the double spending allowance vulnerability.
*
* @notice Decrease the allowance of `operator` by -`substractedAmount`
* @notice Decrease the allowance of `operator` by -`subtractedAmount`
*
* @dev Atomically decreases the allowance granted to `operator` by the caller.
* This is an alternative approach to {authorizeOperator} that can be used as a mitigation
* for the double spending allowance problem.
*
* @custom:events
* - {AuthorizedOperator} event indicating the updated allowance after decreasing it.
* - {RevokeOperator} event if `substractedAmount` is the full allowance,
* - {RevokeOperator} event if `subtractedAmount` is the full allowance,
* indicating `operator` does not have any alauthorizedAmountForlowance left for `msg.sender`.
*
* @param operator the operator to decrease allowance for `msg.sender`
* @param substractedAmount the amount to decrease by in the operator's allowance.
* @param subtractedAmount the amount to decrease by in the operator's allowance.
*
* @custom:requirements
* - `operator` cannot be the zero address.
* - `operator` must have allowance for the caller of at least `substractedAmount`.
* - `operator` must have allowance for the caller of at least `subtractedAmount`.
*/
function decreaseAllowance(
address operator,
uint256 substractedAmount,
uint256 subtractedAmount,
bytes memory operatorNotificationData
) public virtual {
uint256 currentAllowance = authorizedAmountFor(operator, msg.sender);
if (currentAllowance < substractedAmount) {
if (currentAllowance < subtractedAmount) {
revert LSP7DecreasedAllowanceBelowZero();
}

uint256 newAllowance;
unchecked {
newAllowance = currentAllowance - substractedAmount;
newAllowance = currentAllowance - subtractedAmount;
_updateOperator(
msg.sender,
operator,
Expand Down
Loading
Loading