-
Notifications
You must be signed in to change notification settings - Fork 514
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
Checkout #570
base: main
Are you sure you want to change the base?
Checkout #570
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slither found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #570 +/- ##
==========================================
- Coverage 64.08% 63.85% -0.23%
==========================================
Files 215 220 +5
Lines 6632 6748 +116
==========================================
+ Hits 4250 4309 +59
- Misses 2382 2439 +57 ☔ View full report in Codecov by Sentry. |
contract Checkout is PermissionsEnumerable, ICheckout { | ||
/// @dev Registry of vaults created through this Checkout | ||
mapping(address => bool) public isVaultRegistered; | ||
|
||
/// @dev Registry of executors created through this Checkout | ||
mapping(address => bool) public isExecutorRegistered; | ||
|
||
address public immutable vaultImplementation; | ||
address public immutable executorImplementation; | ||
|
||
constructor( | ||
address _defaultAdmin, | ||
address _vaultImplementation, | ||
address _executorImplementation | ||
) { | ||
vaultImplementation = _vaultImplementation; | ||
executorImplementation = _executorImplementation; | ||
|
||
_setupRole(DEFAULT_ADMIN_ROLE, _defaultAdmin); | ||
} | ||
|
||
function createVault(address _vaultAdmin, bytes32 _salt) external payable returns (address) { | ||
bytes32 salthash = keccak256(abi.encodePacked(msg.sender, _salt)); | ||
address vault = Clones.cloneDeterministic(vaultImplementation, salthash); | ||
|
||
(bool success, ) = vault.call(abi.encodeWithSelector(Vault.initialize.selector, _vaultAdmin)); | ||
|
||
require(success, "Deployment failed"); | ||
|
||
isVaultRegistered[vault] = true; | ||
|
||
emit VaultCreated(vault, _vaultAdmin); | ||
|
||
return vault; | ||
} | ||
|
||
function createExecutor(address _executorAdmin, bytes32 _salt) external payable returns (address) { | ||
bytes32 salthash = keccak256(abi.encodePacked(msg.sender, _salt)); | ||
address executor = Clones.cloneDeterministic(executorImplementation, salthash); | ||
|
||
(bool success, ) = executor.call(abi.encodeWithSelector(Executor.initialize.selector, _executorAdmin)); | ||
|
||
require(success, "Deployment failed"); | ||
|
||
isExecutorRegistered[executor] = true; | ||
|
||
emit ExecutorCreated(executor, _executorAdmin); | ||
|
||
return executor; | ||
} | ||
|
||
function authorizeVaultToExecutor(address _vault, address _executor) external { | ||
require(IVault(_vault).canAuthorizeVaultToExecutor(msg.sender), "Not authorized"); | ||
require(isExecutorRegistered[_executor], "Executor not found"); | ||
|
||
IVault(_vault).setExecutor(_executor); | ||
|
||
emit VaultAuthorizedToExecutor(_vault, _executor); | ||
} | ||
} |
Check warning
Code scanning / Slither
Contracts that lock Ether Medium
Contract Checkout has payable functions:
- ICheckout.createVault(address,bytes32)
- ICheckout.createExecutor(address,bytes32)
- Checkout.createVault(address,bytes32)
- Checkout.createExecutor(address,bytes32)
But does not have a function to withdraw the ether
function execute(UserOp calldata op) external { | ||
require(_canExecute(), "Not authorized"); | ||
|
||
if (op.valueToSend != 0) { | ||
IVault(op.vault).transferTokensToExecutor(op.currency, op.valueToSend); | ||
} | ||
|
||
bool success; | ||
if (op.currency == CurrencyTransferLib.NATIVE_TOKEN) { | ||
(success, ) = op.target.call{ value: op.valueToSend }(op.data); | ||
} else { | ||
if (op.approvalRequired) { | ||
IERC20(op.currency).approve(op.target, op.valueToSend); | ||
} | ||
|
||
(success, ) = op.target.call(op.data); | ||
} | ||
|
||
require(success, "Execution failed"); | ||
} |
Check warning
Code scanning / Slither
Unused return Medium
function createExecutor(address _executorAdmin, bytes32 _salt) external payable returns (address) { | ||
bytes32 salthash = keccak256(abi.encodePacked(msg.sender, _salt)); | ||
address executor = Clones.cloneDeterministic(executorImplementation, salthash); | ||
|
||
(bool success, ) = executor.call(abi.encodeWithSelector(Executor.initialize.selector, _executorAdmin)); | ||
|
||
require(success, "Deployment failed"); | ||
|
||
isExecutorRegistered[executor] = true; | ||
|
||
emit ExecutorCreated(executor, _executorAdmin); | ||
|
||
return executor; | ||
} |
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
External calls:
- (success) = executor.call(abi.encodeWithSelector(Executor.initialize.selector,_executorAdmin))
State variables written after the call(s):
- isExecutorRegistered[executor] = true
function createVault(address _vaultAdmin, bytes32 _salt) external payable returns (address) { | ||
bytes32 salthash = keccak256(abi.encodePacked(msg.sender, _salt)); | ||
address vault = Clones.cloneDeterministic(vaultImplementation, salthash); | ||
|
||
(bool success, ) = vault.call(abi.encodeWithSelector(Vault.initialize.selector, _vaultAdmin)); | ||
|
||
require(success, "Deployment failed"); | ||
|
||
isVaultRegistered[vault] = true; | ||
|
||
emit VaultCreated(vault, _vaultAdmin); | ||
|
||
return vault; | ||
} |
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
External calls:
- (success) = vault.call(abi.encodeWithSelector(Vault.initialize.selector,_vaultAdmin))
State variables written after the call(s):
- isVaultRegistered[vault] = true
function withdraw(address _token, uint256 _amount) external { | ||
require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "Not authorized"); | ||
|
||
CurrencyTransferLib.transferCurrency(_token, address(this), msg.sender, _amount); | ||
|
||
emit TokensWithdrawn(_token, _amount); | ||
} |
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
External calls:
- CurrencyTransferLib.transferCurrency(_token,address(this),msg.sender,_amount)
Event emitted after the call(s):
- TokensWithdrawn(_token,_amount)
function _swap(SwapOp memory _swapOp) internal { | ||
address _tokenIn = _swapOp.tokenIn; | ||
address _router = _swapOp.router; | ||
|
||
// get quote for amountIn | ||
(, bytes memory quoteData) = _router.staticcall(_swapOp.quoteCalldata); | ||
uint256 amountIn; | ||
uint256 offset = _swapOp.amountInOffset; | ||
|
||
assembly { | ||
amountIn := mload(add(add(quoteData, 32), offset)) | ||
} | ||
|
||
// perform swap | ||
bool success; | ||
if (_tokenIn == CurrencyTransferLib.NATIVE_TOKEN) { | ||
(success, ) = _router.call{ value: amountIn }(_swapOp.swapCalldata); | ||
} else { | ||
IERC20(_tokenIn).approve(_swapOp.router, amountIn); | ||
(success, ) = _router.call(_swapOp.swapCalldata); | ||
} | ||
|
||
require(success, "Swap failed"); | ||
} |
Check failure
Code scanning / Slither
Functions that send Ether to arbitrary destinations High
Dangerous calls:
- (success,None) = _router.call{value: amountIn}(_swapOp.swapCalldata)
function _swap(SwapOp memory _swapOp) internal { | ||
address _tokenIn = _swapOp.tokenIn; | ||
address _router = _swapOp.router; | ||
|
||
// get quote for amountIn | ||
(, bytes memory quoteData) = _router.staticcall(_swapOp.quoteCalldata); | ||
uint256 amountIn; | ||
uint256 offset = _swapOp.amountInOffset; | ||
|
||
assembly { | ||
amountIn := mload(add(add(quoteData, 32), offset)) | ||
} | ||
|
||
// perform swap | ||
bool success; | ||
if (_tokenIn == CurrencyTransferLib.NATIVE_TOKEN) { | ||
(success, ) = _router.call{ value: amountIn }(_swapOp.swapCalldata); | ||
} else { | ||
IERC20(_tokenIn).approve(_swapOp.router, amountIn); | ||
(success, ) = _router.call(_swapOp.swapCalldata); | ||
} | ||
|
||
require(success, "Swap failed"); | ||
} |
Check warning
Code scanning / Slither
Unused return Medium
function swapAndTransferTokensToExecutor(address _token, uint256 _amount, SwapOp memory _swapOp) external { | ||
require(_canTransferTokens(), "Not authorized"); | ||
require(isApprovedRouter[_swapOp.router], "Invalid router address"); | ||
|
||
_swap(_swapOp); | ||
|
||
uint256 balance = _token == CurrencyTransferLib.NATIVE_TOKEN | ||
? address(this).balance | ||
: IERC20(_token).balanceOf(address(this)); | ||
|
||
require(balance >= _amount, "Not enough balance"); | ||
|
||
CurrencyTransferLib.transferCurrency(_token, address(this), msg.sender, _amount); | ||
|
||
emit TokensTransferredToExecutor(msg.sender, _token, _amount); | ||
} |
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
External calls:
- _swap(_swapOp)
- (success,None) = _router.call{value: amountIn}(_swapOp.swapCalldata)
- IERC20(_tokenIn).approve(_swapOp.router,amountIn)
- (success,None) = _router.call(_swapOp.swapCalldata)
- CurrencyTransferLib.transferCurrency(_token,address(this),msg.sender,_amount)
External calls sending eth:
- _swap(_swapOp)
- (success,None) = _router.call{value: amountIn}(_swapOp.swapCalldata)
Event emitted after the call(s):
- TokensTransferredToExecutor(msg.sender,_token,_amount)
function _swap(SwapOp memory _swapOp) internal { | ||
address _tokenIn = _swapOp.tokenIn; | ||
address _router = _swapOp.router; | ||
|
||
// get quote for amountIn | ||
(, bytes memory quoteData) = _router.staticcall(_swapOp.quoteCalldata); | ||
uint256 amountIn; | ||
uint256 offset = _swapOp.amountInOffset; | ||
|
||
assembly { | ||
amountIn := mload(add(add(quoteData, 32), offset)) | ||
} | ||
|
||
// perform swap | ||
bool success; | ||
if (_tokenIn == CurrencyTransferLib.NATIVE_TOKEN) { | ||
(success, ) = _router.call{ value: amountIn }(_swapOp.swapCalldata); | ||
} else { | ||
IERC20(_tokenIn).approve(_swapOp.router, amountIn); | ||
(success, ) = _router.call(_swapOp.swapCalldata); | ||
} | ||
|
||
require(success, "Swap failed"); | ||
} |
Check warning
Code scanning / Slither
Assembly usage Warning
function _swap(SwapOp memory _swapOp) internal { | ||
address _tokenIn = _swapOp.tokenIn; | ||
address _router = _swapOp.router; | ||
|
||
// get quote for amountIn | ||
(, bytes memory quoteData) = _router.staticcall(_swapOp.quoteCalldata); | ||
uint256 amountIn; | ||
uint256 offset = _swapOp.amountInOffset; | ||
|
||
assembly { | ||
amountIn := mload(add(add(quoteData, 32), offset)) | ||
} | ||
|
||
// perform swap | ||
bool success; | ||
if (_tokenIn == CurrencyTransferLib.NATIVE_TOKEN) { | ||
(success, ) = _router.call{ value: amountIn }(_swapOp.swapCalldata); | ||
} else { | ||
IERC20(_tokenIn).approve(_swapOp.router, amountIn); | ||
(success, ) = _router.call(_swapOp.swapCalldata); | ||
} | ||
|
||
require(success, "Swap failed"); | ||
} |
Check warning
Code scanning / Slither
Low-level calls Warning
No description provided.