From 2c361aeea4d95e78798c80be860fe3a50fde9d80 Mon Sep 17 00:00:00 2001 From: adu Date: Tue, 9 Jul 2024 12:20:51 +0800 Subject: [PATCH] slither: add comments for disabled detector --- src/core/Bootstrap.sol | 7 +++++++ src/core/ClientGatewayLzReceiver.sol | 4 ++++ src/core/CustomProxyAdmin.sol | 5 +++-- src/core/ExocoreGateway.sol | 2 ++ src/core/NativeRestakingController.sol | 7 +++++-- src/core/Vault.sol | 2 ++ src/libraries/BeaconChainProofs.sol | 1 + src/storage/BootstrapStorage.sol | 2 ++ 8 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/core/Bootstrap.sol b/src/core/Bootstrap.sol index 980ffefa..6a31ca3f 100644 --- a/src/core/Bootstrap.sol +++ b/src/core/Bootstrap.sol @@ -158,6 +158,9 @@ contract Bootstrap is _addWhitelistTokens(tokens); } + // Though `_deployVault` would make external call to newly created `Vault` contract and initialize it, + // `Vault` contract belongs to Exocore and we could make sure its implementation does not have dangerous behavior + // like reentrancy. // slither-disable-next-line reentrancy-no-eth function _addWhitelistTokens(address[] calldata tokens) internal { for (uint256 i; i < tokens.length; i++) { @@ -474,6 +477,10 @@ contract Bootstrap is } // implementation of ILSTRestakingController + // Though `_deposit` would make external call to `Vault` and some state variables would be written in the following + // `_delegateTo`, + // `Vault` contract belongs to Exocore and we could make sure it's implementation does not have dangerous behavior + // like reentrancy. // slither-disable-next-line reentrancy-no-eth function depositThenDelegateTo(address token, uint256 amount, string calldata operator) external diff --git a/src/core/ClientGatewayLzReceiver.sol b/src/core/ClientGatewayLzReceiver.sol index 7fe717e4..f945c52e 100644 --- a/src/core/ClientGatewayLzReceiver.sol +++ b/src/core/ClientGatewayLzReceiver.sol @@ -22,6 +22,7 @@ abstract contract ClientGatewayLzReceiver is PausableUpgradeable, OAppReceiverUp _; } + // This function would call other functions inside this contract through low-level-call // slither-disable-next-line reentrancy-no-eth function _lzReceive(Origin calldata _origin, bytes calldata payload) internal virtual override whenNotPaused { if (_origin.srcEid != EXOCORE_CHAIN_ID) { @@ -184,6 +185,9 @@ abstract contract ClientGatewayLzReceiver is PausableUpgradeable, OAppReceiverUp emit DepositThenDelegateResult(delegateSuccess, delegator, operator, token, amount); } + // Though `_deployVault` would make external call to newly created `Vault` contract and initialize it, + // `Vault` contract belongs to Exocore and we could make sure its implementation does not have dangerous behavior + // like reentrancy. // slither-disable-next-line reentrancy-no-eth function afterReceiveAddWhitelistTokensRequest(bytes calldata requestPayload) public diff --git a/src/core/CustomProxyAdmin.sol b/src/core/CustomProxyAdmin.sol index d6aa20ac..ca3f9cb4 100644 --- a/src/core/CustomProxyAdmin.sol +++ b/src/core/CustomProxyAdmin.sol @@ -18,12 +18,13 @@ contract CustomProxyAdmin is Initializable, ProxyAdmin { bootstrapper = newBootstrapper; } - // slither-disable-next-line reentrancy-no-eth function changeImplementation(address proxy, address implementation, bytes memory data) public virtual { require(msg.sender == bootstrapper, "CustomProxyAdmin: sender must be bootstrapper"); require(msg.sender == proxy, "CustomProxyAdmin: sender must be the proxy itself"); - ITransparentUpgradeableProxy(proxy).upgradeToAndCall(implementation, data); + + // we follow check-effects-interactions pattern to write state before external call bootstrapper = address(0); + ITransparentUpgradeableProxy(proxy).upgradeToAndCall(implementation, data); } } diff --git a/src/core/ExocoreGateway.sol b/src/core/ExocoreGateway.sol index 952d2c1a..a0e56961 100644 --- a/src/core/ExocoreGateway.sol +++ b/src/core/ExocoreGateway.sol @@ -169,6 +169,8 @@ contract ExocoreGateway is super.setPeer(clientChainId, clientChainGateway); } + // Though this function would call precompiled contract, all precompiled contracts belong to Exocore + // and we could make sure its implementation does not have dangerous behavior like reentrancy. // slither-disable-next-line reentrancy-no-eth function addWhitelistTokens( uint32 clientChainId, diff --git a/src/core/NativeRestakingController.sol b/src/core/NativeRestakingController.sol index 8b7dd3e9..6a473d9b 100644 --- a/src/core/NativeRestakingController.sol +++ b/src/core/NativeRestakingController.sol @@ -45,8 +45,9 @@ abstract contract NativeRestakingController is emit StakedWithCapsule(msg.sender, address(capsule)); } + // The bytecode returned by `BEACON_PROXY_BYTECODE` and `EXO_CAPSULE_BEACON` address are actually fixed size of byte array, + // so it would not cause collision for encodePacked // slither-disable-next-line encode-packed-collision - // slither-disable-next-line reentrancy-no-eth function createExoCapsule() public whenNotPaused nativeRestakingEnabled returns (address) { require( address(ownerToCapsule[msg.sender]) == address(0), @@ -60,8 +61,10 @@ abstract contract NativeRestakingController is abi.encodePacked(BEACON_PROXY_BYTECODE.getBytecode(), abi.encode(address(EXO_CAPSULE_BEACON), "")) ) ); - capsule.initialize(address(this), msg.sender, BEACON_ORACLE_ADDRESS); + + // we follow check-effects-interactions pattern to write state before external call ownerToCapsule[msg.sender] = capsule; + capsule.initialize(address(this), msg.sender, BEACON_ORACLE_ADDRESS); emit CapsuleCreated(msg.sender, address(capsule)); diff --git a/src/core/Vault.sol b/src/core/Vault.sol index 4163bc74..223baeb9 100644 --- a/src/core/Vault.sol +++ b/src/core/Vault.sol @@ -49,6 +49,8 @@ contract Vault is Initializable, VaultStorage, IVault { emit WithdrawalSuccess(withdrawer, recipient, amount); } + // Though `safeTransferFrom` has arbitrary passed in `depositor` as sender, this function is only callable by `gateway` + // and `gateway` would make sure only the `msg.sender` would be the depositor. // slither-disable-next-line arbitrary-send-erc20 function deposit(address depositor, uint256 amount) external payable onlyGateway { underlyingToken.safeTransferFrom(depositor, address(this), amount); diff --git a/src/libraries/BeaconChainProofs.sol b/src/libraries/BeaconChainProofs.sol index 08e6a8c2..21f00cd0 100644 --- a/src/libraries/BeaconChainProofs.sol +++ b/src/libraries/BeaconChainProofs.sol @@ -48,6 +48,7 @@ library BeaconChainProofs { uint64 internal constant SECONDS_PER_SLOT = 12; /// @notice Number of seconds per epoch: 384 == 32 slots/epoch * 12 seconds/slot + /// @dev This constant would be used by other contracts that import this library // slither-disable-next-line unused-state uint64 internal constant SECONDS_PER_EPOCH = SLOTS_PER_EPOCH * SECONDS_PER_SLOT; diff --git a/src/storage/BootstrapStorage.sol b/src/storage/BootstrapStorage.sol index 4946aff1..d6a49818 100644 --- a/src/storage/BootstrapStorage.sol +++ b/src/storage/BootstrapStorage.sol @@ -452,6 +452,8 @@ contract BootstrapStorage is GatewayStorage { return true; } + // The bytecode returned by `BEACON_PROXY_BYTECODE` and `EXO_CAPSULE_BEACON` address are actually fixed size of byte array, + // so it would not cause collision for encodePacked // slither-disable-next-line encode-packed-collision function _deployVault(address underlyingToken) internal returns (IVault) { Vault vault = Vault(