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

CI: Aderyn Analysis #87

Open
wants to merge 9 commits into
base: mainnet
Choose a base branch
from
Open

CI: Aderyn Analysis #87

wants to merge 9 commits into from

Conversation

tringuyenskymavis
Copy link
Contributor

Description

Checklist

  • I have clearly commented on all the main functions following the NatSpec Format
  • The box that allows repo maintainers to update this PR is checked
  • I tested locally to make sure this feature/fix works

@tringuyenskymavis tringuyenskymavis changed the title chore: test ci CI: Aderyn Analysis Nov 13, 2024
@ronin-chain ronin-chain deleted a comment from github-actions bot Nov 13, 2024
@ronin-chain ronin-chain deleted a comment from github-actions bot Nov 13, 2024
@ronin-chain ronin-chain deleted a comment from github-actions bot Nov 13, 2024
Copy link

github-actions bot commented Nov 13, 2024

Aderyn Analysis Report

This report was generated by Aderyn, a static analysis tool built by Cyfrin, a blockchain security company. This report is not a substitute for manual audit or security review. It should not be relied upon for any purpose other than to assist in the identification of potential security vulnerabilities.

Table of Contents

Summary

Files Summary

Key Value
.sol Files 39
Total nSLOC 3108

Files Details

Filepath nSLOC
src/extensions/GatewayV3.sol 56
src/extensions/MinimumWithdrawal.sol 25
src/extensions/RONTransferHelper.sol 18
src/extensions/TransparentUpgradeableProxyV2.sol 15
src/extensions/WethUnwrapper.sol 41
src/extensions/WithdrawalLimitation.sol 143
src/extensions/bridge-operator-governance/BridgeManager.sol 241
src/extensions/bridge-operator-governance/BridgeManagerCallbackRegister.sol 66
src/extensions/bridge-operator-governance/BridgeManagerQuorum.sol 45
src/extensions/bridge-operator-governance/BridgeTrackingHelper.sol 19
src/extensions/collections/HasContracts.sol 34
src/extensions/collections/HasProxyAdmin.sol 16
src/extensions/sequential-governance/CoreGovernance.sol 171
src/extensions/sequential-governance/GlobalCoreGovernance.sol 79
src/extensions/sequential-governance/governance-proposal/CommonGovernanceProposal.sol 73
src/extensions/sequential-governance/governance-proposal/GlobalGovernanceProposal.sol 39
src/extensions/sequential-governance/governance-proposal/GovernanceProposal.sol 46
src/extensions/sequential-governance/governance-relay/CommonGovernanceRelay.sol 71
src/extensions/sequential-governance/governance-relay/GlobalGovernanceRelay.sol 18
src/extensions/sequential-governance/governance-relay/GovernanceRelay.sol 16
src/mainchain/MainchainBridgeManager.sol 67
src/mainchain/MainchainGatewayBatcher.sol 47
src/mainchain/MainchainGatewayV3.sol 316
src/ronin/gateway/BridgeReward.sol 201
src/ronin/gateway/BridgeSlash.sol 169
src/ronin/gateway/BridgeTracking.sol 213
src/ronin/gateway/PauseEnforcer.sol 63
src/ronin/gateway/RoninBridgeManager.sol 142
src/ronin/gateway/RoninBridgeManagerConstructor.sol 51
src/ronin/gateway/RoninGatewayV3.sol 341
src/tokens/erc20/WBTC.sol 25
src/tokens/erc20/WBTC_Sepolia.sol 10
src/types/Types.sol 17
src/types/operations/LibTUint256Slot.sol 83
src/utils/CommonErrors.sol 48
src/utils/ContractType.sol 19
src/utils/DeprecatedSlots.sol 7
src/utils/IdentityGuard.sol 43
src/utils/RoleAccess.sol 14
Total 3108

Issue Summary

Category No. of Issues
High 6
Low 0

High Issues

H-1: Unsafe Casting

Downcasting int/uints in Solidity can be unsafe due to the potential for data loss and unintended behavior.When downcasting a larger integer type to a smaller one (e.g., uint256 to uint128), the value may exceed the range of the target type,leading to truncation and loss of significant digits. Use OpenZeppelin's SafeCast library to safely downcast integers.

1 Found Instances
  • Found in src/ronin/gateway/BridgeSlash.sol Line: 99

H-2: Uninitialized State Variables

Solidity does initialize variables by default when you declare them, however it's good practice to explicitly declare an initial value. For example, if you transfer money to an address we must make sure that the address has been initialized.

14 Found Instances
  • Found in src/extensions/GatewayV3.sol Line: 17
  • Found in src/extensions/GatewayV3.sol Line: 18
  • Found in src/mainchain/MainchainGatewayV3.sol Line: 37
  • Found in src/mainchain/MainchainGatewayV3.sol Line: 39
  • Found in src/mainchain/MainchainGatewayV3.sol Line: 48
  • Found in src/mainchain/MainchainGatewayV3.sol Line: 50
  • Found in src/mainchain/MainchainGatewayV3.sol Line: 55
  • Found in src/ronin/gateway/RoninGatewayV3.sol Line: 35
  • Found in src/ronin/gateway/RoninGatewayV3.sol Line: 37
  • Found in src/ronin/gateway/RoninGatewayV3.sol Line: 50
  • Found in src/ronin/gateway/RoninGatewayV3.sol Line: 52
  • Found in src/ronin/gateway/RoninGatewayV3.sol Line: 58
  • Found in src/utils/DeprecatedSlots.sol Line: 13
  • Found in src/utils/DeprecatedSlots.sol Line: 18

H-3: Yul block contains return function call.

Remove this, as this causes execution to halt. Nothing after that call will execute, including code following the assembly block.

2 Found Instances
  • Found in src/extensions/TransparentUpgradeableProxyV2.sol Line: 27
  • Found in src/ronin/gateway/BridgeTracking.sol Line: 63

H-4: Functions send eth away from contract but performs no checks on any address.

Consider introducing checks for msg.sender to ensure the recipient of the money is as intended.

2 Found Instances
  • Found in src/extensions/WethUnwrapper.sol Line: 27
  • Found in src/extensions/WethUnwrapper.sol Line: 32

H-5: Return value of the function call is not checked.

Function returns a value but it is ignored.

15 Found Instances
  • Found in src/extensions/bridge-operator-governance/BridgeManager.sol Line: 84
  • Found in src/extensions/bridge-operator-governance/BridgeManager.sol Line: 217
  • Found in src/extensions/bridge-operator-governance/BridgeManager.sol Line: 224
  • Found in src/extensions/sequential-governance/governance-proposal/GovernanceProposal.sol Line: 19
  • Found in src/extensions/sequential-governance/governance-proposal/GovernanceProposal.sol Line: 54
  • Found in src/extensions/sequential-governance/governance-relay/GovernanceRelay.sol Line: 24
  • Found in src/mainchain/MainchainGatewayV3.sol Line: 96
  • Found in src/ronin/gateway/BridgeReward.sol Line: 196
  • Found in src/ronin/gateway/BridgeReward.sol Line: 235
  • Found in src/ronin/gateway/RoninBridgeManager.sol Line: 44
  • Found in src/ronin/gateway/RoninBridgeManager.sol Line: 102
  • Found in src/ronin/gateway/RoninBridgeManager.sol Line: 166
  • Found in src/ronin/gateway/RoninBridgeManager.sol Line: 212
  • Found in src/ronin/gateway/RoninGatewayV3.sol Line: 109
  • Found in src/ronin/gateway/RoninGatewayV3.sol Line: 439

H-6: Contract locks Ether without a withdraw function.

It appears that the contract includes a payable function to accept Ether but lacks a corresponding function to withdraw it, which leads to the Ether being locked in the contract. To resolve this issue, please implement a public or external function that allows for the withdrawal of Ether from the contract.

4 Found Instances
  • Found in src/extensions/TransparentUpgradeableProxyV2.sol Line: 6
  • Found in src/mainchain/MainchainGatewayV3.sol Line: 16
  • Found in src/ronin/gateway/BridgeSlash.sol Line: 20
  • Found in src/ronin/gateway/RoninGatewayV3.sol Line: 19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant