🔐Zellic Audit
GoGoPool
Smart Contract Security Assessment
February 22, 2023
Prepared for: Multisig Labs
Prepared by: Katerina Belotskaia and Vlad Toie
Zellic Inc.
Contents
About Zellic
Zellic was founded in 2020 by a team of blockchain specialists with more than a decade of combined industry experience. We are leading experts in smart contracts and Web3 development, cryptography, web security, and reverse engineering. Before Zellic, we founded perfect blue, the top competitive hacking team in the world. Since then, our team has won countless cybersecurity contests and blockchain security events.
Zellic aims to treat clients on a case-by-case basis and to consider their individual, unique concerns and business needs. Our goal is to see the long-term success of our partners rather than simply provide a list of present security issues. Similarly, we strive to adapt to our partners’ timelines and to be as available as possible. To keep up with our latest endeavors and research, check out our website zellic.io or follow @zellic_io on Twitter. If you are interested in partnering with Zellic, please email us at hello@zellic.io or contact us on Telegram at https://t.me/zellic_io.
1 Executive Summary
Zellic conducted an audit for Multisig Labs from November 14th to 29th, 2022.
Our general overview of the code is that it was well-organized and structured. The code coverage is high, and tests are included for the majority of the functions. Some areas of the code have limited negative testing, which could be improved. The documentation was adequate, although it could be improved. The code was easy to comprehend, and in most cases, intuitive.
Zellic thoroughly reviewed the GoGoPool codebase to find protocol-breaking bugs as defined by the documentation and to find any technical issues outlined in the Methodology section (2.2) of this document.
Specifically, taking into account GoGoPool’s threat model, we focused heavily on issues that would break core invariants such as the management of the minipools, staking, withdrawing and minting shares, and the states of the Storage contract.
During our assessment on the scoped GoGoPool contracts, we discovered seven findings. Of the seven findings, four were of high severity, one was of medium severity, one was of low severity and the remaining finding was informational.
Additionally, Zellic recorded its notes and observations from the audit for Multisig Labs’s benefit in the Discussion section at the end of the document.
Breakdown of Finding Impacts
Critical
0
High
4
Medium
1
Low
1
Informational
1
2 Introduction
2.1 About GoGoPool
GoGoPool allows Avalanche users to stake a minimum of 0.01 AVAX and operate a validator node with a minimum of 1000 AVAX, while providing instant liquidity and earning rewards for validating subnets. As an open protocol, any individual, business, or subnet can plug into the protocol without being charged platform fees.
2.2 Methodology
During a security assessment, Zellic works through standard phases of security auditing including both automated testing and manual review. These processes can vary significantly per engagement, but the majority of the time is spent on a thorough manual review of the entire scope.
Alongside a variety of open-source tools and analyzers used on an as-needed basis, Zellic focuses primarily on the following classes of security and reliability issues:
Basic coding mistakes. Many critical vulnerabilities in the past have been caused by simple, surface-level mistakes that could have easily been caught ahead of time by code review. We analyze the scoped smart contract code using automated tools to quickly sieve out and catch these shallow bugs. Depending on the engagement, we may also employ sophisticated analyzers such as model checkers, theorem provers, fuzzers, and so forth as necessary. We also perform a cursory review of the code to familiarize ourselves with the contracts.
Business logic errors. Business logic is the heart of any smart contract application. We manually review the contract logic to ensure that the code implements the expected functionality as specified in the platform’s design documents. We also thoroughly examine the specifications and designs themselves for inconsistencies, flaws, and vulnerabilities. This involves use cases that open the opportunity for abuse, such as flawed tokenomics or share pricing, arbitrage opportunities, and so forth.
Complex integration risks. Several high-profile exploits have not been the result of any bug within the contract itself; rather, they are an unintended consequence of the contract’s interaction with the broader DeFi ecosystem. We perform a meticulous review of all of the contract’s possible external interactions and summarize the associated risks: for example, flash loan attacks, oracle price manipulation, MEV/sandwich attacks, and so forth.
Codematurity. We review for possible improvements in the codebase in general. We look for violations of industry best practices and guidelines and code quality standards. We also provide suggestions for possible optimizations, such as gas optimization, upgradeability weaknesses, centralization risks, and so forth.
For each finding, Zellic assigns it an impact rating based on its severity and likelihood. There is no hard-and-fast formula for calculating a finding’s impact; we assign it on a case-by-case basis based on our professional judgment and experience. As one would expect, both the severity and likelihood of an issue affect its impact; for instance, a highly severe issue’s impact may be attenuated by a very low likelihood. We assign the following impact ratings (ordered by importance): Critical, High, Medium, Low, and Informational.
Similarly, Zellic organizes its reports such that the most important findings come first in the document rather than being ordered on impact alone. Thus, we may sometimes emphasize an “Informational” finding higher than a “Low” finding. The key distinction is that although certain findings may have the same impact rating, their importance may differ. This varies based on numerous soft factors, such as our clients’ threat models, their business needs, their project timelines, and so forth. We aim to provide useful and actionable advice to our partners that consider their long-term goals rather than simply provide a list of security issues at present.
2.3 Scope
The engagement involved a review of the following targets:
GoGoPool Contracts
Repository: https://github.com/multisig-labs/gogopool-contracts
Versions: 7768287e94bff0f2e12f03427309777e82a6e2fc
Contracts:
BaseAbstract.sol
RewardsPool.sol
BaseUpgradeable.sol
MultisigManager.sol
Oracle.sol
MinipoolManager.sol
Vault.sol
Storage.sol
Base.sol
ProtocolDAO.sol
Ocyticus.sol
tokens/TokenggAVAX.sol
tokens/TokenGGP.sol
tokens/upgradeable/ERC20Upgradeable.sol
tokens/upgradeable/ERC4626Upgradeable.sol
ClaimProtocolDAO.sol
ClaimNodeOp.sol
Staking.sol
Type: Solidity
Platform: EVM-compatible
2.4 Project Overview
Zellic was contracted to perform a security assessment with two consultants for a total of four person-weeks. The assessment was conducted over the course of two calendar weeks.
Contact Information
The following project managers were associated with the engagement:
The following consultants were engaged to conduct the assessment:
2.5 Project Timeline
The key dates of the engagement are detailed below.
3 Detailed Findings
3.1 The transferAVAX
function allows arbitrary transfers
transferAVAX
function allows arbitrary transfersTarget : Vault.sol
Category : Business Logic
Likelihood : Medium
Severity : High
Impact : High
Description
The transferAVAX
function is used to perform transfers of avax between two registered contracts.
The current checks ensure that the msg.sender
is a registeredNetworkContract
; however, the function lacks a check on whether the msg.sender
actually calls the function or not.
Impact
Due to the fact that fromContractName
can be an arbitrary address, a presumably malicious registeredNetworkCContract
can drain the avax balances of all the other registered contracts.
Recommendations
We recommend removing the fromContractName
parameter altogether and ensuring that the funds can only be transferred by the caller of the function, msg.sender
.
Remediation
The issue has been fixed by Multisig Labs in commit 84211f.
3.2 Ocyticus does not include the Staking pause
Target : Ocyticus, Staking
Category : Business Logic
Likelihood : Medium
Severity : High
Impact : High
Description
The pauseEverything
and resumeEverything
functions are used to restrict access to important functions.
Apart from the TokenGGAvax
and MinipoolManager
, the Staking
contract also makes use of the whenNotPaused
modifier for its important functions. The paused state, will, however, not trigger at the same time with the pauseEverything
call, since the Staking
contract is omitted here, both for pausing and resuming.
Impact
Should an emergency arise, pauseEverything
will be called. In this case, Staking
will be omitted, which could put user funds in danger.
Recommendations
We recommend ensuring that the Staking
contract is also paused in the pauseEverything
function as well as un-paused in the resumeEverything
function.
Remediation
The issue has been fixed by Multisig Labs in commit dbc499.
3.3 The reward amount manipulation.
Target : ClaimNodeOp.sol
Category : Business Logic
Likelihood : Medium
Severity : High
Impact : High
Descriptions
A staker is eligible for the upcoming rewards cycle if they have staked their tokens for a long enough period of time. The reward amount is distributed in proportion to the amount of funds staked by the user from the total amount of funds staked by all users who claim the reward. But since the rewardsStartTime
is the time of creation of only the first pool, and during the reward calculations all staked funds are taken into account, even if they have not yet been blocked and can be withdrawn, the attack described below is possible.
The attack scenario:
An attacker stakes ggp tokens and creates a minipool with a minimum
avaxAssignmentRequest
value.The multisig initiates the staking process by calling the
claimAndInitiateStaking
function.Wait for the time of distribution of rewards.
Before the reward distribution process begins, the attacker creates a new minipool with the maximum
avaxAssignmentRequest
value.Initiate the reward distribution process.
Immediately after that, the attacker cancels the minipool with
cancelMinipool
function before theclaimAndInitiateStaking
function call and returns most part of their staked funds.
Impact
The attacker can increase their reward portion without actually staking their own funds.
Recommendations
Take into account only the funds actually staked, or check that all minipools have been launched.
Remediation
The issue has been fixed by Multisig Labs in commits c90b2f and f49931.
3.4 Network registered contracts have absolute storage control.
Target : Project-wide
Category : Business Logic
Likelihood : Low
Severity : High
Impact : High
Description
The network-registered contracts have absolute control over the storage that all the contracts are associated with through the Storage
contract. This is inherent due to the overall design of the protocol,which makes use of a single Storage
contract eliminating the need of local storage. For that reason any registeredContract
can update
any storage slot even if it “belongs” to another contract.
As an example, the setter functions inside the Staking
contract have different restrictions for caller (e.g., the setLastRewardsCycleCompleted
function can be called only by ClaimNodeOp
contract), but actually the setUint
function from it may be called by any RegisteredNetworkContract
.
Impact
We believe that in a highly unlikely case,a malicious networkRegistered
contract could potentially alter the entire protocol Storage
to their will. Additionally, if it were possible to setBool
of an arbitrary address, then this scenario would be further exploitable by a malicious developer contract.
Recommendations
We recommend paying extra attention to the registration of networkContracts
, as well as closely monitoring where and when the setBool
function is used, since the network registration is based on a boolean value attributed to the contract address.
Remediation
The issue has ben acknowledged by the Multisig Labs. Their official reply is reproduced below:
While it is true that any registered contract can write to Storage, we view all of the separate contracts comprising the Protocol as a single system. A single entity (either the Guardian Multisig or in future the ProtocolDAO) will be in control of all of the contracts. In this model, if an attacker can register a single malicious contract, then they are also in full control of the Protocol itself. Because all of the contracts are treated as a single entity, there is no additional security benefit to be gained by providing access controls between the various contract’s storage slots. As a mitigation, the Protocol will operate several distributed Watchers that will continually scan the central Storage contract, and alert on any changes.
3.5 Oracle may reflect an outdated price
Target : Oracle
Category : Business Logic
Likelihood : Medium
Severity : Medium
Impact : Medium
Description
Some functions at protocol-level make use of the getGGPPriceInAvax
. This getter retrieves the price
, which is set by the Rialto
multisig.
Due to the nature of on-chain price feeds, Oracles
need to have an as-often-as-possible policy in regards to how often the price gets updated. For that reason, the reliance on the Rialto
may be problematic should it fail to update the price
often enough.
Impact
Should the price be erroneous, possible front-runs may happen at the protocol level, potentially leading to a loss of funds on the user-end side.
Recommendations
We recommend implementing a slippage check, which essentially does not allow a price to be used should it have been updated more than x blocks ago.
Remediation
The finding has been acknowledged by the Multisig Labs team. Their official reply is reproduced below:
The price of GGP is used in the Protocol to determine collateralization ratios for minipools as well as slashing amounts. If the price of GGP is unknown or out- dated, the protocol cannot operate. So our remediation for this will be to have a distributed set of Watchers that will Pause the Protocol if the GGP Price becomes outdated. At some point in the future the Protocol will use on-chain TWAP price oracles to set the GGP price.
3.6 Fields are not reset exactly after their usage
Target : MinipoolManager
Category : Business Logic
Likelihood : Low
Severity : Low
Impact : Low
Description
Due to the nature of the protocol, some fields are queried and used in one intermediary state of the application and then reset in the last state of the application. As an example, see the avaxNodeOpRewardAmt
value, which is queried and used in withdrawMinipoolFunds
(which can only be called in the WITHDRAWABLE
stage)
and then either reset in the recordStakingEnd
function, to the new rounds’ avaxNodeOpRewardAmt
, or set to 0 in recordStakingError
.
The protocol’s structure assumes that the way in which the states are transitioned through is consistent.
Impact
Should major changes occur in the future of the protocol,we suspect that some states that are presumably reset in an eventual state of the protocol may be omitted. This could in turn lead to unexpected consequences to the management of the minipool.
Recommendations
We highly recommend that once important storage states are used, they should also be reset. In this way, future versions of the protocol will have a solid way of transitioning without requiring additional synchronization of storage state.
Remediation
The issue has ben acknowledged by the Multisig Labs. Their official reply is reproduced below:
The Protocol maintains some fields in
Storage
so that data such asavaxNodeOpRewardAmt
can be displayed to the end user. The fields will be reset if the user relaunches a minipool with the samenodeID
again in the future. This is by design.
3.7 Contracts can deposit arbitrary tokens in the Vault
Target : Vault.sol
Category : Business Logic
Likelihood : Medium
Severity : Low
Impact : Informational
Description
Multiple functions from the Vault
contract allow arbitrary tokens to be deposited and withdrawn by networkRegistered
contracts. For example, see the depositToken
function:
Impact
As per the current implementation, there are no security implications. However, we consider that the Vault
plays an essential role in the entire protocol, and thus we highly recommend fixing this issue for posterity.
Recommendations
Upon discussions with the Multisig Lab team, we settled that the best mitigation is whitelisting
the tokenContract
that are used in each function. This further allows flexibility and security in smoothly upgrading the Vault
should it support more tokens. In that case, the mitigated version of the function could be:
Remediation
The issue has been fixed by Multisig Labs in commit 644e8e.
4 Discussion
The purpose of this section is to document miscellaneous observations that we made during the assessment.
4.1 The rewardsCycleEnd
calculation.
rewardsCycleEnd
calculation.The rewardsCycleEnd
value from the TokenggAVAX
contract should always be evenly divisible by rewardsCycleLength
. This condition, however, is only met during the contract initialization, where the rewardsCycleLeng
this initially calculated. The rewardsCycleLength
is eventually recalculated inside the syncRewards
function, but this time, there is no check whether the value is evenly divisible or not.
Remediation
The issue has been fixed by Multisig Labs in commit 556ac4.
4.2 Lack of checks.
The
calculateAndDistributeRewards
function from theClaimNodeOp
contract does not explicitly verify that thestakerAddr
is a valid staker address.Add a check that
rewardsPool.getRewardsCycleCount()
is not zero to thecalculateAndDistributeRewards
function from theClaimNodeOpcontract
.The
registerMultisig
function in theMultisigManager
contract does not check that themultisig.count
value has reached 10 to ensure thatThere will never be more than 10 total multisigs
, which is a comment on therequireNextActiveMultisig
function.The
recordStakingStart
function in theMinipoolManager
contract does not validate that thestartTime
value is not greater than the current time.The set
RewardsStartTime
function in theStaking
contract does not validate that thetime
value is not greater than the current time or that it can be only the current time or 0.The
getInflationAmt
in theRewardsPool
contract does not process the case when the max amount of tokens are released (22_500_000, the total minted amount).
Remediation
The issue has been fixed by Multisig Labs in commit 878b2e.
4.3 The process of distributing ggp rewards
In order to receive a reward the staker must be registered for the required amount of time. But the current implementation of the protocol allows users to stake most of the funds immediately before distribution of the reward. The isEligible
function verifies that the staker should be registered at least ProtocolDAO
. RewardsEligibilityMinSeconds
amount of seconds before the rewards cycle starts (this happens after the first minipool is created), but this check takes into account only the first staking, and the first staked amount may be minimal. Therefore, users can use this possibility to their advantage.
Remediation
The discussion point has been acknowledged by the Multisig Labs team. Their official reply is reproduced below:
We acknowledge that this attack is possible and is a side effect of the nature of our rewards protocol and the short duration of validating on Avalanche. There is some cost and difficulty to exploiting this. It depends on one getting a large amount of GGP before a rewards cycle. If GGP is only available on one AMM, this would greatly move the price with no CEX to arbitrage against. The end result would most likely not be profitable to the attacker if their intention was to dump.
4.4 Checks-effects-interactions pattern
We recommend following the checks-effects-interactions pattern during the claimAndRestake
function in the ClaimNodeOp
contract by moving the staking.decreaseGGPRewards(msg.sender, ggpRewards);
line above the external calls.
Remediation
The issue has been fixed by Multisig Labs in commit 750812.
4.5 Missing status update.
In MinipoolManager
the _cancelMinipoolAndReturnFunds
function should reset the rewardsStartTime
if the .minipoolCount
value for staker is zero.
Remediation
The discussion point has been acknowledged by the Multisig Labs team. Their official reply is reproduced below:
We don’t think that resetting
rewardsStartTime
is the fix because of the scenario below.
Day 1: NodeOp1 creates minipool 1, and it gets launched. Reward startTime set to Day 1.
Day 14: Minipool 1 ends. mpCount = 0. But rewards is still Day 1 so we can get paid on day 28.
Day 15: NodeOp1 creates minipool 2, mpCount = 1
Day 15: NodeOp1 cancels it before launch. mpCount = 0. We can’t reset rewards time because we need to get paid on Day 28. We DO reset the
AVAXAssignedHighWatermark
, so the AVAX used for this cancelled minipool doesn’t count. Instead we remediated by splitting upavaxAssignedHighWater
andavaxAssigned
in this PR. Now the AVAX value used for rewards (avaxAssignedHighWater
), will only be increased when the node is started inrecordStakingStart
.
The issue has been remediated by Multisig Labs in PR 181.
4.6 Unused variables
In Storage
, the intStorage
and bytesStorage
mappings and related functions are not used and can be deleted.
Remediation
The issue has ben acknowledged by the Multisig Labs and they plan to use them in the future.
4.7 Contract upgrades
We recommend paying additional attention when upgrading the contracts. Should the same Storage
be used, the contract itself might not be re-initializable
since its storage would already be used by the previously initialized contract. For example, this could happen in the RewardsPool
contract.
Notice that the RewardPool.initialized
will always be true after the first contract has been initialized.
Remediation
The issue has ben acknowledged by the Multisig Labs. Their official reply is reproduced below:
This is by-design. This specific contract was built to ensure even if upgraded that the
InflationIntervalStartTime
andRewardsCycleStartTime
values would not be overwritten.
4.8 IWithdrawer inheritance
In the withdrawAVAX
function from Vault, it is assumed that msg.sender has inherited the IWithdrawer
interface. We consider that there could be a check for this during the registration process, since in Vault
, for example, withdrawAVAX
cannot be used (it will revert) unless msg.sender
has the IWithdrawer
interface implemented beforehand.
Remediation
The discussion point has been acknowledged by the Multisig Labs team. Their official reply is reproduced below:
We added methods to register, unregister and upgrade contracts to the Protocol Dao. We’ll add a check to our deploy scripts to handle verifying that we inherit from IWithdrawer.
4.9 Protocol DAO setters range
In protocol DAO, setters that deal with rates should range from 0.0 - 1.0 ether. This is not directly enforced as of now. The same could be done for the rest of the setter functions in the contract.
Remediation
The issue has been fixed by Multisig Labs in commit f49931.
4.10 Leftover tokens in RewardsPool
.
RewardsPool
.In the startRewardsCycle
, the allotment each party is supposed to receive is calculated; however, due to the nature of the arithmetics, some tokens might be left out due to rounding errors.
Remediation
The issue has ben acknowledged by the Multisig Labs and they have determined that the amounts would not be significant.
5 Threat Model
The purpose of this section is to provide a full threat model description for each function.
As time permitted, we analyzed each function in the smart contracts and created a written threat model for some critical functions. A threat model documents a given function’s externally controllable inputs and how an attacker could leverage each input to cause harm.
5.1 File: TokenggAVAX
TokenggAVAX
Function: initialize()
initialize()
Intended behavior:
Should initialize all state variables and function calls required for the contract to function.
Branches and code coverage:
Intended branches:
Should be callable by anyone?
Should be called after every upgrade.
Negative behavior:
Shouldn’t allow 2 x calling this.
Preconditions:
Assumes it’s not callable by anyone, or that there’s no way someone can frontrun this transaction
Assumes that the
Storage
is adequately configured (should be fine, sinceguardian
role is assigned in the constructor, for themsg.sender
)
Inputs:
asset
:Control : full control
Checks : no checks
Impact : used as underlying asset for the vault
storageAddress
:Control : full control
Checks : no checks
Impact : used as upgradeable storage contract.
Function: receive()
receive()
Intended behavior:
This function is used for receiving native tokens. It can be called only by the asset
address.
Branches and code coverage:
Intended branches:
Allow
asset
contract to send native tokens to contract.
Negative behavior:
It cannot be called from any other address.
Preconditions:
the
asset
should be set afterinitialize
call
Inputs:
msg.value
:Control : controllable
Authorization : no
Impact : -
msg.sender
:Control : controllable
Authorization :
assert(msg.sender == address(asset));
Impact :
only accept AVAX via fallback from the WAVAX contract
. Otherwise, the balance information may be out of sync.
External call analysis
There are no external calls here.
Function: syncRewards()
syncRewards()
Intended behavior:
Should “distribute rewards” to TokenggAVAX holders. Anyone may call this.
lastSync
- time of last successful call to this function
rewardsCycleEnd
- the time when the total reward will be available;
totalReleasedAssets
- the full amount of available tokens for withdrawal + the last reward value from the previous cycle. If the reward was not withdrawn immediately after the end of the cycle when the function syncRewards
is called for the next cycle, lastRewardsAmt
value will be added to the value totalReleasedAssets
, and this reward still will be available for withdrawal.
Branches and code coverage:
Intended branches:
rewardsCycleEnd
= deadline for nextrewardsCycle
lastSync
= current timestamplastRewardsAmt\_
= to the amount that rewards will deplete from.totalReleasedAssets
is calculated correctly for the next cycle - not sure that it is calculated correctly because it happens differently duringinitialize
calllastRewardsAmt
is calculated for the next cycle if the new reward was deposited.if rewards didn’t deposit, the
lastRewardsAmt
will equal 0 for the next cyclelastRewardsAmt
is calculated correctly and equals 0 for the first cycleif nothing changed since the past cycle
lastRewardsAmt
is calculated correctly and equals 0 andtotalReleasedAssets
was increased by the previouslastRewardsAmt
current
block.timestamp
should be less thanrewardsCycleEnd
Negative behavior:
It basically shouldn’t update unless stuff unless it’s really time to update stuff (see below)
Shouldn’t allow calling unless the
rewardsCycle
has passed theblock.timestamp
.
Preconditions:
Assumes that the state variables (
lastRewardsAmt
,lastSync
,rewardsCycleEnd
andtotalReleasedAssets
are properly updated)Can be called by anyone.
Inputs:
Function call analysis
asset.balanceOf(address(this))
What is controllable? The amount of returned value
If return value controllable, how is it used and how can it go wrong? It can grow if the asset is artificially pumped in the contract;
What happens if it reverts, reenters, or does other unusual control flow? Doesn’t revert.
Function: totalAssets()
totalAssets()
Intended behavior:
This function returns the total amount of underlying assets held by the vault.
Branches and code coverage:
Intended branches:
After the current cycle ends and the new one starts, the
totalAssets
amount will contain the pastlastRewardsAmt
value.totalAssets
is calculated correctly if the current cycle is going.If the current cycle ends and the new one doesn’t start, the
totalAssets
should be equaltotalReleasedAssets\_
+lastRewardsAmt
Negative behavior:
There’s multiple types of uints there, should ensure that there’s no way that any of them can overflow and block the functionality of the contract.
must not revert (as per eip4626)
Preconditions:
Assumes
lastSync
is different than 0 (default value, which is never initialized)? this is missingassumes that
block.timestamp
is safecasted? just as in syncRewards (currently missing)
Inputs:
There aren’t input values here.
Function call analysis
There aren’t function calls here.
Function: depositFromStaking()
depositFromStaking()
Intended behavior:
Should allow converting
native
AVAX tokens towAVAX
(just likewETH
)Allows to
MinipoolManager
contract return with drawn funds and deposit reward.It is assumed that, at first will be called
MinipoolManager.sol:createMinipool
function, which calldepositAVAX
and after that caller will be able to callwithdrawForStaking
for previously deposited value overMinipoolManager.sol:claimAndInitiateStaking
and only after thatdepositFromStaking
can be called overrecordStakingEnd
orrecordStakingError
functions fromMinipoolManager.sol
Branches and code coverage:
Intended branches:
the
asset
balance of the current contract will increase by themsg.value
after the callstakingTotalAssets
will decrease by thebaseAmt
value after the callbaseAmt
+rewardAmt
should be equalmsg.value
Negative behavior:
Shouldn’t be callable by anyone (there’s a check put in place, such that only
onlySpecificRegisteredContract
can call the function.if
stakingTotalAssets
is less thanbaseAmt
transaction will be rejected
Preconditions:
stakingTotalAssets
should contain a value more or equal tobaseAmt
. It means that this value should have been withdrawn overwithdrawForStaking
functionmsg.sender
should be approved for a call
Inputs:
msg.value
:Control : controlled, but actually,it is the value from
getUint(keccak256(abi.encodePacked(“minipool.item”, minipoolIndex, “.avaxLiquidStakerAmt”)));
Checks : should be equal
baseAmt
+rewardAmt
Impact : -
msg.sender
:Control : only approved MinipoolManager contract
Checks :
onlySpecificRegisteredContract(“MinipoolManager”, msg.sender)
Impact : function should be called only from trusted
MinipoolManager
contract
uint256
rewardAmt
:Control : partly controlled
Checks :
msg.value == baseAmt + rewardAmt
Impact : -
uint256
baseAmt
:Control : partly controlled
Checks :
msg.value == baseAmt + rewardAmt
Impact : -
Function call analysis
IWAVAX(address(asset)).deposit{value: totalAmt}();
What is controllable? the
totalAmt
is basicallymsg.value
If return value controllable, how is it used and how can it go wrong? na
What happens if it reverts, reenters, or does other unusual control flow? na
Function: withdrawForStaking()
withdrawForStaking()
Intended behavior:
Should perform the
withdrawalfromwAVAX
, for theMinipoolManager
Branches and code coverage:
Intended branches:
wAVAX.balanceOf(address(this)) -= assets
andbalanceOf(msg.sender) += as sets
Negative behavior:
Shouldn’t allow unlimited amount to be withdrawn
Shouldn’t be callable when it’s
paused
(has thewhenNotPaused
) modifierif
assets
more than theamountAvailableForStaking
transaction will be rejectedif
asset.balanceOf(address(this))
is less thanassets
transaction will be rejectedif
msg.sender
is not approved transaction will be rejected
Preconditions:
Assumes that there has been some
depositFromStaking
beforehand.Assumes that the same
MinipoolManager
deposited the amount. And that there cannot be any issues should one deposit and someone else (with same role) withdraw.
Inputs:
assets
:Control : full control
Checks :
assets > amountAvailableForStaking()
Impact : arbitrary input for the amount of
assets
that are to be withdrawn from thewAVAX
msg.sender
:Control : only approved MinipoolManager contract
Checks :
onlySpecificRegisteredContract(“MinipoolManager”, msg.sender)
Impact : since the caller can withdraw any amount of funds through this function, it is critically important that it is called only by a trusted contract.
Function call analysis
withdrawer.receiveWithdrawalAVAX{value: assets}();
What is controllable? the
assets,withdrawer;
it basically calls thereceiveWithdrawalAVAX
on themsg.sender
!!! Really importantIf return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? reenters: no problems because the contract being called is trusted. reverts: no problems
IWAVAX(address(asset)).withdraw(assets);
What is controllable? the
assets
value; theasset
address is state varIf return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? no problems
Function: depositAVAX
compare with deposit()
from inherited
depositAVAX
compare with deposit()
from inheritedIntended behavior:
Allows any user to deposit
AVAX
in exchange forwAVAX
. It basically doesn’t transfer thewAVAX
back to the user, it keeps it and issues shares to the user.
Branches and code coverage:
Intended branches:
previewDeposit
should issue the amount of shares correctly!!Should transfer the `wAVAX back to the user.
Should exchange user’s supplied
AVAX
intowAVAX
Negative behavior:
Shouldn’t issue more or less shares than intended.
Preconditions:
Assumes users would use this function to deposit, rather than depositing on their own.
Assumes
previewDeposit
calculates the amount of shares correctly.
Inputs:
IWAVAX(address(asset)).deposit()
What is controllable?
assets
- the amount of deposited native tokensIf return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? no problems
afterDeposit()
What is controllable?
assets
- the amount of deposited native tokensIf return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? no problems
\_mint()
What is controllable?
msg.sender
- is minted tokens receiver addressIf return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? no problems
previewDeposit()
&convertToShares()
What is controllable?
assets
- the amount of deposited native tokensIf return value controllable, how is it used and how can it go wrong? if there are any mistakes during shares value calculations, then caller will get more or less shares than expected. If more then caller will be able to drain other users funds, if less then caller will withdraw less native tokens that was deposited.
What happens if it reverts, reenters, or does other unusual control flow? no problems
Function: withdrawAvax()
compare this with withdraw()
from inherited
withdrawAvax()
compare this with withdraw()
from inheritedIntended behavior:
Supposed to
withdrawwAVAX
on behalf of themsg.sender
, and then transfer the nativeAVAX
back to themsg.sender
.
Branches and code coverage:
Intended branches:
the
wavax
balance of the contract should decrease(byassets
)the
avax
balance of user should increase (byassets
)the
shares
of the user should decrease (byshares
)make sure that
preivewWithdraw
calculates the shares properly, in all market conditions
Negative behavior:
shouldn’t allow withdrawing if
_burn
revertedshouldn’t allow burning on behalf of other users
Preconditions:
Assumes there are no rounding errors in
previewWithdraw
or other similar arithmetic issues.Assumes that user has enough
shares
to actually withdraw enoughwAVAX
Inputs:
assets
:Control : full control; the amount of assets that the user
intends
to withdraw.Checks : there is no check here, however, it’s assumed that
previewWithdraw
calculates the amount of shares properly,and then that_burn
fails should themsg.sender
not have enough shares to actually receive the amount of assets.Impact : arbitrary input for the amount of
assets
that are to be withdrawn from thewAVAX
msg.sender
:Control : any caller
Checks : must have the appropriate amount of shares
Impact : the caller will receive the appropriate amount of native tokens
Function call analysis
previewWithdraw(assets)
What is controllable? the assets parameter;
If return value controllable, how is it used and how can it go wrong? return the amount of shares. In case of wrong calculations a caller can burn an excessive number of shares or, conversely, burn too few and receive disproportionately many native tokens.
What happens if it reverts, reenters, or does other unusual control flow? no problems
IWAVAX(address(asset)).withdraw(assets);
What is controllable? the
assets
value; theasset
address is state varIf return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? no problems
Function: redeemAVAX()
compare with redeem()
from inherited
redeemAVAX()
compare with redeem()
from inheritedIntended behavior:
Should redeem the
shares
for underlying nativeavax
. Similar to howwithdraw
works.
Branches and code coverage:
No test coverage
Intended branches:
assets
value is calculated correctlytotalReleasedAssets
is decreased byassets
valuemsg.sender
received theassets
amount of native tokenstoken gg balance of
msg.sender
is decreased byshares
value
Negative behavior:
shouldn’t allow withdrawing if
_burn
revertedshouldn’t allow burning on behalf of other users
revert if
contract.paused
isTrue
Preconditions:
Assumes that user has enough
shares
to burn.
Inputs:
shares
:Control : controlled
Checks : balance of
msg.sender
should be more or equal ofshares
amount
Impact : the number of gg tokens that the user can burn and receive a certain number of native tokens.
`msg.sender``:
Control : any caller
Checks : must have the appropriate amount of shares
Impact : the caller will receive the appropriate amount of native tokens
Function call analysis
previewRedeem(shares)
What is controllable? the shares parameter;
If return value controllable, how is it used and how can it go wrong? return the amount of
assets
. In case of wrong calculations a caller can receive a lot (thereby stealing other users funds) or, conversely, too few native tokens.What happens if it reverts, reenters, or does other unusual control flow? no problems
IWAVAX(address(asset)).withdraw(assets);
What is controllable? -
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? no problems
5.2 File: ClaimNodeOP
ClaimNodeOP
Function: calculateAndDistributeRewards()
calculateAndDistributeRewards()
Intended behavior:
Set the share of rewards that a
staker
is owed. (Fraction of 1 ether)
Branches and code coverage:
Lacks extensive testing.
Intended branches:
Update the
rewardsCycleCount
of staker.Ensure calculations are properly performed.
Increase the
ggpRewards
for thestakerAddr
based on the inputtotalEligibleGGPStaked
.
Negative behavior:
Should fail if
stakerAddr
is not eligible for rewards.
Preconditions:
Assumes
stakerAddr
is a valid one.Assumes that the caller has used the correct
totalEligibleGGPStaked
amount.
Inputs:
msg.sender
:Control : -
Checks :
onlyMultisig
Impact : the access to this function should be restricted because this function allows to assign any part of reward budget to any
stakerAddr
.
stakerAddr
:Control : full control
Checks : no checks at this level; But will revert during the
increaseGGPRewards
function call.Impact : the address of valid staker who can claim the reward.
totalEligibleGGPStaked
:Control : full control
Checks : there aren’t checks
Impact : the total amount of staked funds, from which the percentage of reward to
stakerAddr
will be calculated. So this value allow to control the reward part forstakerAddr
.
Function call analysis
staking.getLastRewardsCycleCompleted(stakerAddr)
What is controllable?
stakerAddr
is controllableIf return value controllable, how is it used and how can it go wrong? if someone will be able to manipulate
lastRewardsCycleCompleted
value, thestakerAddr
will be able to double receive the reward.What happens if it reverts, reenters, or does other unusual control flow? no problems
staking.getEffectiveGGPStaked(stakerAddr);
What is controllable?
stakerAddr
is controllableIf return value controllable, how is it used and how can it go wrong? the amount of staked tokens is used to calculate the percentage of the total staked tokens.
What happens if it reverts, reenters, or does other unusual control flow? no problem
staking.setLastRewardsCycleCompleted(stakerAddr, rewardsPool.getRewardsCycleCount());
What is controllable?
stakerAddr
is controllableIf return value controllable, how is it used and how can it go wrong? there isn’t return value.
What happens if it reverts, reenters, or does other unusual control flow? no problem
staking.resetAVAXAssignedHighWater(stakerAddr);
What is controllable?
stakerAddr
is controllableIf return value controllable, how is it used and how can it go wrong? there isn’t return value.
What happens if it reverts, reenters, or does other unusual control flow? no problem
staking.increaseGGPRewards(stakerAddr, rewardsAmt);
What is controllable?
stakerAddr
is controllableIf return value controllable, how is it used and how can it go wrong? there isn’t return value.
What happens if it reverts, reenters, or does other unusual control flow? no problem
staking.setRewardsStartTime(stakerAddr, 0);
What is controllable?
stakerAddr
is controllableIf return value controllable, how is it used and how can it go wrong? there isn’t return value.
What happens if it reverts, reenters, or does other unusual control flow? no problem
Function: claimAndRestake()
claimAndRestake()
Intended behavior:
Allows
msg.sender
to claim therewards
they were allocated.
Branches and code coverage:
Lacks extensive testing.
Intended branches:
Should decrease rewards balance of
msg.sender
Restake the amount of
ggpRewards - claimAmt
Negative behavior:
Should not allow claiming more than
msg.sender
was owed
Preconditions:
Assumes
msg.sender
has some rewardsAssume that the
vault
holds enough tokens to pay the rewards formsg.sender
.
Inputs:
msg.sender
:Control : -
Checks : if the
ggpRewards
value is zero, will revert.Impact : the address who owns non zero reward value.
claimAmt
:Control : full control
Checks : should not be more that the reward: claimAmt > ggpRewards
Impact : the amount of withdrawn funds, the surplus will be restake.
Function call analysis
vault.withdrawToken(address(this), ggp, restakeAmt)
What is controllable?
restakeAmt
is controllableIf return value controllable, how is it used and how can it go wrong? there is no return value here.
What happens if it reverts, reenters, or does other unusual control flow? will revert if there are not enough tokens.
staking.getGGPRewards(msg.sender)
What is controllable? -
If return value controllable, how is it used and how can it go wrong? return value is used for calculating the amount of rewards that
msg.sender
is owed.What happens if it reverts, reenters, or does other unusual control flow? no problems
5.3 File: ClaimProtocolDAO.sol
ClaimProtocolDAO.sol
Function: spend()
spend()
Intended behavior:
Allows to spend the ProtocolDAO’s GGP rewards
Branches and code coverage:
Intended branches:
The balance of
recipientAddress
is increased byamount;
there is a revert put in place in casetransfer
fails.
Negative behavior:
should be rejected if this contract has not enough ggp tokens in the
vault.tokenBalance
should reject if
msg.sender
isn’t the guardian
Preconditions:
msg.sender
is the guardiantokens should be transferred to
ClaimProtocolDAO
contract over thevault.transferToken
function
Inputs:
amount
:Control : limited control
Checks :
amount==0||amount>vault.balanceOfToken(“ClaimProtocolDAO”, ggpToken)
Impact :
recipientAddress
:Control : controlled
Checks : there aren’t checks here
Impact : since there are no address checks, in case of a mistake, tokens can be transferred to the wrong user.
invoiceID
:Control : controlled
Checks : there aren’t checks here
Impact : no impact
msg.sender
:Control : -
Checks :
onlyGuardian
Impact : it allows caller to withdraw the entire balance of ggpToken of this contract from vault. The access to this function should be restricted.
Function call analysis
vault.withdrawToken()
What is controllable? recipientAddress, amount
If return value controllable, how is it used and how can it go wrong? there is no return value here
What happens if it reverts, reenters, or does other unusual control flow? will revert if msg.sender doesn’t have enough tokens
5.4 File:BaseUpgradeable.sol
The contract is inherited from BaseAbstract.sol
and Initializable.sol
(@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol);
Function: _BaseUpgradeable_init()
_BaseUpgradeable_init()
Allows to initialize the gogoStorage
storage address. The function is internal and can be called only once due to onlyInitializing
modifier.
5.5 File: Base.sol
Base.sol
The contract is inherited from BaseAbstract.sol
; The contract contains only constructor with initialization of gogoStorage
address.
5.6 File: BaseAbstract.sol
BaseAbstract.sol
Function: setters()
setters()
Intended behavior:
Allows you to make changes to the data stored in the shared storage. All function is internal, therefore, they cannot be called directly. But they are called from various functions from inherited contracts.
5.7 File: Storage.sol
Storage.sol
Function: setGuardian()
setGuardian()
Intended behavior: Allow to reassign the guardian address. But to complete this process the new guardian should call confirmGuardian
function.
Branches and code coverage:
Intended branches:
After successful call the
guardian
address didn’t change.
Negative behavior:
Reject if
msg.sender
isn’t the guardian; check put in place.
Preconditions:
msg.sender
is current guardian.
Inputs:
msg.sender:
Control : -
Checks : msg.sender != guardian
Impact : due to the guardian having a lot of control over the protocol, it’s critically important that an untrusted caller doesn’t have access to this function.
Function call analysis
There aren’t external calls here.
Function: confirmGuardian()
confirmGuardian()
Intended behavior: Allow to reassign the guardian address. But to complete this process the new guardian should call confirmGuardian
function.
Branches and code coverage:
Intended branches:
After successful call the
guardian
address is equal tomsg.sender
andnewGuardian
.
Negative behavior:
Reject if
msg.sender
isn’t thenewGuardian
; check put in place.
Preconditions:
The current guardian called the setGuardian
function and msg.sender
became the newGuardian
.
Inputs:
msg.sender:
Control : -
Checks : msg.sender !=newGuardian
Impact : due to the guardian having a lot of control over the protocol, it’s critically important that an untrusted caller doesn’t have access to this function.
Function call analysis
There aren’t external calls here.
Function: setters()
setters()
Intended behavior:
Should be used among more contracts as a shared means of storage
Branches and code coverage:
Intended branches:
Should update the {type} of value located at each particular key.
Negative behavior:
Network registered contracts shouldn’t abuse the
booleanStorage[keccak256(a bi.encodePacked(“contract.exists”, msg.sender))]
modifier. Basically once a contract iswhitelisted
, it can remove/register other contracts asnetwork
registered, or modify any other states altogether.
Preconditions:
Assumes that
msg.sender
handles the states properly, and doesn’t have typos when reading/updating specific states. Basically allf unctions that interact with the getters/ setters/ deleters from other contracts should be extremely well tested.
5.8 File: TokenGGP.sol
TokenGGP.sol
The contract is standard ERC20
from @rari-capital/solmate/src/tokens/ERC20.sol
.
5.9 File: Vault.sol
Vault.sol
Function: depositAVAX()\*\*
\*\*
Intended behavior:
Allows registered contract to deposit avax.
Branches and code coverage:
Intended branches:
avaxBalances
ofmsg.sender
increased bymsg.value
Negative behavior:
if
msg.sender
is notRegisteredNetworkContract
transaction will be revertedif msg.value == 0, will be reverted
Preconditions:
msg.sender
should be registered by the guardian
Inputs:
msg.sender
:Control : -
Checks :
onlyRegisteredNetworkContract
Impact : no impact
msg.value
:Control : limited control
Checks : msg.value == 0
Impact : no impact
Function call analysis
There aren’t external calls here.
Function: withdrawAVAX()
withdrawAVAX()
Intended behavior:
Allows registered contract to withdraw the deposited avax.
Branches and code coverage:
Intended branches:
after the call
avaxBalances[msg.sender]
decreased byamount
Negative behavior:
if
msg.sender
is notRegisteredNetworkContract
transaction will be revertedif
avaxBalances[msg.sender] < amount
, transaction will be reverted
Preconditions:
avaxBalances of msg.sender & amount
msg.sender should be registered contract by guardian
Inputs:
msg.sender
:Control : -
Checks : onlyRegisteredNetworkContract
Impact : should has non zero balance for withdraw
amount
:Control : controlled
Checks : avaxBalances[getContractName(msg.sender)] < amount
Impact : must withdraw only his tokens
Function call analysis
withdrawer.receiveWithdrawalAVAX()
What is controllable? amount - partly controlled, the
avaxBalances[msg.sender] > amount
If return value controllable, how is it used and how can it go wrong? there isn’t a return value here
What happens if it reverts, reenters, or does other unusual control flow? function is nonReentrant and state is updated before the external call.
Function: transferAVAX()
transferAVAX()
Intended behavior:
Allows transferring the balance from one registered contract to another.
Allows a transfer, not from the owner, and there is also no check for an allowance from the owner
Branches and code coverage:
Intended branches:
avaxBalances[toContractName]
is increased amountavaxBalances[fromContractName]
is decreased by amount
Negative behavior:
Should be rejected if
avaxBalances[fromContractName] < amount
Should be rejected if
toContractName
andfromContractName
is not added togogoStorage
Should be rejected if msg.sender is not
fromContractName
Preconditions:
toContractName
andfromContractName
is added togogoStorage
msg.sender
isRegisteredNetworkContract
avaxBalances[fromContractName] & amount
Inputs:
toContractName
:Control : controlled
Checks : contract name should be saved inside gogoStorage
Impact : in the case of an incorrect recipient, funds may be lost.
fromContractName
:Control : controlled
Checks : contract name should be saved inside gogoStorage
Impact : the contract which funds will be transferred, in this case the
msg.sender
has full control
msg.sender
:Control : -
Checks : onlyRegisteredNetworkContract
Impact : -
amount:
Control : controlled
Checks :
avaxBalances[fromContractName] < amount
Impact : -
Function call analysis
There aren’t external calls here.
Function: depositToken()
depositToken()
Intended behavior:
Allows registered contract to deposit any tokens
Branches and code coverage:
Intended branches:
tokenBalances
of networkContractName & contractKey is increased byamount
Negative behavior:
Should reject if
msg.sender
is notguardianOrRegisteredContracts
Preconditions:
msg.sender has enough tokens
msg.sender is guardianOrRegisteredContracts
Inputs:
amount
:Control : limited control
Checks : amount == 0
Impact : no problems
tokenContract
:Control : full control
Checks : there isn’t checks here
Impact : address of external contract to be called
networkContractName
:Control : limited control
Checks : contract name should be saved inside gogoStorage
Impact : the recipient of tokens, in the case of an incorrect recipient, funds may be lost.
Function call analysis
tokenContract.safeTransferFrom()
What is controllable? amount
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? will revert if
msg.sender
doesn’t have enough tokens
Function: withdrawToken()
withdrawToken()
Intended behavior:
Allow
registered msg.sender
to withdraw ERC20 tokens.
Branches and code coverage:
Intended branches:
Check
withdrawalAddress
?Decrease
tokenBalance[paid(caller, token)]
Validate the
tokenContract
, such that no arbitrary tokens can be used.
Negative behavior:
Shouldn’t allow withdrawing more than
msg.sender
owns.
Preconditions:
Assumes
msg.sender
is registered;Assumes that the
tokenAddress
is legit and not some malicious token
Inputs:
withdrawalAddress
:Control : full control
Checks : no checks
Impact : in the case of an incorrect recipient, funds may be lost.
tokenAddress
:Control : full control
Checks : no checks
Impact : should allow to pass only trusted contracts.
amount
:Control : limited control
Checks : check that it’s != 0 and that user has more balance than it.
Impact : shouldn’t allow to pass more tokens amount than caller owns.
Function call analysis
tokenContract.safeTransfer(withdrawalAddress, amount)
What is controllable?
withdrawalAddress
,amount
If return value controllable, how is it used and how can it go wrong? no checks on
withdrawalAddress
.What happens if it reverts, reenters, or does other unusual control flow? will revert if
msg.sender
doesn’t have enough tokens
Function: transferToken()
transferToken()
Intended behavior:
Transfer token from one contract(msg.sender) to another
Branches and code coverage:
Intended branches:
Validate the
tokenContract
, such that no arbitrary tokens can be used.Assure both contracts are registered.
Compared to the
transferAVAX
, this function does not allow the transfer from arbitrary tokens, and only frommsg.sender
Increase
tokenBalances[to]
AND decreasetokenBalances[from]
.
Negative behavior:
Revert if
msg.sender
is not a registered contract.Revert ifmsg.senderdoesn’t have enough tokens amount.
Preconditions:
Assumes both contracts have been registered beforehand.
Inputs:
networkContractName
:Control : full
Checks : check that it’s registered
Impact : in the case of an incorrect recipient, funds may be lost.
tokenAddress
:Control : full control
Checks : No checks! Any token
Impact : should allow to pass only trusted contract address.
Function call analysis
There aren’t external calls here.
5.10 File: MinipoolManager
MinipoolManager
Function: createMinipool()
createMinipool()
Intended behavior:
Create a Minipool. Accepts avax native deposit (which have to be staked in) and it’s open to public.
Allows to any caller to recreate a minipool is current state is finished or canceled.
Branches and code coverage:
Intended branches:
Ensure that the
msg.sender
is a registered staker (required checks are added in each underlying function)Should ensure that the
avaxAssignmentRequest
can be fulfilled (or that it is atleast achievable)User’s
avax
balance should deplete, and the contract’s balance should increase.After the call, the current state of the minipool is
Prelaunch
native token balance of
assets
should increase bymsg.value
assets
balance ofvault
contract should increase bymsg.value
if the pool for
nodeID
exists and the current state is Finished or Canceled minipool data should be resetcreate a new poll if the pool for
nodeID
did not exist beforeStaking.sol:getRewardsStartTime(msg.sender)
should be equalblock.timestamp
ifRewardsStartTime
was zero before the callStaking.sol:getMinipoolCount(msg.sender)
should increase by 1Staking.sol:getAVAXAssigned(msg.sender)
should increase byavaxAssignmentRequest
Staking.sol:getAVAXStake(msg.sender)
should increase bymsg.value
Negative behavior:
Shouldn’t work when the contract is paused?/
Should assure that the
nodeId
hasn’t registered beforehand and is unique basically, so no overwrites can be made.should revert if minipool for
nodeID
already exists and the currentStatus is not Finished or currentStatus is not Canceledshould revert if
msg.sender
invalid staker
Preconditions:
Assumes that the supplied
msg.value
surpasses the minimum staking amount.Assumes that the
multisig
that is to be assigned is ≠ 0.Assumes that should the
miniPool
exist, it can only be overwritten if the node is either finished or cancelled.In the case that an already existing
miniPoolId
exists, it assumes that ALL PRIOR STATES HAVE BEEN RESET (FROM ALL CONTRACTS THAT WOULD HAVE INTERACTED WITH THIS ONE IN THE FIRST)msg.sender
should be registered stakermsg.sender should stake ggp over Staking.stakeGGP() function
Inputs:
msg.sender
:Control : controlled
Checks :
staking.increaseAVAXStake()
,requireValidStaker()
checksmsg.sender
address (should stake ggp over stakeGGP() function)Impact : N/A
msg.value
:Control : N/A
Checks :
msg.value
should be equalavaxAssignmentRequest
Impact : N/A
nodeId
:Control : full control
Checks : there are some checks on whether the
nodeID
has been registered before; need to look into thisImpact : could potentially be overwritten.
duration
:Control : full control
Checks : There are no checks on the duration amount
Impact : N/A
delegationfee
:Control : full
Checks : No checks
Impact : N/A
avaxAssignmentRequest
:Control : full control; needs to match
msg.value
since it’s the amount of requestedAVAX
TO BE MATCHED IN THE POOL.Checks : there are checks on whether it matches
msg.value
there are also some checks on whether it matches the
dao
details; assure that the data returned from there is not 0?Impact : N/A
Function call analysis
!!! Important functions (withdraw/ deposit/ etc) shouldn’t work when the contract is paused.
vault.depositAVAX()
What is controllable? msg.value
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? no problems
getCollateralizationRatio()
What is controllable? msg.sender
If return value controllable, how is it used and how can it go wrong? The returns collateralization ratio also depends on how much msg.sender de- posited ggp
What happens if it reverts, reenters, or does other unusual control flow? no problems
increaseMinipoolCount()
What is controllable? msg.sender (had to deposit ggp before)
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? no problems
increaseAVAXAssigned()
What is controllable? msg.sender (had to deposit ggp before), avaxAssignmentRequest
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? no problems
increaseAVAXStake()
What is controllable? msg.sender (had to deposit ggp before), msg.value
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? no problems
Function: cancelMinipool()
cancelMinipool()
Intended behavior:
Allows owner to cancel existing minipool and get back the deposited funds.
Branches and code coverage:
Intended branches:
Should update all details related to the specific
nodeId
. In such a way that one can then be reused eventually (create with samenodeId
)Refund all invested funds to the
owner
(deployer)Make sure that the minipool is prelaunch (NOT CHECKED); it’s assured though in
requireValidStateTransition
basically, since it checks the current status against the wanted status update.Staking.sol:getAVAXAssigned(msg.sender)
should decrease byavaxLiquidStakerAmt
Staking.sol:getAVAXStake(msg.sender)
should decrease byavaxNodeOpAmt
Staking.sol:getMinipoolCount(msg.sender)
should decrease by 1the native tokens balance of the caller should increase by the amount of funds previously deposited.
After the call, the current state of the minipool is
Canceled
Negative behavior:
Shouldn’t leave previously set fields to their value (eg. the
avaxLiquidStakerAmt
)Shouldn’t allow unauthorized access(
msg.sender
has to be theowner
)should revert if the current state of mini pool isn’t
Prelaunch
should revert if called non-owner of minipool
should revert if minipool for nodeID doesn’t exist
Preconditions:
the minipool should be created over the
createMinipool
functionthe current state of the minipool should be
Prelaunch
Assumes that the
nodeId
has been created beforehand and that it’s in theprelaunch
stageAssumes that the
owner
of thenodeID
calls it
Inputs:
nodeId
:Control : full control
Checks : there’s a check on whether the minipool is valid.
Impact : Id of minipool which will be canceled and funds will returned to owner.
msg.sender
:Control : onlyOwner of minipool can call
Checks :
onlyOwner(index);
Impact : only the owner should be able to call this function. otherwise, users will maliciously close other people’s pools to get more rewards.
Function call analysis
\_cancelMinipoolAndReturnFunds()
What is controllable? the
nodeID
is controllable.If return value controllable, how is it used and how can it go wrong? there isn’t return value here.
What happens if it reverts, reenters, or does other unusual control flow? can be reverted if there aren’t enough native tokens for withdraw.
owner.safeTransferETH()
What is controllable? nothing controllable
If return value controllable, how is it used and how can it go wrong? there isn’t return value here.
What happens if it reverts, reenters, or does other unusual control flow? no problems
vault.withdrawAVAX()
What is controllable? nothing controllable
If return value controllable, how is it used and how can it go wrong? there isn’t a return value here
What happens if it reverts, reenters, or does other unusual control flow? will revert if contract has not enough shares
Function: _cancelMinipoolAndReturnFunds()
_cancelMinipoolAndReturnFunds()
Intended behavior:
Internal function.
Main logic of cancelling a minipool and returning the funds that were initially attributed to it.
Branches and code coverage:
Intended branches:
Ensure that all states are reset after a Minipool has been cancelled and that
owner
no longer has access to it.Ensure that current state allows
cancellation
.Ensure that
avaxNodeOpAmt
is decreased.Ensure that
avaxLiquidStakerAmt
is decreased
Negative behavior:
Shouldn’t allow
cancellation
if the current state !=prelaunch
Shouldn’t allow cancellation on behalf of
msg.sender != owner
Preconditions:
Assumes that the function has been called from a privileged one (i.e one that has a check that
msg.sender == owner of market
)
Inputs:
nodeID
:Control : full control
Checks : no checks at this level
Impact : nothing is done on the `nodeId at this level, so not that important
index
:Control : full control (it’s generated in previous function)
Checks : no checks
Impact : important, as it allows altering states of the
minipool
Function call analysis
decreaseAVAXStake()
What is controllable? the
owner
(who’s supposed to be the caller of the function)
it basically decreases the
avaxNodeOpAmt
value which is originallyincreased
in the pool creation! The detail here is that it uses.avaxNodeOpAmount
to store theamount
, while it decreases theavaxNodeOpAmt
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow?
if it reverts it could affect cancelling the pool. (that’s why it’s better to only use one type of amount ^ )
decreaseAVAXAssigned()
What is controllable? nothing, the values are taken from the storage.
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? if current
avaxAssigned
is not enough function will be reverted
resetAVAXAssignedHighWater()
What is controllable? nothing, the value is taken from the storage.
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? allows to set the
avaxAssignedHighWater
to the previous value, so that the current value is not used when calculating the reward.
decreaseMinipoolCount()
What is controllable? nothing, the value is taken from the storage.
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? reduces the number of pools, if it is reset to zero, this staker will not be taken into account when calculating the reward.
Function: withdrawMinipoolFunds()
withdrawMinipoolFunds()
Intended behavior:
Node operator can claim all
avax
they are due.(staked + rewards if any)
Branches and code coverage:
LIMITED TESTING
Intended branches:
Should decrease
msg.sender
stake in theminipool
byavaxNodeOpAmt
the native tokens balance of minipool owner should increase by
totalAvaxAmt
value (deposited amount + reward)
Negative behavior:
Shouldn’t be callable by any
msg.sender
or on anynodeId
should revert if the owner calls it a second time after the successful first execution
should revert if the current state of minipool isn’t
Withdrawable
orError
should revert if called non-owner of minipool
should revert if minipool for nodeID doesn’t exist
Preconditions:
The minipool should be created over the
createMinipool
function.Assumes that the state can transition tofinished, and that the current state of the minipool should be
Withdrawable
(afterrecordStakingEnd
call) orError
.
Inputs:
msg.sender
:Control : -
Checks : there is a check that msg.sender is owner of minipool
Impact : allows to owner of minipool withdraw funds when staking finished
nodeID
:Control : controlled
Checks : there are a check of the status of the minipool and a check of the owner
Impact : allows to return the funds to the owner of minipool if staking was finished
Function call analysis
owner.safeTransferETH()
What is controllable? nothing controllable
If return value controllable, how is it used and how can it go wrong? there isn’t return value here.
What happens if it reverts, reenters, or does other unusual control flow? no problems
vault.withdrawAVAX()
What is controllable? nothing controllable
If return value controllable, how is it used and how can it go wrong? there isn’t a return value here
What happens if it reverts, reenters, or does other unusual control flow? will revert if contract has not enough shares
Function: claimAndInitiateStaking()
claimAndInitiateStaking()
Intended behavior:
Remove the
minipool's
avax from the protocol and stake it on avalanche, register node as validator.
Branches and code coverage:
Intended branches:
Ensure only
multisig rialto
can call this.Should ensure the status of the
minipool
is such that it can be launchedShould decrease the
avax
associated to the pool (something with.avaxLiquidStakerAmt
)
Negative behavior:
transaction should be rejected if current status != Prelaunch
transaction should be rejected if msg.sender isn’t approved address
Preconditions:
Assumes that contract has enough
wavax
staked that can be withdrawable.
Inputs:
msg.sender:
Control : -
Checks :
onlyValidMultisig(nodeID)
: msg.sender == assignedMultisig`Impact : only valid multisig can call this function, because the all deposit funds will be transferred to caller.
nodeID
:Control : full control
Checks :
requireValidMinipool(nodeID)
Impact : no impact
Function call analysis
msg.sender.safeTransferETH()
What is controllable? msg.sender is controlled
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? will revert in case of error
vault.withdrawAVAX()
What is controllable? nothing is controlled
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? allows to withdraw
avaxNodeOpAmt
from vault and transfer this funds to caller
ggAVAX.withdrawForStaking()
What is controllable? nothing is controlled
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? allows to withdraw
avaxLiquidStakerAmt
from vault and transfer this funds to caller
Function: recordStakingStart()
recordStakingStart()
Intended behavior:
Rialto calls after
claimAndInitiateStaking
succeeded.
Branches and code coverage:
Intended branches:
Changes the
start
time. Make sure it’s not in past or future?Should transition a
nodeID
into “staking” period.
Negative behavior:
Anyone other than
rialto
shouldn’t be able to call this.transaction should be rejected if current status != Launched
Preconditions:
Has to assure that enough values are in the
minipool
Inputs:
startTime
:Control : controllable
Checks : there isn’t check
Impact : if the value is far in the future it will be impossible to complete the stacking successfully only with error state
txID
:Control : controllable
Checks : there isn’t check
Impact : n/a
nodeID
:Control : partly controllable
Checks :
requireValidMinipool(nodeID)
Impact : n/a
msg.sender
:Control : -
Checks :
onlyValidMultisig(nodeID) : msg.sender == assignedMultisig
Impact : if a malicious user is able to call the function, he will be able to set
startTime
value, at which it will be impossible to successfully complete the stacking with only an error state
Function call analysis
There aren’t external function calls here.
Function: recordStakingEnd()
recordStakingEnd()
Intended behavior:
Finish the
validation
period of thestaking
for thnodeid
.
Branches and code coverage:
Intended branches:
Should update all states accordingly after the transfers occur.
End time should be in the future (
starttime
and not in past compared toblock.timestamp
?)Should only be callable when the
endtime
is reached.
Negative behavior:
Shouldn’t be callable twice or in any other circumstance other than the transition to
withdrawable
transaction should be rejected if msg.value is not enough
transaction should be rejected if msg.sender isn’t approved address
transaction should be rejected if current status != Staking
Preconditions:
the current state of the minipool should be
Staking
.
Inputs:
msg.value
:Control : -
Checks :
msg.value
should be equaltotalAvaxAmt + avaxTotalRewardAmt
Impact :
avaxTotalRewardAmt
:Control : full control
Checks :
msg.value
should beequaltotalAvaxAmt + avaxTotalRewardAmt
Impact : the value completely controls how much reward the owner of the pool will receive.
endTime
:Control : controllable
Checks : should be more than the startTime and more than current time
Impact : no impact
nodeID
:Control : partly controllable
Checks :
requireValidMinipool(nodeID)
Impact : no impact
msg.sender
:Control : -
Checks :
onlyValidMultisig(nodeID) : msg.sender == assignedMultisig
Impact : only valid multisig can control when staking will be finished
Function call analysis
slash()
What is controllable? minipoolIndex
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? can be reverted if
ggAVAX.depositFromStaking
What is controllable?
avaxLiquidStakerRewardAmt
- partly controlledIf return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? revert if
stakingTotalAssets
value is less thanavaxLiquidStakerAmt
vault.depositAVAX()
What is controllable?
avaxNodeOpRewardAmt
- partly controlledIf return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? revert if
previewDeposit
returns 0.
Function: recordStakingError()
recordStakingError()
Intended behavior:
A staking error occurred while registering the node as a validator.
Can be called after claimAndInitiateStaking
or recordStakingStart
Branches and code coverage:
Intended branches:
After the call the new status is
Error
Negative behavior:
transaction should be rejected if current status !=
Staking
orLaunched
Preconditions:
current status should be
Launched
orStaking
Inputs:
msg.value
:Control : -
Checks :
msg.value
should be equalavaxNodeOpAmt + avaxLiquidStakerAmt
- the withdrawn fundsImpact : amount of returned to staker funds. must not be less than the funds taken.
errorCode
:Control : controlled
Checks : there isn’t check here
Impact : no problems
nodeID
:Control : controlled
Checks : check that minipool exists
Impact : the ID of the minipool that will be completed with an error without issuing a reward.
Function call analysis
ggAVAX.depositFromStaking()
What is controllable? nothing is controlled
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? will revert if
stakingTotalAssets
is less thanavaxLiquidStakerAmt
vault.depositAVAX()
What is controllable?
avaxNodeOpRewardAmt
- partly controlledIf return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? revert if
previewDeposit
returns 0.
5.11 File: MultisigManager
MultisigManager
Function: registerMultisig()
registerMultisig()
Intended behavior:
Register a multisig. Defaults to disabled when first registered. The index where the
multisig
is to be added should be the previously increasedmultisig.count
Branches and code coverage:
Intended branches:
“There will never be more than 10 total multisigs” There should be a check that 10 total multisigs can be registered (index != 9) and no more
Should register the
addr
as a new multisig, only if it doesn’t exist already.
Negative behavior:
Shouldn’t allow anyone else other than the guardian to call it
Shouldn’t overwrite already existing multisig
Shouldn’t also enable the multisig
Preconditions:
Assumes
getIndexOf
calculates theindex
properly and that two addresses cannot point to sameindex
.Assumes there’s a way to deregister a
Multisig
? Currently, there’s none; there’s only a way to disable them.
Inputs:
Function call analysis
Function: enableMultisig()
enableMultisig()
Intended behavior:
Should enable a registered multisig.
Branches and code coverage:
Intended branches:
The “enabled” of the
index
should be set totrue
.
Negative behavior:
Shouldn’t update the index of another multisig.
Shouldn’t be callable by anyone.
Shouldn’t enable a multisig that doesn’t exist.
Preconditions:
Assumes that the
multisig
has been created beforehand.
Inputs:
Function call analysis
Function: disableMultisig()
disableMultisig()
Intended behavior:
Should disable a registered multisig.
Branches and code coverage:
Intended branches:
The “enabled” of the
index
should be set tofalse
.
Negative behavior:
Shouldn’t be callable by any
msg.sender
Shouldn’t update an non-existing
index
Preconditions:
Assumes that it can be called under any circumstances. What if it’s called during a transaction where it needs to approve it?
Inputs:
Function call analysis
5.12 File: Ocyticus
Ocyticus
Function: addDefender()
, removeDefender()
addDefender()
, removeDefender()
Intended behavior:
Allow guardian to
add
orremove
defenders.
Branches and code coverage:
Lacks testing
Intended branches:
Should update the
defenders
states properly.
Negative behavior:
Should only be callable by
guardian
; covered byonlyGuardian
modifier.
Preconditions:
Assumes they are called by external accounts.
Inputs:
n/a
Function call analysis
Function: pauseEverything()
pauseEverything()
Intended behavior:
Allows the
defender
topause
every contract that can be paused.
Branches and code coverage:
Intended branches:
Pause
TokenGGAVAX
Pause
MinipoolManager
Pause
Staking
(MISSING!) - added as remediation
Negative behavior:
Preconditions:
Assumes that the contracts can be
paused
.Assumes that when paused, no important functions from these contracts can be called! Double check this
Inputs:
n/a
Function call analysis
n/a
Function: resumeEverything()
resumeEverything()
Intended behavior:
Branches and code coverage:
Intended branches:
Unpause
TokenGGAVAX
Unpause
MinipoolManager
Unpause
Staking
- added as remediation
Negative behavior:
Preconditions:
Assumes that some other function will reenable all
multisigs
? That’s not covered in this contract
Inputs:
n/a
Function call analysis
n/a
5.13 File: Oracle
Oracle
Function: setGGPPriceInAVAX()
, getGGPPriceInAVAXFromOneInch
, getGGPPriceInAVAX
setGGPPriceInAVAX()
, getGGPPriceInAVAXFromOneInch
, getGGPPriceInAVAX
Intended behavior:
Interface for off-chain aggregated data, used for pricing the tokens and calculating amounts. The
getGGPPriceInAVAXFromOneInch
should never be used on chain.
Branches and code coverage:
Lacks testing.
Intended branches:
The functions/contracts that make use of the
GetGGPPriceInAvax
SHOULD have some slippage check in regards to the timestamp when the price has been updated: eg. If the price update happened more than 5 blocks away, revert the transaction.
Negative behavior:
Shouldn’t be callable by anyone. Only Multisig modifier put in place.
Preconditions:
getGGPPriceInAVAXFromOneInch
should only be called off-chain; it’s not reliable enough to be called on chain directly.Assumes the Multisig update the
getGGPPRiceInAvax
quite often and that they are trustworthy.
Inputs:
There aren’t input values here.
Function: setOneInch()
setOneInch()
Intended behavior:
Allows to guardian to set the address of the One Inch price aggregator contract
Branches and code coverage:
Intended branches:
after the call
Oracle.OneInch
is updated to new address
Negative behavior:
Revert if caller is not Guardian.
Preconditions:
msg.sender
is Guardian
Inputs:
addr
:Control : controlled
Checks : There isn’t check here.
Impact : The contract address which will be called inside view
getGGPPriceInAVAXFromOneInch
function
Function call analysis
There aren’t external calls here.
Function: setGGPPriceInAVAX()
setGGPPriceInAVAX()
Intended behavior:
The function is used by the Multisig to update the on-chain prices, with presumably the data retrieved off-chain from
OneInch
.
Branches and code coverage:
Intended branches:
Should update the
GGPTimestamp
Should update the
GGPPriceInAvax
Negative behavior:
Revert if caller is not Multisig
Preconditions:
msg.sender
is Multisig
Inputs:
price
:Control : controlled
Checks : price != 0
Impact : the price value is used during
calculateGGPSlashAmt
call
timestamp
:Control : controlled
Checks :
timestamp
should be >=lastTimestamp
ortimestamp
should be <=block.timestamp
Impact : n/a
Function call analysis
There aren’t external calls here.
5.14 File: ProtocolDAO
ProtocolDAO
Function: initialize()
initialize()
Intended behavior:
Initialize the contract
TotalGGPCirculatingSupply = 18.000.000
buttotalTokenGGPsupply = 22.500.000
Branches and code coverage:
Not tested in the case of a re-deployment (or upgrade, as discussed with the team).
Intended branches:
All set parameters should have a getter.
Negative behavior:
Setters that deal with rates should range from
0.0 - 1.0 ether
. This is not directly enforced; The same should be done for the rest of thesetter
functions from the contract. This was mitigated.
Preconditions:
Assumes that it can only be called once, and that is through the
onlyGuardian
Assumes it will be called BEFORE any other functions that would use the initialized variables will be called. Maybe assure in important functions that
getBool(keccak256(“ProtocolDAO.initialized”))
is TRUE
Inputs:
Function call analysis
5.15 File: RewardsPool
RewardsPool
Function: initialize()
initialize()
Intended behavior:
Re-initialize all
RewardsPool
variables for a newRewardsPool
; This is upgradeable
Branches and code coverage:
Not tested in the case of a re-deployment (or upgrade, as discussed with the team).
Intended branches:
Should set the
RewardsPool
variables to their initial values.
Negative behavior:
Preconditions:
Assumes it’s the first the this type of contract has been deployed.
Inputs:
There aren’t input values here.
Function call analysis
There aren’t external calls here.
Function: inflate()
inflate()
Intended behavior:
Called to release more
GGP
from the total supply.says “mint” new tokens, but all of them are already minted.
Branches and code coverage:
Intended branches:
Should update the rewardsCycle total amount.
Should update the
inflationIntervalElapsedSeconds
Should increase circulating supply of tokens.
Preconditions:
Assumes it won’t be called that often
Inputs:
There aren’t input values here.
Function call analysis
dao.setTotalGGPCirculatingSupply(newTotalSupply)
What is controllable? -
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? no problems
Function: startRewardsCycle()
startRewardsCycle()
Intended behavior:
Runs aggprewards cycle if possible.
Branches and code coverage:
More extensive testing required.
Intended branches:
if
dao allotment
exists != transferdaoAllotment
to DAO != its balance should increaseif
nop allotment
exists != transfernopAllotment
to NOP != its balance should increaseif
multisig allotment
exists != transfermultisigAllotment
to MULTISIG != its balance should increaseMake sure allotments add up to 100% (the percentages)
Negative behavior:
Shouldn’t be callable whenever (
rewardscycle
should be scheduled)
Preconditions:
Assumes that the
rewardsCycle
is startable.Also assumes that each allotment is > 0. works even if that’s not the case.
Inputs:
There aren’t input values here.
Function call analysis
nopClaim.setRewardsCycleTotal(nopClaimContractAllotment)
What is controllable? -
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? no problems
vault.transferToken()
What is controllable? -
If return value controllable, how is it used and how can it go wrong? there isn’t return value here
What happens if it reverts, reenters, or does other unusual control flow? revert if
tokenBalance
is less thanamount
value, or ifamount
is zero
Function: distributeMultisigAllotment()
distributeMultisigAllotment()
Intended behavior:
Should distribute the
ggp
to the multisigs.
Branches and code coverage:
Intended branches:
Should only be called with legitimate
ggp
tokens.
Negative behavior:
Lacks negative testing
Should not distribute rewards to deactivated
multisigs
.
Preconditions:
Assumes there aren’t that many multisigs
Assumes that if multisigs gets deleted, they won’t be eligible for
rewards
.
Inputs:
allotment
:Control : value is calculated inside
getClaimingContractDistribution(“ClaimMultisig”)
Checks : no checks at this function level, however, there maybes ome leftover tokens due to rounding errors; assure that these are sent somewhere after all allotments? (in
startRewardsCycle
)Impact : determines the total amount of tokens that will be sent to multisigs.
vault
:Control : address is taken from
Vault(getContractAddress(“Vault”))
Checks : passed from previous function; same as
ggp
parameter.Impact : n/a
ggp
:Control : address is taken from
TokenGGP(getContractAddress(“TokenGGP”))
Checks : full control; it’s passed from the previous function; ENSURE that it’s never called somewhere else or with a different GGP than here
Impact : n/a
Function call analysis
mm.getCount();
What is controllable? -
If return value controllable, how is it used and how can it go wrong? out of gas inside the for loop if count value is too big
What happens if it reverts, reenters, or does other unusual control flow? no problems
mm.getMultisig(i)
What is controllable? -
If return value controllable, how is it used and how can it go wrong? returns address and status of multisig, if enabled then this address will receive ggp tokens.
What happens if it reverts, reenters, or does other unusual control flow? no problems
vault.withdrawToken(enabledMultisigs[i], ggp, tokensPerMultisig)
What is controllable? since this is an internal call, all input values are taken from storage.
If return value controllable, how is it used and how can it go wrong? there isn’t return value here.
What happens if it reverts, reenters, or does other unusual control flow? will revert if
safeTransfer
call reverts and iftokenBalances
less than amount value
5.16 File: Staking
Staking
Function: GGP staking components
GGP staking components
Intended behavior:
Limited negative testing
getGGPStake
= view current stakeincreaseGGPStake
= increase.ggpStaked
decreaseGGPStake
= decrease.ggpStaked
Branches and code coverage:
Intended branches:
Should retrieve / increase / decrease the
ggpStaked
.
Negative behavior:
Shouldn’t update an unregistered
stakerIndex
.
Preconditions:
increase
assumes that user hasdeposited
theggp
and that the contract’s balance has/will increasedecrease
assumes that the user haswithdrawn
and that theggp
balance of the contract willdecrease + ggp
balance of user willincrease
.
Where are the functions used:
increaseGGPSTake
: Used in_stakeGGP
decreaseGGPStake
: Used inslashGGP
,withdrawGGP
Function: increaseAVAXStake()
increaseAVAXStake()
Intended behavior:
Increase the amount of AVAX for stakerAddr.
The function is called only from MinipoolManager.createMinipool
.
Branches and code coverage:
Intended branches:
After the function call the
getAVAXStake
forstakerAddr
increased by theamount
value
Negative behavior:
The function will revert if
stakerAddr
is not valid stakerThe function will revert if msg.sender is not
MinipoolManager
contract
Preconditions:
stakerAddr
calledstakeGGP
and was registered as a staker.
Inputs:
msg.sender
:Control : -
Checks :
onlySpecificRegisteredContract(“MinipoolManager”, msg.sender)
Impact : access to the function by untrusted addresses will allow manipulating the number of tokens staked.
amount
:Control :
msg.value
is passed from the functionMinipoolManager.createMinipool
to ths function. limited control.Checks : there are no checks.
Impact : this value reflects the number of stacked tokens. manipulating this value will allow an attacker to specify the number of tokens that have not actually been deposited.
stakerAddr
:Control :
msg.sender
fromMinipoolManager.createMinipool
. not controlled.Checks : the
requireValidStaker
function checks the address. If this address isn’t staker, will revert.Impact : in case of full access it will allow any user to increase the number of tokens deposited.
Function call analysis
requireValidStaker()
What is controllable?
stakerAddr
If return value controllable, how is it used and how can it go wrong? return the
stakerIndex
corresponding to thestakerAddr
. The Index must be unique, otherwise will be possible to lose funds.What happens if it reverts, reenters, or does other unusual control flow? will be reverted if
stakerAddr
is not a valid staker.
addUint()
- What is controllable? amountIf return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? it can be reverted in overflow case,
Function: decreaseAVAXStake()
decreaseAVAXStake()
Intended behavior:
Decrease the amount of AVAX for stakerAddr.
The function is called from MinipoolManager.withdrawMinipoolFunds
and MinipoolManager._cancelMinipoolAndReturnFunds
.
Branches and code coverage:
Intended branches:
After the function call the
getAVAXStake
forstakerAddr
decreased by theamount
value
Negative behavior:
The function will revert if
stakerAddr
is not valid stakerThe function will revert if the
avaxStaked
for thestakerAddr
is less thanamount
The function will revert if msg.sender is not
MinipoolManager
contract
Preconditions:
stakerAddr
have calledstakeGGP
and was registered as a staker.stakerAddr
has non zeroavaxStaked
value
Inputs:
msg.sender
:Control : -
Checks :
onlySpecificRegisteredContract(“MinipoolManager”, msg.sender)
Impact : access to the function by untrusted addresses will allow manipulating the number of tokens staked
amount
:Control :
getUint(keccak256(abi.encodePacked(“minipool.item”, minipoolIndex, “.avaxNodeOpAmt”)))
value fromgogoStorage
, limited control.Checks : this value cannot be more than current the
avaxStaked
valueImpact : this value reflects the number of stacked tokens. manipulating this value will allow an attacker to specify the number of tokens that have not actually been withdrawn.
stakerAddr
:Control : owner of minipool. not controlled.
Checks : the
requireValidStaker
function checks the address. If this address isn’t staker, will revert.
Impact : in case of full access it will allow any user to decrease the number of tokens deposited.
Function call analysis
subUint()
What is controllable? amount
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? will revert if
avaxStaked
less thanamount
.
requireValidStaker()
What is controllable? stakerAddr
If return value controllable, how is it used and how can it go wrong? re- turn the
stakerIndex
corresponding to thestakerAddr
. The Index must be unique, otherwise will be possible to lose funds.What happens if it reverts, reenters, or does other unusual control flow? will be reverted ifstakerAddris not a valid staker.
Function: increaseAVAXAssigned()
increaseAVAXAssigned()
Intended behavior:
Increase the amount of AVAX a given staker is assigned by the protocol
The function is called only from MinipoolManager.createMinipool
.
Branches and code coverage:
Intended branches:
After the function call the
getAVAXAssigned
forstakerAddr
increased by theamount
value
Negative behavior:
The function will revert if
stakerAddr
is not valid stakerThe function will revert if msg.sender is not
MinipoolManager
contract
Preconditions:
stakerAddr
have calledstakeGGP
and was registered as a staker.
Inputs:
amount
:Control :
avaxAssignmentRequest
is passed from the functionMinipoolManager.createMinipool
to ths function and should be equal themsg.sender
value. limited control.Checks : there are no checks
Impact : this value reflects the number of assigned tokens. Manipulating this value will allow an attacker to specify the number of tokens that have not actually been assigned.
msg.sender
:Control : -
Checks :
onlySpecificRegisteredContract(“MinipoolManager”, msg.sender)
Impact : access to the function by untrusted addresses will allow manipulating the number of tokens assigned.
stakerAddr
:Control :
msg.sender
fromMinipoolManager.createMinipool
. not controlled.Checks : the
requireValidStaker
function checks the address. If this address isn’t staker, will revert.Impact : in case of full access it will allow any user to increase the number of tokens assign.
Function call analysis
setUint(...))“.avaxAssignedHighWater”)
What is controllable? -
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? no problems
addUint(...))“.avaxAssigned”)
What is controllable? amount
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? no problems
Function: decreaseAVAXAssigned()
decreaseAVAXAssigned()
Intended behavior:
Allows to decrease the amount of AVAX a given staker is assigned by the protocol
The function is called from MinipoolManager.recordStakingEnd
and MinipoolManager. recordStakingError
and MinipoolManager._cancelMinipoolAndReturnFunds
.
Branches and code coverage:
Intended branches:
After the function call the
getAVAXAssigned
forstakerAddr
decreased by theamount
value
Negative behavior:
The function will revert if
stakerAddr
is not valid stakerThe function will revert if the
avaxAssigned
for thestakerAddr
is less thanamount
The function will revert if msg.sender is not
MinipoolManager
contract
Preconditions:
stakerAddr
have calledstakeGGP
and was registered as a staker.stakerAddr
has non zeroavaxAssigned
value
Inputs:
msg.sender
:Control : -
Checks :
onlySpecificRegisteredContract(“MinipoolManager”, msg.sender)
Impact : access to the function by untrusted addresses will allow manipulating the number of tokens assign.
amount
:Control :
getUint(keccak256(abi.encodePacked(“minipool.item”, minipoolIndex, “.avaxLiquidStakerAmt”)))
value fromgogoStorage
, limited control.Checks : this value cannot be more than current the
avaxAssigned
valueImpact : this value reflects the number of staked tokens. Manipulating this value will allow an attacker to specify the number of tokens that have not actually been deposited.
stakerAddr
:Control : owner of minipool. not controlled.
Checks : the
requireValidStaker
function checks the address. If this address isn’t staker, will revert.Impact : incase of full access i will allow any user to decreased the number of tokens assigned.
Function call analysis
subUint()
What is controllable? amount
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? will revert if
avaxAssigned
less thanamount
.
requireValidStaker()
What is controllable? stakerAddr
If return value controllable, how is it used and how can it go wrong? return the
stakerIndex
corresponding to thestakerAddr
. The Index must be unique, otherwise will be possible to lost funds.What happens if it reverts, reenters, or does other unusual control flow? will be reverted if
stakerAddr
is not a valid staker.
Function: setRewardsStartTime
setRewardsStartTime
Intended behavior:
Rewards start time refers to the
timestamp
when the staker registered forGGPRewards
Branches and code coverage:
Intended branches:
Ensure that
time
is in the future?Should allow
setting
therewardStartTime
Negative behavior:
Also, assuming that
onlyRegisteredNetworkContract
calls it. Also I think they whitelist their ownStaking
contract (basically address(this))
Preconditions:
Assumes that it’s called from
onlySpecificRegisteredContract(“ClaimNodeOp”, msg.sender)
Inputs:
time
:Control : full control
Checks : there’s no check on whether the
time
is in the future or notImpact : the value is used during reward distribution, if zero, the staker will not receive reward
Function call analysis
There aren’t external calls here.
Where are the functions used:
setRewardsStartTime
: used inMinipoolManager
andClaimNodeOp
Function: GGP Rewards()
GGP Rewards()
Intended behavior:
Should
get, increase, decrease
theGGPRewards
assigned to a staker.
Branches and code coverage:
Intended branches:
These should update whenever the staker claims / is issued rewards.
Should retrieve/increase/decrease the amount of
GGPrewards
a staker has earned and not claimed yet.
Negative behavior:
Should revert if anyone other than the
ClaimNodeOp
contract calls them.
Preconditions:
Assumes that the calling contract holds the correct accounting for how the
ggp
rewards are issued and maintained.
Function call analysis
There aren’t external calls here.
Where are the functions used:
increaseGGPRewards
: used inClaimNodeOP
decreaseGGPrewards
: used inClaimNodeOP
Function: increaseMinipoolCount()
increaseMinipoolCount()
Intended behavior:
The function is called from MinipoolManager.createMinipool
Increase the number of minipools the given staker has
Branches and code coverage:
Intended branches:
After the function call the
.minipoolCount
increased by 1
Negative behavior:
The function will revert if the
.minipoolCount
is zeroThe function will revert if msg.sender is not
MinipoolManager
contract
Preconditions:
stakerAddr
have calledstakeGGP
and was registered as a staker.
Inputs:
stakerAddr
:Control : owner of minipool. not controlled.
Checks : the
requireValidStaker
function checks the address. If this address isn’t staker, will revert.Impact : in case of full access it will allow any user to increase the amount of minipools
msg.sender
:Control : -
Checks :
onlySpecificRegisteredContract(“MinipoolManager”, msg.sender)
Impact : access to the function by untrusted addresses will allow manipulating the number of the given staker minipools. The
setRewardsStartTime
value depends of the amount of minipools, if minipoolCount = 0RewardsStartTime
will be reset. IfRewardsStartTime == 0
thenRewardsStartTime
will be set during minipool creation. And ifRewardsStartTime == 0
then owner of minipool doesn’t get the GGP rewards
Function call analysis
addUint()
What is controllable? -
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? no problems
requireValidStaker()
What is controllable? stakerAddr
If return value controllable, how is it used and how can it go wrong? return the
stakerIndex
corresponding to thestakerAddr
. The Index must be unique, otherwise will be possible to lost funds.What happens if it reverts, reenters, or does other unusual control flow? will be reverted if
stakerAddr
is not a valid staker.
Function: decreaseMinipoolCount()
decreaseMinipoolCount()
Intended behavior:
Decrease the number of minipools the given staker has
The function is called from MinipoolManager.recordStakingEnd
and MinipoolManager._cancelMinipoolAndReturnFunds
Branches and code coverage:
Intended branches:
After the function call the
.minipoolCount
decreased by 1
Negative behavior:
The function will revert if
stakerAddr
is not valid stakerThe function will revert if the
.minipoolCount
is zeroThe function will revert if msg.sender is not
MinipoolManager
contract
Preconditions:
stakerAddr
have calledstakeGGP
and was registered as a staker.The
.minipoolCount
is not zero
Inputs:
stakerAddr
:Control : owner of minipool. not controlled.
Checks : the
requireValidStaker
function checks the address. If this address isn’t staker, will revert.Impact : in case of full access it will allow any user to decrease the amount of minipools
msg.sender
:Control : -
Checks :
onlySpecificRegisteredContract(“MinipoolManager”, msg.sender)
Impact : access to the function by untrusted addresses will allow manipulating the number of the given staker minipools. The
setRewardsStartTime
value depends of the amount of minipools, if minipoolCount = 0RewardsStartTime
will be reset. ifRewardsStartTime == 0
thenRewardsStartTime
will be set during minipool creation. And ifRewardsStartTime == 0
then owner of minipoll doesn’t get the GGP rewards
Function call analysis
subUint()
What is controllable? -
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? will revert if
.minipoolCount
is 0.
requireValidStaker()
What is controllable? stakerAddr
If return value controllable, how is it used and how can it go wrong? return the
stakerIndex
corresponding to thestakerAddr
. The Index must be unique, otherwise will be possible to lost funds.What happens if it reverts, reenters, or does other unusual control flow? will be reverted if
stakerAddr
is not a valid staker.
Function: setRewardsStartTime()
setRewardsStartTime()
Intended behavior:
Set the timestamp when the staker registered for GGP rewards.
The setRewardsStartTime
value depends of the amount of minipools, if minipoolCount = 0 RewardsStartTime
will be reset inside the calculateAndDistributeRewards()
function, which called from processGGPRewards
if isEligible
true (is not true if RewardsStartTime == 0
). if RewardsStartTime == 0
then RewardsStartTime
will be set during minipool creation.
Branches and code coverage:
Intended branches:
After the function call the
.rewardsStartTime
is equal totime
Negative behavior:
The function will revert if
stakerAddr
is not valid stakerThe function will revert if
msg.sender
is notRegisteredNetworkContract
Preconditions:
stakerAddr
have calledstakeGGP
and was registered as a staker.
Inputs:
time
:Control : partly controlled: during minipool creation
block.timestamp
is passedChecks : there aren’t any checks
Impact : if set to 0 than owner of minipool cannot get the GGP rewards and if non zero will be able to get
(isEligible(): if (block.timestamp - rewardsStartTime) != dao.getRewardsEligibilityMinSeconds())
stakerAddr
:Control : owner of minipool. not controlled.
Checks : the
requireValidStaker
function checks the address. If this address isn’t staker, will revert.Impact : in case of full access it will allow any user to set the
RewardsStartTime
and bypass theisEligible
check.
msg.sender
:Control : -
Checks :
onlyRegisteredNetworkContract
Impact : access to the function by untrusted addresses will allow manipulating the
RewardsStartTime
value. IfRewardsStartTime != 0
then owner of minipool will be able to get the GGP rewards
Function call analysis
setUint()
What is controllable? -
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? no problems
requireValidStaker()
What is controllable? stakerAddr
If return value controllable, how is it used and how can it go wrong? re- turn the
stakerIndex
corresponding to thestakerAddr
. The Index must be unique, otherwise will be possible to lost funds.What happens if it reverts, reenters, or does other unusual control flow? will be reverted if
stakerAddr
is not a valid staker.
Function: increaseGGPRewards()`
Intended behavior:
Increase the amount of GGP rewards the staker has earned and not claimed
The function is called from ClaimNodeOp.calculateAndDistributeRewards
Branches and code coverage:
Intended branches:
After the call the
.ggpRewards
amount will be increased byamount
Negative behavior:
The function will revert if
stakerAddr
is not valid stakerThe function will revert if
msg.sender
is notClaimNodeOp
contract
Preconditions:
stakerAddr
have calledstakeGGP
and was registered as a staker.
Inputs:
amount
:Control :
Checks : there aren’t checks
Impact : The value determines how much the user will be able to receive rewards. In case of full access to the function, users will be able to steal all funds from the vault.
stakerAddr
:Control : owner of minipool. not controlled.
Checks : the
requireValidStaker
function checks the address. If this address isn’t staker, will revert.Impact : in case of full access it will allow any user to increase the.ggpRewards
msg.sender
:Control : -
Checks :
onlySpecificRegisteredContract(“ClaimNodeOp”, msg.sender)
Impact : access to the function by untrusted addresses will allow manipulating the
.ggpRewards
value.
Function call analysis
requireValidStaker()
What is controllable? stakerAddr
If return value controllable, how is it used and how can it go wrong? re- turn the
stakerIndex
corresponding to thestakerAddr
. The Index must be unique, otherwise will be possible to lost funds.What happens if it reverts, reenters, or does other unusual control flow? will be reverted if
stakerAddr
is not a valid staker.
addUint()
What is controllable? amount
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? no problems
Function: decreaseGGPRewards()
decreaseGGPRewards()
Intended behavior:
Decrease the amount of GGP rewards the staker has earned and not claimed.
The function is called from ClaimNodeOp.claimAndRestake
Branches and code coverage:
Intended branches:
After the call the
.ggpRewards
is decreased by theamount
value.
Negative behavior:
The function will revert if
stakerAddr
is not valid stakerThe function will revert if the
.ggpRewards
is less thanamount
The function will revert if msg.sender is not
ClaimNodeOp
contract
Preconditions:
stakerAddr
have calledstakeGGP
and was registered as a staker.The
.ggpRewards
is set by theClaimNodeOp.calculateAndDistributeRewards
function call
Inputs:
amount
:Control : not controlled
Checks : there aren’t checks
Impact : in case of an untrusted caller, the
.ggpRewards
can be reset and owner of pool will not be able to get reward
stakerAddr
:Control : owner of minipool. not controlled.
Checks : the
requireValidStaker
function checks the address. If this address isn’t staker, will revert.
Impact : in case of full access it will allow any user to decrease the
.ggpRewards
msg.sender
:Control : -
Checks :
onlySpecificRegisteredContract(“ClaimNodeOp”, msg.sender)
Impact : access to the function by untrusted addresses will allow manipulating the
.ggpRewards
value.
Function call analysis
requireValidStaker()
What is controllable? stakerAddr
If return value controllable, how is it used and how can it go wrong? return the
stakerIndex
corresponding to thestakerAddr
. The Index must be unique, otherwise will be possible to lost funds.What happens if it reverts, reenters, or does other unusual control flow? will be reverted if
stakerAddr
is not a valid staker.
subUint()
What is controllable? amount
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? will revert if
.ggpRewards
is less thanamount
.
Function: setLastRewardsCycleCompleted()
setLastRewardsCycleCompleted()
Intended behavior:
Set the most recent reward cycle number that the staker has been paid out for.
The function is called from ClaimNodeOp.calculateAndDistributeRewards
Branches and code coverage:
Intended branches:
After the call the
.lastRewardsCycleCompleted
is equal to thecycleNumber
value
Negative behavior:
The function will revert if
stakerAddr
is not valid stakerThe function will revert if msg.sender is not
ClaimNodeOp
contract
Preconditions:
stakerAddr
have calledstakeGGP
and was registered as a staker.
Inputs:
cycleNumber
:Control : the value from the
rewardsPool.getRewardsCycleCount()
function callChecks : there aren’t checks
Impact : prevents rereceiving the reward in the same cycle.
stakerAddr
:Control : owner of minipool. not controlled.
Checks : the
requireValidStaker
function checks the address. If this address isn’t staker, will revert.Impact : in case of full access it will allow any user to decrease the
.ggpRewards
msg.sender
:Control : -
Checks :
onlySpecificRegisteredContract(“ClaimNodeOp”, msg.sender)
Impact : access to the function by untrusted addresses will allow manipulating the
.lastRewardsCycleCompleted
value.
Function call analysis
requireValidStaker()
What is controllable? stakerAddr
If return value controllable, how is it used and how can it go wrong? re- turn the
stakerIndex
corresponding to thestakerAddr
. The Index must be unique, otherwise will be possible to lost funds.What happens if it reverts, reenters, or does other unusual control flow? will be reverted if
stakerAddr
is not a valid staker.
setUint()
What is controllable?
cycleNumber
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? no problems
Function: getMinimumGGPStake()
getMinimumGGPStake()
Intended behavior:
Retrieve staker’s minimum
GGP
stake, based on currentGGP
price.
Branches and code coverage:
Intended branches:
Ensure that
stakerAddr
is valid; currently not checked
Preconditions:
Assumes that the
stakerAddr
has someavaxAssigned
to them.
Function call analysis
(uint256 ggpPriceInAvax, ) = oracle.getGGPPriceInAVAX();
What is controllable? -
If return value controllable, how is it used and how can it go wrong? Part of the return value is ignored (that refers to the block.timestamp when the price has been updated) Maybe it’s a good idea to also return that? The price could be really outdated; Add something like a max amount of blocks that go without update?
What happens if it reverts, reenters, or does other unusual control flow? will revert if
price
is zero
Function: getCollateralizationRatio()
getCollateralizationRatio()
Intended behavior:
Return collateralization ratio based on current
GGP
price.
Branches and code coverage:
Intended branches:
Ensure that
stakerAddr
is valid; currently not checked
Preconditions:
Assumes that the
stakerAddr
has someavaxAssigned
to them.
Function call analysis
(uint256 ggpPriceInAvax, ) = oracle.getGGPPriceInAVAX();
What is controllable? -
If return value controllable, how is it used and how can it go wrong? Part of the return value is ignored (that refers to the block.timestamp when the price has been updated) Maybe it’s a good idea to also return that? The price could be really outdated; Add something like a max amount of blocks that go without update?
What happens if it reverts, reenters, or does other unusual control flow? will revert if
price
is zero
Where is the function used:
MinipoolManager
:Staking
:
Function: getEffectiveRewardsRatio()
getEffectiveRewardsRatio()
Intended behavior:
return effective collateralization ratio used to pay rewards based on
GGP
price andAVAX
high water.
Branches and code coverage:
Intended branches:
Ensure that
stakerAddr
is valid; currently not checked
Preconditions:
Assumes that the
stakerAddr
has someGGPstaked
already.
Function call analysis
(uint256 ggpPriceInAvax, ) = oracle.getGGPPriceInAVAX();
What is controllable? -
If return value controllable, how is it used and how can it go wrong? Part of the return value is ignored (that refers to the block.timestamp when the price has been updated) Maybe it’s a good idea to also return that? The price could be really outdated; Add something like a max amount of blocks that go without update?
What happens if it reverts, reenters, or does other unusual control flow? will revert if
price
is zero
dao.getMaxCollateralizationRatio();
What is controllable? -
If return value controllable, how is it used and how can it go wrong? re- turn the max collateralization ratio of GGP to Assigned AVAX eligible for rewards. This value is used for
EffectiveGGPStaked
value calculations for reward distribution processWhat happens if it reverts, reenters, or does other unusual control flow? no problems
Function: getEffectiveGGPStaked()
getEffectiveGGPStaked()
Intended behavior:
Get amount of
ggp
that will count towards therewards
cycle.
Branches and code coverage:
Intended branches:
Ensure that
stakerAddr
is valid; currently not checked
Preconditions:
the
price
value is set inside Oracle contract
Function call analysis
(uint256 ggpPriceInAvax, ) = oracle.getGGPPriceInAVAX();
What is controllable? -
If return value controllable, how is it used and how can it go wrong? Part of the return value is ignored (that refers to the block.timestamp when the price has been updated) Maybe it’s a good idea to also return that? The price could be really outdated; Add something like a max amount of blocks that go without update?
What happens if it reverts, reenters, or does other unusual control flow? will revert if
price
is zero
Where is the function used:
ClaimNodeOp
:
Function: stakeGGP()
and _stakeGGP
stakeGGP()
and _stakeGGP
Intended behavior:
Should allow any user to stake
GGP
into the contract.
Branches and code coverage:
Intended branches:
Should revert if
msg.sender
transferred less thanamount
tokens.The ggp balance of the
msg.sender
should deplete by amount, whilst thecontract
should have enough todeposit
into thevault(like a middleman)
The
GGPStake
of the user should be increased by thestaked
amount.
Negative behavior:
Limited negative testing
Shouldn’t allow transferring arbitrary tokens
Preconditions:
Assumes
msg.sender
is registered as a staker in the contract; however, if that’s not the case, it creates an index for a new staker:Assumes that
msg.sender
has previously approved the amount that is to be transferred bystakeGGP
.
Inputs:
amount
:Control : full control
Checks : there are no 0 checks, however, they do
safeTransferFrom
user with theamount
Impact : n/a
Function call analysis
ggp.safeTransferFrom()
What is controllable? amount
If return value controllable, how is it used and how can it go wrong? there ins’t return value
What happens if it reverts, reenters, or does other unusual control flow? will revert if msg.sender doesn’t have enough ggp tokens.
_stakeGGP()
What is controllable? amount
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? no problems
Function: restakeGGP()
restakeGGP()
Intended behavior:
allow restaking for
claimedGGP
rewards
Branches and code coverage:
Intended branches:
after the call the
.ggpStaked
value ofstakerAddr
will be increased byamount
value
Negative behavior:
Limited negative testing
if msg.sender doesn’t have enough ggp tokens, transaction will be reverted
if msg.sender is not trusted ClaimNodeOp contract, transaction will be reverted
Preconditions:
Assumes
msg.sender
isClaimNodeOp
msg.sender must have at least the amount value of ggp tokens
Inputs:
amount
:Control : limited control
Checks : safeTransferFrom will revert if msg.sender balance less than
amount
Impact : -
stakerAddr
:Control : full control
Checks : there aren’t any checks
Impact : -
msg.sender
:Control : -
Checks :
onlySpecificRegisteredContract(“ClaimNodeOp”, msg.sender)
Impact : the function allows caller to increase
.ggpStaked
value for any user. but caller should send this value of ggp tokens to contract
Function call analysis
ggp.safeTransferFrom()
What is controllable? amount
If return value controllable, how is it used and how can it go wrong? there ins’t return value
What happens if it reverts, reenters, or does other unusual control flow? will revert if msg.sender doesn’t have enough ggp tokens.
_stakeGGP()
What is controllable? amount
If return value controllable, how is it used and how can it go wrong? there isn’t return value
What happens if it reverts, reenters, or does other unusual control flow? no problems
Function: withdrawGGP()
withdrawGGP()
Intended behavior:
Allows withdrawing
GGP
tokens.
Branches and code coverage:
Intended branches:
Should ensure that the
.ggpStaked
decreases.
Negative behavior:
Should never lock-up ggp; this could happen i a scenario where the
msg.sender
is never over 150% collateralization
Preconditions:
Assumes that the user is over
150%
in collateralization ratio.Assure that
maxCollateralizationRatio
is synced up! Maybe check the last block and compare it with the last block fromgetCollateralizationRatio
as well?! a de-sync could lead to lower threshold of withdrawals. Any huge fluctuations would greatly affect this.
Inputs:
amount
:Control : full controll
Checks : checks that
amount > getGGPStake
and check thatgetCollateralizationRatio(msg.sender)
at least 150 after withdraw
Impact : could lead to loss of funds if not depleted properly.
Function: slashGGP()
slashGGP()
Intended behavior:
Should be used by the
MinipoolManage
in case that aminipool
has ended; this happen
Branches and code coverage:
Intended branches:
Decrease the
ggpStake
of thestaker
(assumingstaker
has some left)StakerAddr
must be registered.
Negative behavior:
Only allow
minipoolmanager
to call this.
Preconditions:
Assumes that
decreaseGGPSTake
can be called on thestakerAddr
(this implies thatstakerAddr
has been registered beforehand)
Inputs:
ggpAmt
:Control : full control
Checks : assumes that
decreaseGGPStake
properly decreases the amount that thestakerAddr
hasImpact : n/a
6 Audit Results
At the time of our audit, the code was not deployed to mainnet Avalanche.
During our assessment on the scoped GoGoPool contracts, we discovered seven findings. Of the seven findings, four were of high severity, one was of medium severity, one was of low severity and the remaining finding was informational. Multisig Labs acknowledged all findings and implemented fixes.
6.1 Disclaimers
This assessment does not provide any warranties about finding all possible issues within its scope; in other words, the evaluation results do not guarantee the absence of any subsequent issues. Zellic, of course, also cannot make guarantees about any additional code added to the assessed project after the audit version of our assessment. Furthermore, because a single assessment can never be considered comprehensive, we always recommend multiple independent assessments paired with a bug bounty program.
For each finding, Zellic provides a recommended solution. All code in these recommendations are intended to convey how an issue may be resolved (i.e., the idea), but they may not be tested or functional code.
Finally, the contents of this assessment report are for informational purposes only; do not construe any information in this report as legal, tax, investment, or financial advice. Nothing contained in this report constitutes a solicitation or endorsement of a project by Zellic.
Last updated