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
Rank: 33/111
Findings: 5
Award: $648.40
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: immeas
Also found by: 0xdeadbeef0x, Allarious, Franfran, HollaDieWaldfee, Josiah, RaymondFam, SmartSek, ast3ros, unforgiven
242.1694 USDC - $242.17
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%
🌟 Selected for report: gz627
Also found by: AkshaySrivastav, Allarious, Czar102, HE1M, HollaDieWaldfee, KmanOfficial, adriro, ast3ros, betweenETHlines, bin2chen, brgltd, cccz, chaduke, hihen, imare, mookimgo, neumo, nogo, peanuts, unforgiven
21.713 USDC - $21.71
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
🌟 Selected for report: bin2chen
Also found by: 0xbepresent, 0xhunter, RaymondFam, Saintcode_, adriro, ast3ros, cryptonue, immeas
179.3848 USDC - $179.38
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
🌟 Selected for report: IllIllI
Also found by: 0xSmartContract, AkshaySrivastav, HollaDieWaldfee, RaymondFam, Rolezn, ast3ros, betweenETHlines, brgltd, chaduke, codeislight, cozzetti, lukris02, pauliax, sces60107
122.8177 USDC - $122.82
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.
Recommended Mitigation Steps:
Node operator specified fee (must be between 0 and 1 ether) 2% is 0.2 ether. However, node operator can set without upper bound.
Recommended Mitigation Steps:
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.
Recommended Mitigation Steps:
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
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:
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.
Recommended Mitigation Steps:
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:
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:
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
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:
Recommended Mitigation Steps:
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
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
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:
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
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
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:
increaseAVAXAssignedHighWater function should has modifier onlySpecificRegisteredContract("MinipoolManager", msg.sender) since it is only called by MinipoolManager
Recommended Mitigation Steps:
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
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.
Recommended Mitigation Steps:
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.
The assets and asset variables are to similar and can be misspelled and difficult to read.
Recommended Mitigation Steps:
ggp should be change to ggpAmount. ggp could be mistake to the address.
event GGPSlashed(address indexed nodeID, uint256 ggp);
#0 - GalloDaSballo
2023-01-24T16:35:19Z
Invalid
L
R
L
Dup 493
L
L
Dup 521
Invalid
Dup 742
Invalid, The sponsor may want the flexibility to start distribution after X time (for example for public scrutiny or bug bounty)
R
Invalid / off
NC
NC
NC
NC
R
Invalid
NC
L
R
NC
Invalid
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
🌟 Selected for report: NoamYakov
Also found by: AkshaySrivastav, IllIllI, ast3ros, betweenETHlines, c3phas, camdengrieh, chaduke, fatherOfBlocks, kartkhira, latt1ce, shark
82.3208 USDC - $82.32
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.
Recommended Mitigation Steps
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
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.
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:
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.
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
The rewardsCycleLength variable is set in initialize function and never changed in the contract. It should be constant to save gas.
#0 - GalloDaSballo
2023-01-27T15:13:09Z
rewardsCycleLength 2300
#1 - c4-judge
2023-02-03T19:12:28Z
GalloDaSballo marked the issue as grade-b