February 22, 2023
Prepared for: Multisig Labs
Prepared by: Katerina Belotskaia and Vlad Toie
Zellic Inc.
- About Zellic
- 1 Executive Summary
- 2 Introduction
- 3 Detailed Findings
- 3.1 The
transferAVAX
function allows arbitrary transfers - 3.2 Ocyticus does not include the
Staking
pause - 3.3 The reward amount manipulation.
- 3.4 Network registered contracts have absolute storage control
- 3.5 Oracle may reflect an outdated price
- 3.6 Fields are not reset exactly after their usage
- 3.7 Contracts can deposit arbitrary tokens in the Vault
- 3.1 The
- 4 Discussion
- 4.1 The
rewardsCycleEnd
calculation - 4.2 Lack of checks.
- 4.3 The process of distributing ggp rewards
- 4.4 Checks-effects-interactions pattern
- 4.5 Missing status update
- 4.6 Unused variables
- 4.7 Contract upgrades
- 4.8 IWithdrawer inheritance
- 4.9 Protocol DAO setters range
- 4.10 Leftover tokens in
RewardsPool
- 4.1 The
- 5 Threat Model
- 5.1 File:
TokenggAVAX
- 5.2 File:
ClaimNodeOP
- 5.3 File:
ClaimProtocolDAO.sol
- 5.4 File:
BaseUpgradeable.sol
- 5.5 File:
Base.sol
- 5.6 File:
BaseAbstract.sol
- 5.7 File:
Storage.sol
- 5.8 File:
TokenGGP.sol
- 5.9 File:
Vault.sol
- 5.10 File:
MinipoolManager
- 5.11 File:
MultisigManager
- 5.12 File:
Ocyticus
- 5.13 File:
Oracle
- 5.14 File:
ProtocolDAO
- 5.15 File:
RewardsPool
- 5.16 File:
Staking
- 5.1 File:
- 6 Audit Results
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.
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 ( 4 )1 at the end of the document.
Impact Level | Count |
---|---|
Critical | 0 |
High | 4 |
Medium | 1 |
Low | 1 |
Informational | 1 |
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.
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.
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
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:
Jasraj Bedi , Co-founder
jazzy@zellic.io
Chad McDonald , Engagement Manager
chad@zellic.io
The following consultants were engaged to conduct the assessment:
Katerina Belotskaia , Engineer
kate@zellic.io
Vlad Toie , Engineer
vlad@zellic.io
The key dates of the engagement are detailed below.
November 14, 2022 Kick-off call
November 14, 2022 Start of primary review period
November 28, 2022 End of primary review period
- Target : Vault.sol
- Category : Business Logic
- Likelihood : Medium
- Severity : High
- Impact : High
The transferAVAX
function is used to perform transfers of avax between two registered contracts.
function transferAVAX (
string memory fromContractName,
string memory toContractName,
uint256 amount
) external onlyRegisteredNetworkContract{
// Valid Amount?
if (amount == 0 ) {
revertInvalidAmount();
}
// Emit transfer event
emit AVAXTransfer(fromContractName, toContractName, amount);
// Make sure the contracts are valid, will revert if not
getContractAddress(fromContractName);
getContractAddress(toContractName);
// Verify there are enough funds
if (avaxBalances[fromContractName] < amount) {
revert InsufficientContractBalance();
}
// Update balances
avaxBalances[fromContractName] = avaxBalances[fromContractName] - amount;
avaxBalances[toContractName] = avaxBalances[toContractName] + amount;
}
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.
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.
We recommend removing the fromContractName
parameter altogether and ensuring that the funds can only be transferred by the caller of the function, msg.sender
.
// @audit-info doesn't exist in rocketvault
function transferAVAX (
string memory fromContractName,
string memory toContractName,
uint256 amount
) external onlyRegisteredNetworkContract {
// Valid Amount?
if(amount=) 0 ) {
revert InvalidAmount();
}
// Emit transfer event
emit AVAXTransfer(msg.sender, toContractName, amount);
// Make sure the contracts are valid, will revert if not
getContractAddress(msg.sender);
getContractAddress(toContractName);
// Verify there are enough funds
if (avaxBalances[msg.sender] < amount) {
revert InsufficientContractBalance();
}
// Update balances
avaxBalances[msg.sender] = avaxBalances[msg.sender] - amount;
avaxBalances[toContractName] = avaxBalances[toContractName] + amount;
}
The issue has been fixed by Multisig Labs in commit 84211f.
- Target : Ocyticus, Staking
- Category : Business Logic
- Likelihood : Medium
- Severity : High
- Impact : High
The pauseEverything
and resumeEverything
functions are used to restrict access to important functions.
function pauseEverything() external onlyDefender {
ProtocolDAO dao = ProtocolDAO(getContractAddress(“ProtocolDAO”));
dao.pauseContract(“TokenggAVAX”);
dao.pauseContract(“MinipoolManager”);
disableAllMultisigs();
}
/// @notice Reestablish all contract's abilities
/// @dev Multisigs will need to be enabled seperately, we dont know which ones to enable
function resumeEverything() external onlyDefender {
ProtocolDAO dao = ProtocolDAO(getContractAddress(“ProtocolDAO”));
dao.resumeContract(“TokenggAVAX”);
dao.resumeContract(“MinipoolManager”);
}
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.
Should an emergency arise, pauseEverything
will be called. In this case, Staking
will be omitted, which could put user funds in danger.
We recommend ensuring that the Staking
contract is also paused in the pauseEverything
function as well as un-paused in the resumeEverything
function.
function pauseEverything() external onlyDefender {
ProtocolDAO dao = ProtocolDAO(getContractAddress(“ProtocolDAO”));
dao.pauseContract(“TokenggAVAX”);
dao.pauseContract(“MinipoolManager”);
dao.pauseContract(“Staking”);
disableAllMultisigs();
}
/// @notice Reestablish all contract's abilities
/// @dev Multisigs will need to be enabled separately, we don't know which ones to enable
function resumeEverything() external onlyDefender {
ProtocolDAO dao = ProtocolDAO(getContractAddress(“ProtocolDAO”));
dao.resumeContract(“TokenggAVAX”);
dao.resumeContract(“MinipoolManager”);
dao.resumeContract(“Staking”);
}
The issue has been fixed by Multisig Labs in commit dbc499.
- Target : ClaimNodeOp.sol
- Category : Business Logic
- Likelihood : Medium
- Severity : High
- Impact : High
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.
The attacker can increase their reward portion without actually staking their own funds.
Take into account only the funds actually staked, or check that all minipools have been launched.
The issue has been fixed by Multisig Labs in commits c90b2f and f49931.
- Target : Project-wide
- Category : Business Logic
- Likelihood : Low
- Severity : High
- Impact : High
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.
modifier onlyRegisteredNetworkContract() {
if (booleanStorage[keccak256(abi.encodePacked(“contract.exists”,
msg.sender))] == false && msg.sender != guardian) {
revertInvalidOrOutdatedContract();
}
_;
}
// ...
function setAddress (bytes32 key, address value) external onlyRegisteredNetworkContract {
address Storage[key] = value;
}
function setBool (bytes32 key, bool value) external onlyRegisteredNetworkContract {
boolean Storage[key] = value;
}
function setBytes(bytes32 key, bytes calldata value) external onlyRegisteredNetworkContract {
bytes Storage[key] = value;
}
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
.
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.
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.
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.
- Target : Oracle
- Category : Business Logic
- Likelihood : Medium
- Severity : Medium
- Impact : Medium
Some functions at protocol-level make use of the getGGPPriceInAvax
. This getter retrieves the price
, which is set by the Rialto
multisig.
/// @notice Get the price of GGP denominated in AVAX
/// @return price of ggp in AVAX
/// @return timestamp representing when it was updated
function getGGPPriceInAVAX() external view returns (uint256 price, uint256 timestamp) {
price = getUint(keccak256(“Oracle.GGPPriceInAVAX”));
if(price == 0) {
revert InvalidGGPPrice();
}
timestamp = getUint(keccak256(“Oracle.GGPTimestamp”));
}
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.
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.
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.
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.
- Target : MinipoolManager
- Category : Business Logic
- Likelihood : Low
- Severity : Low
- Impact : Low
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)
function withdrawMinipoolFunds(address nodeID) external nonReentrant {
int256 minipoolIndex = requireValidMinipool(nodeID);
address owner = onlyOwner(minipoolIndex);
requireValidStateTransition(minipoolIndex, MinipoolStatus.Finished);
setUint(keccak256(abi.encodePacked(“minipool.item”, minipoolIndex, “.status”)), uint256(MinipoolStatus.Finished));
uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked(“minipool.item”, minipoolIndex, “.avaxNodeOpAmt”)));
uint256 avaxNodeOpRewardAmt = getUint(keccak256(abi.encodePacked(“minipool.item”, minipoolIndex, “.avaxNodeOpRewardAmt”)));
uint256 totalAvaxAmt = avaxNodeOpAmt + avaxNodeOpRewardAmt;
Staking staking = Staking(getContractAddress(“Staking”));
staking.decreaseAVAXStake(owner, avaxNodeOpAmt);
Vault vault = Vault(getContractAddress(“Vault”));
vault.withdrawAVAX(totalAvaxAmt);
owner.safeTransferETH(totalAvaxAmt);
}
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.
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.
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.
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.
- Target : Vault.sol
- Category : Business Logic
- Likelihood : Medium
- Severity : Low
- Impact : Informational
Multiple functions from the Vault
contract allow arbitrary tokens to be deposited and withdrawn by networkRegistered
contracts. For example, see the depositToken
function:
function depositToken(string memory networkContractName, ERC20 tokenContract, uint256 amount) external guardianOrRegisteredContracts {
// Valid Amount?
if (amount == 0 ) {
revert InvalidAmount();
}
// Make sure the network contract is valid (will revert if not)
getContractAddress(networkContractName);
// Get contract key
bytes32 contractKey = keccak256(abi.encodePacked(networkContractName, address(tokenContract)));
// Emit token transfer event
emit TokenDeposited(contractKey, address(tokenContract), amount);
// Send tokens to this address now, safeTransfer will revert if it fails
tokenContract.safeTransferFrom(msg.sender, address(this), amount);
// Update balances
tokenBalances[contractKey] = tokenBalances[contractKey] + amount;
}
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.
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:
function depositToken(string memory networkContractName, ERC20 tokenContract, uint256 amount) external guardianOrRegisteredContracts {
require(whitelisted[tokenContract], “tokenContract not whitelisted”);
if (amount == 0 ) {
revert InvalidAmount();
}
// ...
The issue has been fixed by Multisig Labs in commit 644e8e.
The purpose of this section is to document miscellaneous observations that we made during the assessment.
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.
The issue has been fixed by Multisig Labs in commit 556ac4.
- 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).
The issue has been fixed by Multisig Labs in commit 878b2e.
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.
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.
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.
The issue has been fixed by Multisig Labs in commit 750812.
In MinipoolManager
the _cancelMinipoolAndReturnFunds
function should reset the rewardsStartTime
if the .minipoolCount
value for staker is zero.
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.
In Storage
, the intStorage
and bytesStorage
mappings and related functions are not used and can be deleted.
The issue has ben acknowledged by the Multisig Labs and they plan to use them in the future.
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.
function initialize() external onlyGuardian {
if(getBool(keccak256(“RewardsPool.initialized”))) {
Notice that the RewardPool.initialized
will always be true after the first contract has been initialized.
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.
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.
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.
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.
The issue has been fixed by Multisig Labs in commit f49931.
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.
The issue has ben acknowledged by the Multisig Labs and they have determined that the amounts would not be significant.
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.
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?
- Test coverage
- Should be called after every upgrade.
- Test coverage
Negative behavior:
- Shouldn’t allow 2 x calling this.
- Negative test?
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.
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.- Test coverage
Negative behavior:
- It cannot be called from any other address.
- Negative test?
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.
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
- Test coverage
lastSync
= current timestamp- Test coverage
lastRewardsAmt\_
= to the amount that rewards will deplete from.- Test coverage
totalReleasedAssets
is calculated correctly for the next cycle - not sure that it is calculated correctly because it happens differently duringinitialize
call- Test coverage
lastRewardsAmt
is calculated for the next cycle if the new reward was deposited.- Test coverage
- if rewards didn’t deposit, the
lastRewardsAmt
will equal 0 for the next cycle- Test coverage
lastRewardsAmt
is calculated correctly and equals 0 for the first cycle- Test coverage
- if nothing changed since the past cycle
lastRewardsAmt
is calculated correctly and equals 0 andtotalReleasedAssets
was increased by the previouslastRewardsAmt
- Test coverage
- current
block.timestamp
should be less thanrewardsCycleEnd
- Test coverage
Negative behavior:
- It basically shouldn’t update unless stuff unless it’s really time to update stuff (see below)
- Negative test?
- Shouldn’t allow calling unless the
rewardsCycle
has passed theblock.timestamp
.- Negative test?
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.
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.- Test coverage
totalAssets
is calculated correctly if the current cycle is going.- Test coverage
- If the current cycle ends and the new one doesn’t start, the
totalAssets
should be equaltotalReleasedAssets\_
+lastRewardsAmt
- Test coverage
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.
- Negative test?
- must not revert (as per eip4626)
- Negative test?
Preconditions:
- Assumes
lastSync
is different than 0 (default value, which is never initialized)? this is missing - assumes 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.
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 call- Test coverage
stakingTotalAssets
will decrease by thebaseAmt
value after the call- Test coverage
baseAmt
+rewardAmt
should be equalmsg.value
- Test coverage
Negative behavior:
- Shouldn’t be callable by anyone (there’s a check put in place, such that only
onlySpecificRegisteredContract
can call the function.- Negative test?
- if
stakingTotalAssets
is less thanbaseAmt
transaction will be rejected- Negative test?
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 : -
- Control : controlled, but actually,it is the value from
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
- What is controllable? the
Intended behavior:
- Should perform the
withdrawalfromwAVAX
, for theMinipoolManager
Branches and code coverage:
Intended branches:
wAVAX.balanceOf(address(this)) -= assets
andbalanceOf(msg.sender) += as sets
- Test coverage
Negative behavior:
- Shouldn’t allow unlimited amount to be withdrawn
- Negative test?
- Shouldn’t be callable when it’s
paused
(has thewhenNotPaused
) modifier- Negative test?
- if
assets
more than theamountAvailableForStaking
transaction will be rejected- Negative test?
- if
asset.balanceOf(address(this))
is less thanassets
transaction will be rejected- Negative test?
- if
msg.sender
is not approved transaction will be rejected- Negative test?
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 important - 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? reenters: no problems because the contract being called is trusted. reverts: no problems
- What is controllable? the
IWAVAX(address(asset)).withdraw(assets);
- What is controllable? the
assets
value; theasset
address is state var - 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
- What is controllable? the
Intended 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!!- Test coverage
- Should transfer the `wAVAX back to the user.
- Test coverage
- Should exchange user’s supplied
AVAX
intowAVAX
- Test coverage
Negative behavior:
- Shouldn’t issue more or less shares than intended.
- Negative test?
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 tokens - 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
- What is controllable?
afterDeposit()
- What is controllable?
assets
- the amount of deposited native tokens - 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
- What is controllable?
\_mint()
- What is controllable?
msg.sender
- is minted tokens receiver address - 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
- What is controllable?
previewDeposit()
&convertToShares()
- What is controllable?
assets
- the amount of deposited native tokens - If 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
- What is controllable?
Intended 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
)- Test coverage
- the
avax
balance of user should increase (byassets
)- Test coverage
- the
shares
of the user should decrease (byshares
)- Test coverage
- make sure that
preivewWithdraw
calculates the shares properly, in all market conditions- Test coverage
Negative behavior:
- shouldn’t allow withdrawing if
_burn
reverted- Negative test?
- shouldn’t allow burning on behalf of other users
- Negative test?
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
- Control : full control; the amount of assets that the user
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 var - 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
- What is controllable? the
Intended 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 correctly- Test coverage
totalReleasedAssets
is decreased byassets
value- Test coverage
msg.sender
received theassets
amount of native tokens- Test coverage
- token gg balance of
msg.sender
is decreased byshares
value- Test coverage
Negative behavior:
- shouldn’t allow withdrawing if
_burn
reverted- Negative test?
- shouldn’t allow burning on behalf of other users
- Negative test?
- revert if
contract.paused
isTrue
- Negative test?
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
- Checks : balance of
- Impact : the number of gg tokens that the user can burn and receive a certain number of native tokens.
- Control : controlled
- `msg.sender``:
- Control : any caller
- Checks : must have the appropriate amount of shares
- Impact : the caller will receive the appropriate amount of native tokens
- Control : any caller
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
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.- Test coverage
- Ensure calculations are properly performed.
- Test coverage
- Increase the
ggpRewards
for thestakerAddr
based on the inputtotalEligibleGGPStaked
.- Test coverage
Negative behavior:
- Should fail if
stakerAddr
is not eligible for rewards.- Negative test?
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 controllable - If 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
- What is controllable?
staking.getEffectiveGGPStaked(stakerAddr);
- What is controllable?
stakerAddr
is controllable - If 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
- What is controllable?
staking.setLastRewardsCycleCompleted(stakerAddr, rewardsPool.getRewardsCycleCount());
- What is controllable?
stakerAddr
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 problem
- What is controllable?
staking.resetAVAXAssignedHighWater(stakerAddr);
- What is controllable?
stakerAddr
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 problem
- What is controllable?
staking.increaseGGPRewards(stakerAddr, rewardsAmt);
- What is controllable?
stakerAddr
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 problem
- What is controllable?
staking.setRewardsStartTime(stakerAddr, 0);
- What is controllable?
stakerAddr
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 problem
- What is controllable?
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
- Test coverage
- Restake the amount of
ggpRewards - claimAmt
- Test coverage
Negative behavior:
- Should not allow claiming more than
msg.sender
was owed- Negative test?
Preconditions:
- Assumes
msg.sender
has some rewards - Assume 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 controllable - 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 there are not enough tokens.
- What is controllable?
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
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.- Test coverage
Negative behavior:
- should be rejected if this contract has not enough ggp tokens in the
vault.tokenBalance
- Negative test?
- should reject if
msg.sender
isn’t the guardian- Negative test?
Preconditions:
msg.sender
is the guardian- tokens 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
The contract is inherited from BaseAbstract.sol
and Initializable.sol
(@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol);
Allows to initialize the gogoStorage
storage address. The function is internal and can be called only once due to onlyInitializing
modifier.
The contract is inherited from BaseAbstract.sol
; The contract contains only constructor with initialization of gogoStorage
address.
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.
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.- Test coverage
Negative behavior:
- Reject if
msg.sender
isn’t the guardian; check put in place.- Negative test?
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.
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
.- Test coverage
Negative behavior:
- Reject if
msg.sender
isn’t thenewGuardian
; check put in place.- Negative test?
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.
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.
- Test coverage; Limited test coverage
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.- Negative test?
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.
The contract is standard ERC20
from @rari-capital/solmate/src/tokens/ERC20.sol
.
Intended behavior:
Allows registered contract to deposit avax.
Branches and code coverage:
Intended branches:
avaxBalances
ofmsg.sender
increased bymsg.value
- Test coverage
Negative behavior:
- if
msg.sender
is notRegisteredNetworkContract
transaction will be reverted- Negative test?
- if msg.value == 0, will be reverted
- Negative test?
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.
Intended behavior:
Allows registered contract to withdraw the deposited avax.
Branches and code coverage:
Intended branches:
- after the call
avaxBalances[msg.sender]
decreased byamount
- Test coverage
Negative behavior:
- if
msg.sender
is notRegisteredNetworkContract
transaction will be reverted- Negative test?
- if
avaxBalances[msg.sender] < amount
, transaction will be reverted- Negative test?
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.
- What is controllable? amount - partly controlled, the
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 amount- Test coverage
avaxBalances[fromContractName]
is decreased by amount- Test coverage
Negative behavior:
- Should be rejected if
avaxBalances[fromContractName] < amount
- Negative test?
- Should be rejected if
toContractName
andfromContractName
is not added togogoStorage
- Negative test?
- Should be rejected if msg.sender is not
fromContractName
- Negative test?
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
- Control : controlled
msg.sender
:- Control : -
- Checks : onlyRegisteredNetworkContract
- Impact : -
- amount:
- Control : controlled
- Checks :
avaxBalances[fromContractName] < amount
- Impact : -
Function call analysis
There aren’t external calls here.
Intended behavior:
Allows registered contract to deposit any tokens
Branches and code coverage:
Intended branches:
tokenBalances
of networkContractName & contractKey is increased byamount
- Test coverage
Negative behavior:
- Should reject if
msg.sender
is notguardianOrRegisteredContracts
- Negative test?
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
Intended behavior:
- Allow
registered msg.sender
to withdraw ERC20 tokens.
Branches and code coverage:
Intended branches:
- Check
withdrawalAddress
?- Test coverage
- Decrease
tokenBalance[paid(caller, token)]
- Test coverage
- Validate the
tokenContract
, such that no arbitrary tokens can be used.- Test coverage
Negative behavior:
- Shouldn’t allow withdrawing more than
msg.sender
owns.- Negative test?
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
- What is controllable?
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.- Test coverage
- Assure both contracts are registered.
- Test coverage
- Compared to the
transferAVAX
, this function does not allow the transfer from arbitrary tokens, and only frommsg.sender
- Test coverage
- Increase
tokenBalances[to]
AND decreasetokenBalances[from]
.- Test coverage
Negative behavior:
- Revert if
msg.sender
is not a registered contract.- Test coverage
- Revert ifmsg.senderdoesn’t have enough tokens amount.
- Test coverage
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.
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)- Test coverage
- Should ensure that the
avaxAssignmentRequest
can be fulfilled (or that it is atleast achievable)- Test coverage
- User’s
avax
balance should deplete, and the contract’s balance should increase.- Test coverage
- After the call, the current state of the minipool is
Prelaunch
- Test coverage
- native token balance of
assets
should increase bymsg.value
- Test coverage
assets
balance ofvault
contract should increase bymsg.value
- Test coverage
- if the pool for
nodeID
exists and the current state is Finished or Canceled minipool data should be reset- Test coverage
- create a new poll if the pool for
nodeID
did not exist before- Test coverage
Staking.sol:getRewardsStartTime(msg.sender)
should be equalblock.timestamp
ifRewardsStartTime
was zero before the call- Test coverage
Staking.sol:getMinipoolCount(msg.sender)
should increase by 1- Test coverage
Staking.sol:getAVAXAssigned(msg.sender)
should increase byavaxAssignmentRequest
- Test coverage
Staking.sol:getAVAXStake(msg.sender)
should increase bymsg.value
- Test coverage
Negative behavior:
- Shouldn’t work when the contract is paused?/
- Negative test? There isn’t test, but function has modifier
whenNotPaused
- Negative test? There isn’t test, but function has modifier
- Should assure that the
nodeId
hasn’t registered beforehand and is unique basically, so no overwrites can be made.- Negative test?
- should revert if minipool for
nodeID
already exists and the currentStatus is not Finished or currentStatus is not Canceled- Negative test?
- should revert if
msg.sender
invalid staker- Negative test?
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 staker- msg.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 this - Impact : could potentially be overwritten.
duration
:- Control : full control
- Checks : There are no checks on the duration amount
- Impact : N/A
- Control : full control
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
- Control : full control; needs to match
- 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
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
)- Test coverage
- Refund all invested funds to the
owner
(deployer)- Test coverage
- 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.- Test coverage
Staking.sol:getAVAXAssigned(msg.sender)
should decrease byavaxLiquidStakerAmt
- Test coverage
Staking.sol:getAVAXStake(msg.sender)
should decrease byavaxNodeOpAmt
- Test coverage
Staking.sol:getMinipoolCount(msg.sender)
should decrease by 1- Test coverage
- the native tokens balance of the caller should increase by the amount of funds previously deposited.
- Test coverage
- After the call, the current state of the minipool is
Canceled
- Test coverage
Negative behavior:
- Shouldn’t leave previously set fields to their value (eg. the
avaxLiquidStakerAmt
)- Negative test?
- Shouldn’t allow unauthorized access(
msg.sender
has to be theowner
)- Negative test?
- should revert if the current state of mini pool isn’t
Prelaunch
- Negative test?
- should revert if called non-owner of minipool
- Negative test?
- should revert if minipool for nodeID doesn’t exist
- Negative test?
Preconditions:
- the minipool should be created over the
createMinipool
function - the current state of the minipool should be
Prelaunch
- Assumes that the
nodeId
has been created beforehand and that it’s in theprelaunch
stage - Assumes 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.
- What is controllable? the
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
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.- Test coverage
- Ensure that current state allows
cancellation
.- Test coverage
- Ensure that
avaxNodeOpAmt
is decreased.- Test coverage
- Ensure that
avaxLiquidStakerAmt
is decreased- Test coverage
Negative behavior:
- Shouldn’t allow
cancellation
if the current state !=prelaunch
- Negative test?
- Shouldn’t allow cancellation on behalf of
msg.sender != owner
- Negative test?
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)
- What is controllable? the
- 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.
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
- Test coverage
- the native tokens balance of minipool owner should increase by
totalAvaxAmt
value (deposited amount + reward)- Test coverage
Negative behavior:
- Shouldn’t be callable by any
msg.sender
or on anynodeId
- Negative test?
- should revert if the owner calls it a second time after the successful first execution
- Negative test?
- should revert if the current state of minipool isn’t
Withdrawable
orError
- Negative test?
- should revert if called non-owner of minipool
- Negative test? There isn’t test, but there is a check
onlyOwner
inside the function
- Negative test? There isn’t test, but there is a check
- should revert if minipool for nodeID doesn’t exist
- Negative test? There isn’t test, but there is a check
requireValidMinipool
inside the function
- Negative test? There isn’t test, but there is a check
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
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.- Test coverage
- Should ensure the status of the
minipool
is such that it can be launched- Test coverage
- Should decrease the
avax
associated to the pool (something with.avaxLiquidStakerAmt
)- Test coverage
Negative behavior:
- transaction should be rejected if current status != Prelaunch
- Negative test?
- transaction should be rejected if msg.sender isn’t approved address
- Negative test?
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)
- Checks :
- Impact : no impact
- Control : full control
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
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?- Test coverage
- Should transition a
nodeID
into “staking” period.- Test coverage
Negative behavior:
- Anyone other than
rialto
shouldn’t be able to call this.- Negative test?
- transaction should be rejected if current status != Launched
- Negative test?
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)
- Checks :
- Impact : n/a
- Control : partly controllable
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.
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.
- Test coverage
- End time should be in the future (
starttime
and not in past compared toblock.timestamp
?)- Test coverage
- Should only be callable when the
endtime
is reached.- Test coverage
Negative behavior:
- Shouldn’t be callable twice or in any other circumstance other than the transition to
withdrawable
- Negative test?
- transaction should be rejected if msg.value is not enough
- Negative test?
- transaction should be rejected if msg.sender isn’t approved address
- Negative test?
- transaction should be rejected if current status != Staking
- Negative test?
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 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? revert if
stakingTotalAssets
value is less thanavaxLiquidStakerAmt
- What is controllable?
vault.depositAVAX()
- What is controllable?
avaxNodeOpRewardAmt
- partly 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? revert if
previewDeposit
returns 0.
- What is controllable?
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
- Test coverage
Negative behavior:
- transaction should be rejected if current status !=
Staking
orLaunched
- Negative test?
Preconditions:
- current status should be
Launched
orStaking
Inputs:
msg.value
:- Control : -
- Checks :
msg.value
should be equalavaxNodeOpAmt + avaxLiquidStakerAmt
- the withdrawn funds - Impact : 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
- Control : controlled
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.
- Control : controlled
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 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? revert if
previewDeposit
returns 0.
- What is controllable?
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
- Test coverage
- Should register the
addr
as a new multisig, only if it doesn’t exist already.- Test coverage
Negative behavior:
- Shouldn’t allow anyone else other than the guardian to call it
- Negative test?
- Shouldn’t overwrite already existing multisig
- Negative test?
- Shouldn’t also enable the multisig
- Negative test?
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
Intended behavior:
- Should enable a registered multisig.
Branches and code coverage:
Intended branches:
- The “enabled” of the
index
should be set totrue
.- Test coverage
Negative behavior:
- Shouldn’t update the index of another multisig.
- Negative test?
- Shouldn’t be callable by anyone.
- Negative test? Not directly, but the
registerMultisig
which has the same modifier is tested whenmsg.sender != guardian
- Negative test? Not directly, but the
- Shouldn’t enable a multisig that doesn’t exist.
- Negative test? Not tested, there is a check in the code that prevents this from happening.
Preconditions:
- Assumes that the
multisig
has been created beforehand.
Inputs:
Function call analysis
Intended behavior:
- Should disable a registered multisig.
Branches and code coverage:
Intended branches:
- The “enabled” of the
index
should be set tofalse
.- Test coverage
Negative behavior:
- Shouldn’t be callable by any
msg.sender
- Negative test?
- Shouldn’t update an non-existing
index
- Negative test? Not tested, there is a check in the code that prevents this from happening.
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
Intended behavior:
- Allow guardian to
add
orremove
defenders.
Branches and code coverage:
Lacks testing
Intended branches:
- Should update the
defenders
states properly.- Test coverage
Negative behavior:
- Should only be callable by
guardian
; covered byonlyGuardian
modifier.- Negative test?
Preconditions:
- Assumes they are called by external accounts.
Inputs:
n/a
Function call analysis
Intended behavior:
- Allows the
defender
topause
every contract that can be paused.
Branches and code coverage:
Intended branches:
- Pause
TokenGGAVAX
- Test coverage
- Pause
MinipoolManager
- Test coverage
- Pause
Staking
(MISSING!) - added as remediation- Test coverage
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
Intended behavior:
Branches and code coverage:
Intended branches:
- Unpause
TokenGGAVAX
- Test coverage
- Unpause
MinipoolManager
- Test coverage
- Unpause
Staking
- added as remediation- Test coverage
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
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.- Test coverage
Negative behavior:
- Shouldn’t be callable by anyone. Only Multisig modifier put in place.
- Negative test?
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.
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- Test coverage
Negative behavior:
- Revert if caller is not Guardian.
- Negative test?
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
- Control : controlled
Function call analysis
There aren’t external calls here.
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
- Test coverage
- Should update the
GGPPriceInAvax
- Test coverage
Negative behavior:
- Revert if caller is not Multisig
- Negative test?
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.
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.
- Test coverage; not test covered, but verified in the code.
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.- Negative test?
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
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.- Test coverage
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.
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.
- Test coverage
- Should update the
inflationIntervalElapsedSeconds
- Test coverage
- Should increase circulating supply of tokens.
- Test coverage
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
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 increase- Test coverage
- if
nop allotment
exists != transfernopAllotment
to NOP != its balance should increase- Test coverage
- if
multisig allotment
exists != transfermultisigAllotment
to MULTISIG != its balance should increase- Test coverage
- Make sure allotments add up to 100% (the percentages)
- Test coverage
Negative behavior:
- Shouldn’t be callable whenever (
rewardscycle
should be scheduled)- Negative test?
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
Intended behavior:
- Should distribute the
ggp
to the multisigs.
Branches and code coverage:
Intended branches:
- Should only be called with legitimate
ggp
tokens.- Test coverage
Negative behavior:
Lacks negative testing
- Should not distribute rewards to deactivated
multisigs
.- Test coverage
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.
- Control : value is calculated inside
vault
:- Control : address is taken from
Vault(getContractAddress(“Vault”))
- Checks : passed from previous function; same as
ggp
parameter. - Impact : n/a
- Control : address is taken from
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
- Control : address is taken from
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
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
.- Test coverage
Negative behavior:
- Shouldn’t update an unregistered
stakerIndex
.- Negative test?
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
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- Test coverage
Negative behavior:
- The function will revert if
stakerAddr
is not valid staker- Negative test?
- The function will revert if msg.sender is not
MinipoolManager
contract- Negative test?
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.
- Control :
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.
- Control :
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.
- What is controllable?
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? it can be reverted in overflow case,
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- Test coverage
Negative behavior:
- The function will revert if
stakerAddr
is not valid staker- Negative test?
- The function will revert if the
avaxStaked
for thestakerAddr
is less thanamount
- Negative test?
- The function will revert if msg.sender is not
MinipoolManager
contract- Negative test?
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
value - 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 withdrawn.
- Control :
stakerAddr
:- Control : owner of minipool. not controlled.
- Checks : the
requireValidStaker
function checks the address. If this address isn’t staker, will revert.
- Checks : the
- Impact : in case of full access it will allow any user to decrease the number of tokens deposited.
- Control : owner of minipool. not controlled.
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.
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- Test coverage
Negative behavior:
- The function will revert if
stakerAddr
is not valid staker- Negative test?
- The function will revert if msg.sender is not
MinipoolManager
contract- Negative test?
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.
- Control :
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.
- Control :
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
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- Test coverage
Negative behavior:
- The function will revert if
stakerAddr
is not valid staker- Negative test?
- The function will revert if the
avaxAssigned
for thestakerAddr
is less thanamount
- Negative test?
- The function will revert if msg.sender is not
MinipoolManager
contract- Negative test?
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
value - Impact : 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.
- Control :
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.
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?- Test coverage
- Should allow
setting
therewardStartTime
- Test coverage
Negative behavior:
- Also, assuming that
onlyRegisteredNetworkContract
calls it. Also I think they whitelist their ownStaking
contract (basically address(this))- Negative test?
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 not - Impact : 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
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.
- Test coverage
- Should retrieve/increase/decrease the amount of
GGPrewards
a staker has earned and not claimed yet.- Test coverage
Negative behavior:
- Should revert if anyone other than the
ClaimNodeOp
contract calls them.- Negative test?
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
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- Test coverage
Negative behavior:
- The function will revert if the
.minipoolCount
is zero- Negative test?
- The function will revert if msg.sender is not
MinipoolManager
contract- Negative test?
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)
- Checks :
- 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
- Control : -
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.
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- Test coverage
Negative behavior:
- The function will revert if
stakerAddr
is not valid staker- Negative test?
- The function will revert if the
.minipoolCount
is zero- Negative test?
- The function will revert if msg.sender is not
MinipoolManager
contract- Negative test?
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.
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
- Test coverage
Negative behavior:
- The function will revert if
stakerAddr
is not valid staker- Negative test?
- The function will revert if
msg.sender
is notRegisteredNetworkContract
- Negative test?
Preconditions:
stakerAddr
have calledstakeGGP
and was registered as a staker.
Inputs:
time
:- Control : partly controlled: during minipool creation
block.timestamp
is passed - Checks : 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())
- Control : partly controlled: during minipool creation
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.
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
- Test coverage
Negative behavior:
- The function will revert if
stakerAddr
is not valid staker- Negative test?
- The function will revert if
msg.sender
is notClaimNodeOp
contract- Negative test?
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
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.- Test coverage
Negative behavior:
- The function will revert if
stakerAddr
is not valid staker- Negative test?
- The function will revert if the
.ggpRewards
is less thanamount
- Negative test?
- The function will revert if msg.sender is not
ClaimNodeOp
contract- Negative test?
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.
- Checks : the
- Impact : in case of full access it will allow any user to decrease the
.ggpRewards
- Control : owner of minipool. not controlled.
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
.
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- Test coverage
Negative behavior:
- The function will revert if
stakerAddr
is not valid staker- Negative test?
- The function will revert if msg.sender is not
ClaimNodeOp
contract- Negative test?
Preconditions:
stakerAddr
have calledstakeGGP
and was registered as a staker.
Inputs:
cycleNumber
:- Control : the value from the
rewardsPool.getRewardsCycleCount()
function call - Checks : there aren’t checks
- Impact : prevents rereceiving the reward in the same cycle.
- Control : the value from the
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
- What is controllable?
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- Test coverage
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
Intended behavior:
- Return collateralization ratio based on current
GGP
price.
Branches and code coverage:
Intended branches:
- Ensure that
stakerAddr
is valid; currently not checked- Test coverage
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
:
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- Test coverage
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 process - What happens if it reverts, reenters, or does other unusual control flow? no problems
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- Test coverage
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
:
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.- Test coverage
- The ggp balance of the
msg.sender
should deplete by amount, whilst thecontract
should have enough todeposit
into thevault(like a middleman)
- Test coverage
- The
GGPStake
of the user should be increased by thestaked
amount.- Test coverage
Negative behavior:
- Limited negative testing
- Shouldn’t allow transferring arbitrary tokens
- Negative test?
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
- Checks : there are no 0 checks, however, they do
- Impact : n/a
- Control : full control
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
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- Test coverage
Negative behavior:
Limited negative testing
- if msg.sender doesn’t have enough ggp tokens, transaction will be reverted
- Negative test?
- if msg.sender is not trusted ClaimNodeOp contract, transaction will be reverted
- Negative test?
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)
- Checks :
- Impact : the function allows caller to increase
.ggpStaked
value for any user. but caller should send this value of ggp tokens to contract
- Control : -
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
Intended behavior:
- Allows withdrawing
GGP
tokens.
Branches and code coverage:
Intended branches:
- Should ensure that the
.ggpStaked
decreases.- Test coverage
Negative behavior:
- Should never lock-up ggp; this could happen i a scenario where the
msg.sender
is never over 150% collateralization- Negative test?
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
- Checks : checks that
- Impact : could lead to loss of funds if not depleted properly.
- Control : full controll
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)- Test coverage
StakerAddr
must be registered.- Test coverage
Negative behavior:
- Only allow
minipoolmanager
to call this.- Negative test?
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
has - Impact : n/a
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.
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.