GoGoPool contest - betweenETHlines's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L165 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ClaimNodeOp.sol#L77

Vulnerability details

Staking.sol / ClaimNodeOp.sol

Impact

resetAvaxAssignedHighWater can be frontrun to increase rewards

Proof of Concept

Currently, the function resetAvaxAssignedHighWater is called in claimNodeOp during calculateAndDistributeRewards. This function sets avaxAssignedHighWaterto the aggregate of allavaxAssigned`.

A malicious party can therefore recognize the function-call calculateAndDistributeRewards in the mempool and call createMinipool with higher gas than the aforementioned call. This will result in avaxAssigned increased which will then increase avaxAssignedHighWater in the following call. Moreover, the malicious party can simply call cancelMinipool immediately afterwards (the grace period can be circumvented if the attacker has other regular minipools) which will result in status quo for the attacker but with increased avaxAssignedHighWater. We think that this is a valid scenario which is unwanted by the protocol owner because this is an unnatural increase of avaxAssignedHighWater which will assign more rewards to the attacker than he deserves.

Tools Used

VSCode; manual inspection

Recommended Mitigation Steps

While it is hard to mitigate this attack vector, it might make sense to reconsider the whole logic regarding avaxAssignedHighWater, it might be not ideal to include the assigned avax from all minipools, if they are not currently running.

*This is the first contest i took part, therefore my submissions might not be ideal yet, if there are any questions feel free to contact me directly in discord :-)

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ProtocolDAO.sol#L193

Vulnerability details

Impact

Two registered addresses can point to the same name which can result in collisions

Proof of Concept

Currently, the registerContract function does not check if names attached to addresses are unique, it allows to use the same name for several addresses. While this at this point doesn't really sound like an issue, consider following scenario:

1. Contract A is registered with name Breaker
2. Contract B is also registered with name Breaker
3. The following line will override name -> address : setAddress(keccak256(abi.encodePacked("contract.address", name)), addr);
4. However, the next line will not override it but simply set the address of contract B pointing to the same name as Contract A: setAddress(keccak256(abi.encodePacked("contract.address", name)), addr);.

Now consider following scenario as well:

1)Contract A calls withdrawAvax in vault.sol; it will then account the balance change based on the name which is assigned to contract A:

string memory contractName = getContractName(msg.sender);

avaxBalances[contractName] = avaxBalances[contractName] - amount;

2. Contract B calls the same function

This results in contract A and contract B both using the same slot for saving their contract balance, eventually resulting in collisions.

Tools Used

VSCode

Recommended Mitigation Steps

Consider only allowing unique names for contracts, reverting the registerContract function if the name is already occupied.

*This is the first contest i took part, therefore my submissions might not be ideal yet, if there are any questions feel free to contact me directly in discord :-)

Lines of code

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

Vulnerability details

Impact

If a multisig address suddenly turns malicious, there is no quick and easy way to change the assignment to minipools

Proof of Concept

Currently, a multisig address is automatically assigned to a newly created minipool: https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L260

This address can eventually misbehave like stealing funds via claimAndInitiateStaking. However, this fact itself is not the issue, the issue is that the contract cannot easily change the assigned multisig address for the minipool. It is only possible via creating a new contract, registering it as network contract and then call setAddress with the correct parameters. However, it would make more sense to directly implement a function like that in case of emergency.

Tools Used

VSCode

Recommended Mitigation Steps

Consider implementing a function which allows the guardian to change the multisig address assigned to minipools.

Lines of code

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

Vulnerability details

Impact

Currently, recreateMinipool and claimAndInitiateStaking can be called by the multisig, even if the node operator already withdrew his funds.

Proof of Concept

1. nodeOp calls withdrawMinipoolFunds and receives his stake + rewards.
2. multisig calls recreateMinipool which assigns all necessary variables:

setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), compoundedAvaxNodeOpAmt); setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), compoundedAvaxNodeOpAmt);

3. multisig calls claimAndInitiateStaking which withdraws all necessary tokens from ggAvax and the vault and starts the staking process. However, the nodeOp has not even provided any AVAX, since it was already withdrawn within withdrawMinipoolFunds.

Therefore the nodeOp gets a stake granted without even providing the necessary tokens.

Tools Used

VSCode

Recommended Mitigation Steps

Consider rethinking about the logic and state transition itself and eventually disallow the state transition from finished -> prelaunch. However, if that is limited, it needs to be careful validated if the contract still works.

*This is the first contest i took part, therefore my submissions might not be ideal yet, if there are any questions feel free to contact me directly in discord :-)

Lines of code

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

Vulnerability details

Impact

The wait period requirement for cancelling minipools can and will be circumvented

Proof of Concept

Currently, the function cancelMinipool checks if the getRewardsStartTime is far enough in the past to meet the cancel moratorium: https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L279. However, this check is fundamentally flawed because it only checks the time when the first minipool was created. All future minipools can be immediately cancelled if that time passes for the first minipool.

This vulnerability can be abused to frontrun claimAndDistribute, increase avaxAssignedHighWater and immediately cancel the minipool again. However, this issue is described seperately.

Tools Used

VSCode

Recommended Mitigation Steps

Consider implementing a separate startTime variable, based on the nodeId and execute the check on this variable.

1. Unused error messages:

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Vault.sol#L26 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Vault.sol#L27 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Vault.sol#L28

Consider removing them.

2. Allowed tokens are not easily inspectable by users

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

Consider adding them into a enumerableSet and add a view functions.

3. Usage of += / -= for cleaner code

Throughout the codebase

Example: https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Vault.sol#L100

Consider using the += / -= style.

4. Unused function `transferAvax``

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

Consider removing the unused function as it exposes an unnecessary governance privilege.

5. Checks-effects-interaction pattern not respected

Throughout the codebase

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

Consider rewriting the codebase to respect the checks-effects-interactions pattern.

6. Non-zero validation for withdrawToken

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

Consider checking that withdrawalAddress is not the zero address. (It would revert for erc20 standard tokens, but for custom token it might even succeed)

7. Byte storage, int storage as well as the assigned setters are unused

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

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

Consider removing them, if there is no plan to use it in the future.

8. Non-zero validation missing for setGuardian

Currently, the setGuardian function lacks a non-zero validation

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

We acknowledge that this can not become an issue at any time due to the two-step initialization, however, we always recommend a non-zero validation for setter functions because its considered as best-practice.

Consider validating the input address.

9. Unused import MinipoolManager

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

Consider removing the unused import to keep the contract size reasonable.

10. Sub-optimal design for token inflation

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/RewardsPool.sol#L82

Currently, the token inflation is not happening via minting, it is just a vesting-like process. However, this might confuse regular users because the totalSupply will always be display with 21_000_000 tokens.

While we assume that this is the intended logic, we still feel like we need to point this out.

11. Multisig reward distribution rounds down

Currently, the multisig reward distribution rounds down which eventually results in a very small loss of rewards:

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/RewardsPool.sol#L229

Consider adding a multiplier for this arithmetic calculation to decrease the down-rounding impact.

12. Unused import: TokenGGP

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ProtocolDAO.sol#L5

Consider removing this import

13. Missing non-zero validation for setOneInch

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Oracle.sol#L28

Consider validating the input.

14. Users can not easily inspect the list of added Defenders

Currently, there is no easy way for users to inspect the list of Defenders

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Ocyticus.sol#L27

Consider adding an enumerableSet.addressSet for the Defenders.

15. avaxHalfRewards is rounded down

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Ocyticus.sol#L27

the following line of code is rounded down which results in some dust.

Consider executing this operation with an appropiate multiplier.

1. rewardsCycleLength can be made constant

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

Since this variable is initialized and never changed, this can simply be made constant which makes its init redundant and saves gas.

2. emitGuardianChanged can be emitted without caching

If the event is emitted at the beginning of the function it can simply be done using the parameters (guardian, newGuardian). This will save gas because address oldguardian does not need to be cached into memory.

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