Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

startRewardsCycle() would fail if number of enabled Multisigs were zero because of the division by zero error, the issue can wrong reward distribution #839

Closed
code423n4 opened this issue Jan 3, 2023 · 2 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-143 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

Function startRewardsCycle() is Public function that will run a GGP rewards cycle if possible but if the number of the enabled Multisigs were 0 then calling this function would fail because of division by zero error. there may be some moments when number of enabled Multisigs are 0x0 for any reason and this issue would cause ggp rewarding feature of protocol to be broken in those situations so some basic functionalities would not work and users can't access their rewards,.

Proof of Concept

This is function distributeMultisigAllotment() code which is called by startRewardsCycle():

	function distributeMultisigAllotment(
		uint256 allotment,
		Vault vault,
		TokenGGP ggp
	) internal {
		MultisigManager mm = MultisigManager(getContractAddress("MultisigManager"));

		uint256 enabledCount;
		uint256 count = mm.getCount();
		address[] memory enabledMultisigs = new address[](count);

		// there should never be more than a few multisigs, so a loop should be fine here
		for (uint256 i = 0; i < count; i++) {
			(address addr, bool enabled) = mm.getMultisig(i);
			if (enabled) {
				enabledMultisigs[enabledCount] = addr;
				enabledCount++;
			}
		}

		// Dirty hack to cut unused elements off end of return value (from RP)
		// solhint-disable-next-line no-inline-assembly
		assembly {
			mstore(enabledMultisigs, enabledCount)
		}

		uint256 tokensPerMultisig = allotment / enabledCount;
		for (uint256 i = 0; i < enabledMultisigs.length; i++) {
			vault.withdrawToken(enabledMultisigs[i], ggp, tokensPerMultisig);
		}
	}

As you can see code tries to calculates each Multisig rewards in the line tokensPerMultisig = allotment / enabledCount but if enabled Multisig counts were zero then the code would revert and function startRewardsCycle() won't be callable and this would cause node runners and DAO to not receive their ggp rewards in time. blocking the rewards long enough (more than that cycle length) can cause total reward loss of that cycle and there is no enforce in the protocol to make sure there is always one Multisig enabled and for any reason there could be some times that all Multisigs are disabled.

Tools Used

VIM

Recommended Mitigation Steps

check for zero amount and if there is zero Multisig then handle Multisig rewards in another way(set rewards as zero).

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jan 3, 2023
code423n4 added a commit that referenced this issue Jan 3, 2023
C4-Staff added a commit that referenced this issue Jan 6, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Jan 8, 2023

GalloDaSballo marked the issue as duplicate of #143

@c4-judge
Copy link
Contributor

c4-judge commented Feb 8, 2023

GalloDaSballo marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-143 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants