diff --git a/client/library/library/audits/communityGaming-4.html b/client/library/library/audits/communityGaming-4.html new file mode 100644 index 00000000..d071f945 --- /dev/null +++ b/client/library/library/audits/communityGaming-4.html @@ -0,0 +1,34 @@ + + + + The security audit was performed by the Macro security team from October 9th to October 10th, 2024. + + + + + + + + + + + +

Specifically, we audited the following contracts within this repository:

+ + + + + diff --git a/content/collections/public/communityGaming-4-issues.html b/content/collections/public/communityGaming-4-issues.html new file mode 100644 index 00000000..9646595c --- /dev/null +++ b/content/collections/public/communityGaming-4-issues.html @@ -0,0 +1,158 @@ + + Validation + low + medium + fixed + 3fb1d9c8b38448af410f4d4a6d897a8ad323f20a + + ## [L-1] Ineffective `_lockDuration` validity check + + In `Staking.sol`’s [`_stake()`](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Staking.sol#L138-L182) function, the _lockDuration is checked to ensure it is valid. It does so by checking against each valid duration: + + ```solidity + if ( + !(_lockDuration == MINUTE || + _lockDuration == FIVE_MINUTES || + _lockDuration == HOUR || + _lockDuration == MONTH || + _lockDuration == SIX_MONTHS || + _lockDuration == EIGHTEEN_MONTHS) + ) revert InvalidLockDuration(); + ``` + Reference: [Staking.sol#L162-169](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Staking.sol#L162-L169) + + Some of these values along with their associated multipliers are present only for testing purposes: + + ```solidity + // ## for testing purposes only (will be removed on mainnet) ## + + uint256 public constant MINUTE = 60; + uint256 public constant FIVE_MINUTES = 300; + uint256 public constant HOUR = 3600; + ``` + Reference: [Staking.sol#L35-38](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Staking.sol#L35-L38) + + ```solidity + // ## for testing purposes only (will be removed on mainnet) ## + + rewardMultipliers[MINUTE] = 1000; + rewardMultipliers[FIVE_MINUTES] = 2000; + rewardMultipliers[HOUR] = 5000; + ``` + Reference: [Staking.sol#L69-72](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Staking.sol#L69-L72) + + Rather than use these lock duration constants and checking each, checking if the `rewardMultiplier` for that duration is non-zero is also equivalent and more flexible. + + **Remediations to Consider** + + Check if the `rewardMultiplier` for the `_lockDuration` is non-zero, rather than checking against each constant. Also consider in [`_initializeMultipliers()`](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Staking.sol#L64-L78) and [`updateRewardMultipliers()`](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Staking.sol#L80-L110) take an array of duration and multiplier and set using those. This would allow setting test values without directly adding to the contract, as well as adding additional durations post deployment if needed. + + + + + Composability + low + low + fixed + 3fb1d9c8b38448af410f4d4a6d897a8ad323f20a + + ## [L-2] `stakeForUser()` is limited to contracts that transfer tokens before calling + + Currently [stakeForUser()](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Staking.sol#L200-L221) assumes tokens have been transferred in, and have set a non-zero value for `_stakePercentage`: + + ```solidity + if (_stakingPercentage == 0) { + //If _stakingPercentage>0 the transfer already hapenned in the airdrop contract + if (!token.transferFrom(_user, address(this), _amount)) + revert TransferFailed(); + } + ``` + Reference: [Staking.sol#L171-175](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Staking.sol#L171-L175) + + For `Airdrop.sol` this is the case, but other potential whitelisted addresses may not function like this, and prevents eoa’s from being whitelisted without requiring trust. If approvals were used and transferFrom is always used rather than sending before it allows interaction and whitelist options to be more flexible. + + **Remediations to Consider** + + In Airdrop.sol’s constructor approve the Staking contract for uint.max tokens, and remove the prior transfer. Then always transfer using `transferFrom()`, ie when `_stakingPercentage` is non-zero. + + + + + Informational + fixed + 3fb1d9c8b38448af410f4d4a6d897a8ad323f20a + + ## [I-1] `stakeForUser()` can be used to grief claims if whitelist allows + + When staking on the `Staking.sol` contract, staking data gets added to an array of a users pending stakes: + + ```solidity + uint256 unlockTime = block.timestamp + _lockDuration; + uint256 multiplier = rewardMultipliers[_lockDuration]; + userStakes[_user].push(StakedTokens(_amount, unlockTime, multiplier)); + ``` + Reference: [Staking.sol#L177-179](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Staking.sol#L177-L179) + + These pending stakes can be claimed via `withdrawTokens()` and loops through all pending stakes for a user and then claims the sum total of unlocked tokens: + + ```solidity + //go through all stakes and withdraw from the user stakes that are unlockable + for (uint256 i = 0; i < stakes.length; ) { + if (stakes[i].unlockTime <= block.timestamp) { + uint256 amountToWithdraw = stakes[i].amount < remainingAmount + ? stakes[i].amount + : remainingAmount; + totalWithdrawable += stakes[i].amount; + + stakes[i].amount -= amountToWithdraw; + remainingAmount -= amountToWithdraw; + + //if the entire stake batch amount was spent on this withdrawal, remove the stake + if (stakes[i].amount == 0) { + stakes[i] = stakes[stakes.length - 1]; + stakes.pop(); + continue; + } + + //if the entire remaining amount was withdrawn, break the loop + if (remainingAmount == 0) { + break; + } + } + i++; + } + + if (totalWithdrawable < _amount) + revert InsufficientWithdrawableTokens(); + if (!token.transfer(_msgSender(), _amount)) revert TransferFailed(); + ``` + Reference: [Staking.sol#L246-274](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Staking.sol#L246-L274) + + However, if the stakes array gets sufficiently large, a user may not be able to execute this function because the cost may exceed the block gas limit or cost a substantial amount of gas to execute. It is safe to say a user would not normally stake enough to cause this issue, however if able to stake for another address someone could grief other users. + + Currently the only way to stake for another address is via the function [`stakeForUser()`](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Staking.sol#L200-L221) . However it is only callable by whitelisted addresses, an example is the `Airdrop` contract when claiming and staking via [`claimAndStake()`](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Airdrop.sol#L130-L136), which should be limited to a single stake per address. If there is other addresses whitelisted, and allows for unrestricted staking for others then this could become a issue. It is important to note that a griefer would spend more in gas to achieve this. Smart use of `_amount` can help users mitigate, however in the case of smart contract claims attempting to claim their owed amount they could have its execution prevented. + + **Remediations to Consider** + + When adding addresses to the whitelist ensure it does not allow unrestricted staking for other users. + + + + + + Gas Optimization + low + fixed + 3fb1d9c8b38448af410f4d4a6d897a8ad323f20a + + ## [G-1] Unnecessary reentrancy guard + + In `Staking.sol` multiple functions use the `nonReentrant` reentrancy guard including `stake()` `stakeForUser()` and `withdrawTokens()`. However the only external mutative call in these is to transfer the set token via either `transferFrom()` or `transfer()`. Since the token is known and does not trigger callback functions that would allow for reentrancy, it is safe to remove these guards to save users some gas. + + **Remediations to Consider** + + Remove the reentrancy guards from `Staking.sol` + + + + \ No newline at end of file