GoGoPool contest - ast3ros's results

Liquid staking for Avalanche.

General Information

Platform: Code4rena

Start Date: 15/12/2022

Pot Size: $128,000 USDC

Total HM: 28

Participants: 111

Period: 19 days

Judge: GalloDaSballo

Total Solo HM: 1

Id: 194

League: ETH

GoGoPool

Findings Distribution

Researcher Performance

Rank: 33/111

Findings: 5

Award: $648.40

QA:
grade-b
Gas:
grade-b

🌟 Selected for report: 0

🚀 Solo Findings: 0

Findings Information

🌟 Selected for report: immeas

Also found by: 0xdeadbeef0x, Allarious, Franfran, HollaDieWaldfee, Josiah, RaymondFam, SmartSek, ast3ros, unforgiven

Labels

3 (High Risk)
partial-50
duplicate-493

Awards

242.1694 USDC - $242.17

External Links

Judge has assessed an item in Issue #508 as 3 risk. The relevant finding follows:

[L-05] Duration does not have upper bound The duration input parameter does not have upper bound. If the duration is mistakenly set too high, node operator will be slashed significant amount of GGP. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L198

The expectedAVAXRewardsAmt will be propotional to the duration, leads to high amount of slashGGPAmt.

Recommended Mitigation Steps:

Add the check upper bound for duration.

#0 - c4-judge

2023-02-03T16:51:41Z

GalloDaSballo marked the issue as duplicate of #493

#1 - c4-judge

2023-02-03T16:51:59Z

GalloDaSballo marked the issue as partial-50

#2 - GalloDaSballo

2023-02-03T16:52:12Z

Ultimately shows that the slashing will happen based on duration, but is not as good, so 50%

Awards

21.713 USDC - $21.71

Labels

2 (Med Risk)
satisfactory
duplicate-742

External Links

Judge has assessed an item in Issue #508 as 2 risk. The relevant finding follows:

Cannot add additional Multisig when 10 Multisig addresses are registered

#0 - c4-judge

2023-02-03T16:51:53Z

GalloDaSballo marked the issue as duplicate of #742

#1 - c4-judge

2023-02-08T08:11:42Z

GalloDaSballo marked the issue as satisfactory

Findings Information

🌟 Selected for report: bin2chen

Also found by: 0xbepresent, 0xhunter, RaymondFam, Saintcode_, adriro, ast3ros, cryptonue, immeas

Labels

2 (Med Risk)
satisfactory
duplicate-521

Awards

179.3848 USDC - $179.38

External Links

Judge has assessed an item in Issue #508 as 2 risk. The relevant finding follows:

New address and existing address inputs can be the same in upgradeExistingContract

#0 - c4-judge

2023-02-03T16:51:47Z

GalloDaSballo marked the issue as duplicate of #521

#1 - c4-judge

2023-02-08T08:12:15Z

GalloDaSballo marked the issue as satisfactory

Labels

bug
grade-b
QA (Quality Assurance)
Q-08

Awards

122.8177 USDC - $122.82

External Links

[L-01] Initalize function can be called by anybody

initialize() function can be called anybody when the contract is deployed but not initialized.

More importantly, if someone else runs this function, they can start the rewardsCycleEnd before planning.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L72

Recommended Mitigation Steps:

  • Add a control that makes initialize() only call the by the gurardian.

[L-02] Check upper of the delegationFee

Node operator specified fee (must be between 0 and 1 ether) 2% is 0.2 ether. However, node operator can set without upper bound.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L199

Recommended Mitigation Steps:

  • Add threshold check fee < 1 ether

[L-03] delegationFee is initialized but unused

Node operator specifies delegation fee when creating pool and saves in the storage however it was not used. It could be a potential to missing logic.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L199

Recommended Mitigation Steps:

  • Delete the unused variable or add logic to use it.

[L-04] Input Address is not checked

The address of the receiver token need to be checked with address(0) to avoid locking fund. For example when ClaimProtocolDAO calls the spend function with recipientAddress input.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimProtocolDAO.sol#L22 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L138

Input addresses are not checked, setting address(0) will results in losing 1 in 10 slots for Multisig https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L35

[L-05] Duration does not have upper bound

The duration input parameter does not have upper bound. If the duration is mistakenly set too high, node operator will be slashed significant amount of GGP. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L198

The expectedAVAXRewardsAmt will be propotional to the duration, leads to high amount of slashGGPAmt.

Recommended Mitigation Steps:

  • Add the check upper bound for duration.

[L-06] Inaccurate modifier name

The modifier is named onlyRegisteredNetworkContract when it allows registered network contracts and guardian. The name is misleading and could be used for wrong purpose. In this case, the guardian could set, delete, add or substract any bytes32 storage slot. It raises concerning about the ultimate authority of the account.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Storage.sol#L29

Recommended Mitigation Steps:

  • Change the name of the modifier or remove the guardian to ensure that storages are only be written by the registered contracts.

[L-07] WhenNotPaused modifier does not apply to all external functions that can be called by any account.

Pausing the project should pause all external functions from being called by others to change the state of contracts.

Instance: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L156 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L89 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L273 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L287 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L88

Recommended Mitigation Steps:

  • Add WhenNotPaused modifier to above functions.

[L-08] Cannot add additional Multisig when 10 Multisig addresses are registered

MULTISIG_LIMIT = 10. In case some of them are compromised and the multisig number is 10, there are no way to delete and replace multisig addresses. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L41

Recommended Mitigation Steps:

  • Add a function to remove multisig that can only be called by the guardian or DAO voting.

[L-09] Cannot set price of GGP in AVAX to 0

The price of GGP getting from Oracle cannot be set to 0, even though theoratically it could be 0. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Oracle.sol#L48 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Oracle.sol#L62

[L-10] New address and existing address inputs can be the same in upgradeExistingContract

In upgradeExistingContract, if new address and existing address is the same, the address will be registered then unregistered. It will lead to unexpected behaviour since it is not expected by the caller. https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L214-L215

Recommended Mitigation Steps:

  • Add checking the new address is different to existing address or change the order: unregistered then registered

[L-11] In non-upgradeable contract, initialization should be in constructor instead of initialize function

Instance: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L34-L42

Recommended Mitigation Steps:

  • Put all initialization in constructor

[NC-01] Constants should be defined rather than using magic numbers, comments should be added to explain.

Instance: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/utils/RialtoSimulator.sol#L128

[NC-02] Use address instead of contract type in parameters.

Instance: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Base.sol#L9 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/BaseUpgradeable.sol#L10 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L29 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimProtocolDAO.sol#L15 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L178 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L22 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Oracle.sol#L23 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L19 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L30 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L60 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L41 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L139

[NC-03] Missing natspecs in many functions, or natspecs without parameter explaination

Missing Natspecs or incorrect Natspec format https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L204 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L208 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/utils/RialtoSimulator.sol#L49 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/utils/RialtoSimulator.sol#L53 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/utils/RialtoSimulator.sol#L87 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/utils/RialtoSimulator.sol#L98 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/utils/RialtoSimulator.sol#L127 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L87 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L132 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L153 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L166 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L180 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L191

Missing parameter explaination https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimProtocolDAO.sol#L20 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L35 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L46 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L56 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/utils/RialtoSimulator.sol#L58 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/utils/RialtoSimulator.sol#L76 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L142 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L206 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L216 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L241 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L248 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L255

[NC-04] Remove deadcode and dead comments

Dead code:

No contract is called this function

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L80-L101 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L255

Dead comments:

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/BaseAbstract.sol#L6-L8

[NC-05] Function order does not follow Solidity style guides

In the style guides, the order is: constructor -> receive function (if exists) -> fallback function (if exists) -> external -> public -> internal -> private. There are many instances that order of functions is mixed and it leads to the readability of contracts.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L177 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L156 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L82 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L358 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L204 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L208

[NC-06] Missing Events on State Changing Functions and critical functions:

Events are used by off-chain participants to track on-chain state changes. There are several functions that don't emit events:

Instance:

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L204 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L208 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L287 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L27 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L32 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L37 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L47 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Oracle.sol#L28 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L190 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L198 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L82 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Storage.sol#L41

[NC-07] String is frequently used in code to identity storage slot.

Using string to initialize and access storage slot is error prone due to typo. They are repeatedly use many times and many files. For example string "contract.paused":

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L62 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L68 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L59

Recommended Mitigation Steps:

  • Create a constant file constant.sol and put all strings and numerical constants there. All storage access and initialize should use constants from this file.

[NC-08] Restrict the function call to only appropriate caller

increaseAVAXAssignedHighWater function should has modifier onlySpecificRegisteredContract("MinipoolManager", msg.sender) since it is only called by MinipoolManager

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L133

Recommended Mitigation Steps:

  • Change modifier to: onlySpecificRegisteredContract("MinipoolManager", msg.sender)

[NC-09] Typo in natspecs/comments

Instance:

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L82 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L134 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L67

[NC-10] Checks Effects Interactions pattern is not follow

It is best practice to follow the Checks Effects Interactions pattern in all case to reduce the attack surface for malicious contracts trying to hijack control flow after an external call. In this case, the external call is before state updating.

Instance: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L128

Recommended Mitigation Steps:

  • Move the external call to the last line of the function.

[NC-11] Add getter functions for all added allowed tokens

At the moment, there is no way to know which allowed tokens are added. From administrative perspective, the new guardian cannot know or control the list of allowed tokens if there is a change in personnel in the future.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L39

[NC-12] Indentation in comments are not consistent and follow style guide

Instance: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L87

[NC-13] Too similiar variables

The assets and asset variables are to similar and can be misspelled and difficult to read.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L180-L189

Recommended Mitigation Steps:

  • Change one variable name or add prefix, suffix to distinguish.

[NC-14] Correct naming for avoiding mistakes

ggp should be change to ggpAmount. ggp could be mistake to the address.

event GGPSlashed(address indexed nodeID, uint256 ggp);

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L75

#0 - GalloDaSballo

2023-01-24T16:35:19Z

[L-01] Initalize function can be called by anybody

Invalid

[L-02] Check upper of the delegationFee

L

[L-03] delegationFee is initialized but unused

R

[L-04] Input Address is not checked

L

[L-05] Duration does not have upper bound

Dup 493

[L-06] Inaccurate modifier name

L

[L-07] WhenNotPaused modifier does not apply to all external functions that can be called by any account.

L

[L-08] Cannot add additional Multisig when 10 Multisig addresses are registered

Dup 521

[L-09] Cannot set price of GGP in AVAX to 0

Invalid

[L-10] New address and existing address inputs can be the same in upgradeExistingContract

Dup 742

[L-11] In non-upgradeable contract, initialization should be in constructor instead of initialize function

Invalid, The sponsor may want the flexibility to start distribution after X time (for example for public scrutiny or bug bounty)

[NC-01] Constants should be defined rather than using magic numbers, comments should be added to explain.

R

[NC-02] Use address instead of contract type in parameters.

Invalid / off

[NC-03] Missing natspecs in many functions, or natspecs without parameter explaination

NC

[NC-04] Remove deadcode and dead comments

NC

[NC-05] Function order does not follow Solidity style guides

NC

[NC-06] Missing Events on State Changing Functions and critical functions:

NC

[NC-07] String is frequently used in code to identity storage slot.

R

[NC-08] Restrict the function call to only appropriate caller

Invalid

[NC-09] Typo in natspecs/comments

NC

[NC-10] Checks Effects Interactions pattern is not follow

L

[NC-11] Add getter functions for all added allowed tokens

R

[NC-12] Indentation in comments are not consistent and follow style guide

NC

[NC-13] Too similiar variables

Invalid

[NC-14] Correct naming for avoiding mistakes

NC

Am going to penalize due to too many incorrect reports, I recommend you focus on high quality high accuracy reports

#1 - GalloDaSballo

2023-02-03T16:53:31Z

5L 4R 7NC

#2 - c4-judge

2023-02-03T22:09:56Z

GalloDaSballo marked the issue as grade-b

Findings Information

Labels

bug
G (Gas Optimization)
grade-b
G-05

Awards

82.3208 USDC - $82.32

External Links

[G-1] Slot packing saves slots but increases runtime gas consumption due to masking

Packed fields into a slot save storage in deployment but costs more gas in runtime due to masking, especially in case variables are not in a struct and accessed separately.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L50

Recommended Mitigation Steps

  • Use default word size 256 to avoid masking at runtime and save gas.

[GAS-2] Use calldata instead of memory for function arguments that do not get mutated

Instance https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/BaseAbstract.sol#L32 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/BaseAbstract.sol#L90

[GAS-3] Using private state variable to save gas and be consistent

newGuardian variable is declared as public. However it is unnecessary and can be changed to private to save gas. If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.

It is also consistent with the visibility of guardian variable.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Storage.sol#L25

[GAS-4] State variables should be cached in stack variables rather than re-reading them from storage

Instance:

tokenBalances[contractKey] variable:

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L151 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L155

rewardsCycleLength:

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L102

[GAS-5] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Instance https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L61

[GAS-6] Unneccessary event emitting in constructor

In constructor, it is not neccessary to emit the event to inform the GuardianChanged since it it not a change but an initialization. Removing it can save gas in deployment.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Storage.sol#L36

[GAS-7] Check validity of a variable early to save gas if the function fails

The check (tokenBalances[contractKeyFrom] < amount) could be performed at line 179. If it fails then the gas is saved from 2 operations: keccak256 and TokenTransfer event emission

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Vault.sol#L183-L185

[GAS-8] Unchange state variable should be declared constant to save gas

The rewardsCycleLength variable is set in initialize function and never changed in the contract. It should be constant to save gas.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L76

#0 - GalloDaSballo

2023-01-27T15:13:09Z

rewardsCycleLength 2300

#1 - c4-judge

2023-02-03T19:12:28Z

GalloDaSballo marked the issue as grade-b

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter