diff --git a/client/library/library/audits/communityGaming-1.html b/client/library/library/audits/communityGaming-1.html new file mode 100644 index 00000000..5574f780 --- /dev/null +++ b/client/library/library/audits/communityGaming-1.html @@ -0,0 +1,36 @@ + + + + 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/client/library/library/audits/communityGaming-2.html b/client/library/library/audits/communityGaming-2.html new file mode 100644 index 00000000..892bdb5d --- /dev/null +++ b/client/library/library/audits/communityGaming-2.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/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-1-issues.html b/content/collections/public/communityGaming-1-issues.html new file mode 100644 index 00000000..71cee429 --- /dev/null +++ b/content/collections/public/communityGaming-1-issues.html @@ -0,0 +1,231 @@ + + 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] Caution with adjusting merkle root after users have started to claim + + The merkle root used in `Airdrop.sol` is set in the [`constructor`](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Airdrop.sol#L20-L31) as well as can be set with [`UpdateMerkleRoot()`](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Airdrop.sol#L72-L78). If the initial root set in the constructor needs adjusting after users have claimed, these users can not make make use of the adjusted values, so they could have claimed more or less than is owed to them, which could cause in imbalance in the total tokens to distribute via the airdrop. + + **Remediations to Consider** + + Ensure that the initial merkle tree is correct, and/or pause the contract from claiming until it is properly set. If the merkle root does need to be adjusted/fixed after claiming, evaluate the effect for claimed/unclaimed users, potentially requiring sending additional tokens into the contract to ensure all users can complete their claim. + + + + + + Informational + fixed + 3fb1d9c8b38448af410f4d4a6d897a8ad323f20a + + ## [I-2] `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 + high + fixed + d62f923c2d7d4816a7782f8aa9c4aae1b77308e1 + + ## [G-1] Claimed airdrops can use bitmaps + + In `Airdrop.sol`, tokens are distributed to valid holders using a merkle tree. In order to prevent claiming multiple times, a `bool` is set in a mapping of the claiming address once claimed and checked to see if already set. + + ```solidity + // check if user has claimed before + if (usersClaimed[_msgSender()]) revert UserAlreadyClaimed(); + + // save claimed user + usersClaimed[_msgSender()] = true; + ``` + Reference: [Airdrop.sol#L113-117](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Airdrop.sol#L113-L117) + + Since this requires a storage write for each claim while only a single bit of information is required to determine if a user has claimed, a [bitmap](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/BitMaps.sol) can be used instead. + + If a unique index is included in each recipients leaf when generating the merkle tree, then this index could be used as a bit index in a claimed bitmap. This means that only every 256th claimant needs to make a new storage write, while the others within that 256 bit word costs a much the cheaper storage update cost. + + **Remediations to Consider** + + Use a [bitmap](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/BitMaps.sol) to read and write claimed airdrop info for users, using a unique index added to the leaves of the merkle tree. + + + + + + Gas Optimization + low + fixed + 3fb1d9c8b38448af410f4d4a6d897a8ad323f20a + + ## [G-2] Use `immutable` for static initialization values + + In `Airdrop.sol`, `tokenAddress` and `stakingContract` are set in the constructor but cannot be modified. If these are set to be immutable it will safe gas on construction and whenever these values are read. + + + + + Gas Optimization + low + fixed + 3fb1d9c8b38448af410f4d4a6d897a8ad323f20a + + ## [G-3] 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` + + + + + + Code Quality + low + fixed + 3fb1d9c8b38448af410f4d4a6d897a8ad323f20a + + ## [Q-1] Extra inheritance of ERC20 + + [`PredictionCredits.sol`](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/PredictionCredits.sol) inherits both [`ERC20`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol) and [`ERC20Permit`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Permit.sol) however `ERC20Permit` already inherits `ERC20`, so it is unnecessary. + + **Remediations to Consider** + + Remove the inheritance of ERC20 for `PredictionCredits.sol`. + + + \ No newline at end of file diff --git a/content/collections/public/communityGaming-2-issues.html b/content/collections/public/communityGaming-2-issues.html new file mode 100644 index 00000000..62b867cd --- /dev/null +++ b/content/collections/public/communityGaming-2-issues.html @@ -0,0 +1,16 @@ + + Code Quality + low + fixed + 3fb1d9c8b38448af410f4d4a6d897a8ad323f20a + + ## [Q-1] Extra inheritance of ERC20 + + [`Token.sol`](https://github.com/TournamentDAO/cg-smart-contracts/blob/545468c5c309ddb3e70253dd8ff444c4cddca3f8/contracts/cgx_token/Token.sol) and [`PredictionCredits.sol`] inherits both [`ERC20`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol) and [`ERC20Permit`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Permit.sol) however `ERC20Permit` already inherits `ERC20`, so it is unnecessary. + + **Remediations to Consider** + + Remove the inheritance of ERC20 for `Token.sol`. + + + \ No newline at end of file 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 diff --git a/content/collections/public/kwenta-19-issues.html b/content/collections/public/kwenta-19-issues.html index b527b8f9..c34f474d 100644 --- a/content/collections/public/kwenta-19-issues.html +++ b/content/collections/public/kwenta-19-issues.html @@ -1,152 +1,152 @@ - Accounting - critical - high - fixed - b5ec5c30ed5de1eba63409989179e270399aa2e8 - - ## [C-1] Can drain all credited SUSD balance in engine - - Credit is a balance of `SUSD` that can be used to cover conditional order execution fees. Credit can be added to an accountId by calling either [`creditAccount()`](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L551-L563) or the new [`creditAccountZap()`](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L565-L599) function. These end of transferring `SUSD` to the engine and credit the account id for that amount. If multiple accounts have credit, the engine’s `SUSD` balance should be the sum of the total credit (unless tokens are directly sent in). - - This credit is used when executing conditional orders in order to pay the executor, by debiting the fee from their set credit via [`_debit()`](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L650-L660): - - ```solidity - /// @dev impose a fee for executing the conditional order - /// @dev the fee is denoted in $sUSD and is - /// paid to the caller (conditional order executor) - /// @dev the fee does not exceed the max fee set by - /// the conditional order and - /// this is enforced by the `canExecute` function - if (_fee > 0) _debit(msg.sender, _co.orderDetails.accountId, _fee); - ``` - Reference: [Engine.sol#L690-L696](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L690-L696) - - If there was a way for `credit` to be increased by more than what is deposited into the engine contract, then via this fee mechanism the `SUSD` can be drained from the contract. - - The new `creditAccountZap()` function allows for this to happen, as the amount credited isn’t set to the amount transferred in, but rather sets it to the entire balance of `SUSD` in the contract: - - ```solidity - /// @inheritdoc IEngine - function creditAccountZap( - uint128 _accountId, - uint256 _amount, - IERC20 _collateral, - uint128 _marketId, - uint256 _tolerableWrapAmount, - uint256 _tolerableSwapAmount - ) external payable override { - Zap.ZapData memory zapData = Zap.ZapData({ - spotMarket: SPOT_MARKET_PROXY, - collateral: _collateral, - marketId: _marketId, - amount: _amount, - tolerance: Zap.Tolerance({ - tolerableWrapAmount: _tolerableWrapAmount, - tolerableSwapAmount: _tolerableSwapAmount - }), - direction: Zap.Direction.In, - receiver: address(this), - referrer: address(0) - }); - + Accounting + critical + high + fixed + b5ec5c30ed5de1eba63409989179e270399aa2e8 + + ## [C-1] Can drain all credited SUSD balance in engine + + Credit is a balance of `SUSD` that can be used to cover conditional order execution fees. Credit can be added to an accountId by calling either [`creditAccount()`](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L551-L563) or the new [`creditAccountZap()`](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L565-L599) function. These end of transferring `SUSD` to the engine and credit the account id for that amount. If multiple accounts have credit, the engine’s `SUSD` balance should be the sum of the total credit (unless tokens are directly sent in). + + This credit is used when executing conditional orders in order to pay the executor, by debiting the fee from their set credit via [`_debit()`](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L650-L660): + + ```solidity + /// @dev impose a fee for executing the conditional order + /// @dev the fee is denoted in $sUSD and is + /// paid to the caller (conditional order executor) + /// @dev the fee does not exceed the max fee set by + /// the conditional order and + /// this is enforced by the `canExecute` function + if (_fee > 0) _debit(msg.sender, _co.orderDetails.accountId, _fee); + ``` + Reference: [Engine.sol#L690-L696](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L690-L696) + + If there was a way for `credit` to be increased by more than what is deposited into the engine contract, then via this fee mechanism the `SUSD` can be drained from the contract. + + The new `creditAccountZap()` function allows for this to happen, as the amount credited isn’t set to the amount transferred in, but rather sets it to the entire balance of `SUSD` in the contract: + + ```solidity + /// @inheritdoc IEngine + function creditAccountZap( + uint128 _accountId, + uint256 _amount, + IERC20 _collateral, + uint128 _marketId, + uint256 _tolerableWrapAmount, + uint256 _tolerableSwapAmount + ) external payable override { + Zap.ZapData memory zapData = Zap.ZapData({ + spotMarket: SPOT_MARKET_PROXY, + collateral: _collateral, + marketId: _marketId, + amount: _amount, + tolerance: Zap.Tolerance({ + tolerableWrapAmount: _tolerableWrapAmount, + tolerableSwapAmount: _tolerableSwapAmount + }), + direction: Zap.Direction.In, + receiver: address(this), + referrer: address(0) + }); + + _collateral.transferFrom(msg.sender, address(this), _amount); + _collateral.approve(address(zap), _amount); + + // zap $Collateral -> $sUSD + zap.zap(zapData); + + //@audit: the susdAmount is set as the balance contained in the engine + // and set as the credit for the account, now sum of credit > balance + uint256 susdAmount = SUSD.balanceOf(address(this)); + + credit[_accountId] += susdAmount; + + emit Credited(_accountId, susdAmount); + } + ``` + Reference: [Engine.sol#L565-L599](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L565-L599) + + This allows anyone that calls `creditAccountZap()` to receive the entire SUSD balance as credit, then set this credit as a fee for a conditional order execution. This would then drain all USDC from the engine, losing users assets and disrupting conditional order execution. + + Additionally, `credit` can be stolen in the call to [`modifyCollateralZap()`](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L343-L393) due to a similar issue: + + ```solidity + if (_direction == Zap.Direction.In) { _collateral.transferFrom(msg.sender, address(this), _amount); _collateral.approve(address(zap), _amount); - + // zap $Collateral -> $sUSD zap.zap(zapData); - - //@audit: the susdAmount is set as the balance contained in the engine - // and set as the credit for the account, now sum of credit > balance + + //@audit: the susdAmount is set as the balance contained in the engine + // and is then added as collateral for the account id, which can then be stolen + // when collateral is removed uint256 susdAmount = SUSD.balanceOf(address(this)); - - credit[_accountId] += susdAmount; - - emit Credited(_accountId, susdAmount); + + SUSD.approve(address(PERPS_MARKET_PROXY), susdAmount); + + PERPS_MARKET_PROXY.modifyCollateral( + _accountId, USD_SYNTH_ID, susdAmount.toInt256() + ); } - ``` - Reference: [Engine.sol#L565-L599](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L565-L599) - - This allows anyone that calls `creditAccountZap()` to receive the entire SUSD balance as credit, then set this credit as a fee for a conditional order execution. This would then drain all USDC from the engine, losing users assets and disrupting conditional order execution. - - Additionally, `credit` can be stolen in the call to [`modifyCollateralZap()`](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L343-L393) due to a similar issue: - - ```solidity - if (_direction == Zap.Direction.In) { - _collateral.transferFrom(msg.sender, address(this), _amount); - _collateral.approve(address(zap), _amount); - - // zap $Collateral -> $sUSD - zap.zap(zapData); - - //@audit: the susdAmount is set as the balance contained in the engine - // and is then added as collateral for the account id, which can then be stolen - // when collateral is removed - uint256 susdAmount = SUSD.balanceOf(address(this)); - - SUSD.approve(address(PERPS_MARKET_PROXY), susdAmount); - - PERPS_MARKET_PROXY.modifyCollateral( - _accountId, USD_SYNTH_ID, susdAmount.toInt256() - ); - } - ``` - Reference: [Engine.sol#L367-L380](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L367-L380) - - Here all SUSD held is added to the account’s collateral, which is then able to be withdrawn and stolen when collateral is removed. - - **Remediations to Consider** - - Either: - - - Have `zap.zap()` return the amount received and use that as credit. - - Get the balance difference before and after the call to `zap.zap()`. - - This should also be resolved [here](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L421-L424). - - - - - - Authorization - critical - high - fixed - 545e98f72c52364d2613f9ad3987b253b287bf22 - - ## [C-2] Anyone can drain a users collateral for any Perp position - - Both [`unwindCollateral()`](https://github.com/Kwenta/smart-margin-v3/blob/ecf10bdb1c0bc198cbdf68e6b0cf4285c42539e3/src/Engine.sol#L461-L488) and [`unwindCollateralETH()`](https://github.com/Kwenta/smart-margin-v3/blob/ecf10bdb1c0bc198cbdf68e6b0cf4285c42539e3/src/Engine.sol#L490-L524) unwind a users Synthetix Perp position by using [`Zap`](https://github.com/Kwenta/smart-margin-v3/blob/95737447b67016af94505ac1d2bbe75e452d56c9/src/utils/zap/Zap.sol)'s [`unwind()`](https://github.com/Kwenta/smart-margin-v3/blob/95737447b67016af94505ac1d2bbe75e452d56c9/src/utils/zap/Zap.sol#L303-L352) function. `unwind()` uses an Aave flash loan to receive enough USDC which is used to cover the Perp account’s debt, then the collateral is withdrawn, and sold via swap for USDC to cover the loan, with the remaining collateral being sent to the set `_receiver`. - - However, there are no permission requirements to initiate this process via `unwindCollateralETH()` or `unwindCollateral()` , and the receiver of this accounts collateral is set to `msg.sender`, or ETH is sent to `msg.sender` in the case of `unwindCollateralETH()`. This allows for anyone to unwind and take the collateral of any Perp account that has granted the Engine contract Admin permission, which any account using the Engine would have given. - - An attacker an easily call unwindCollateral() on any account and steal all the accounts collateral minus debt. - - **Remediations to Consider** - - Add a check to ensure the caller is the account owner, or sufficiently permissioned user, for both `unwindCollateral()` and `unwindCollateralETH()` similar to what is done when [withdrawing collateral from an account](https://github.com/Kwenta/smart-margin-v3/blob/ecf10bdb1c0bc198cbdf68e6b0cf4285c42539e3/src/Engine.sol#L441). - This ensures that only authorized users can unwind a position and up up with an accounts collateral. - - - - - - Validation - high - low - fixed - 95737447b67016af94505ac1d2bbe75e452d56c9 - - ## [M-1] Collateral isn’t validated to be related to synth in Zap - - `Zap.sol` allows to either sell a token for `SUSD` or by a token with `SUSD` via `SynthetixV3`’s associated spot markets. It does this by either wrapping or unwrapping the token with its synthetic version that the Synthetix spot market interacts with. - - In the case of [_zapOut()](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/utils/zap/Zap.sol#L65-L89), `SUSD` is used to buy the synthetic version of the desired token out, which is defined by the `marketId` value, which is then unwrapped, exchanging the purchased synth for its associated collateral token, which then is assumed that the provided `collateral` param is the token received. However, there is no check to ensure that the `collateral` is associated with the synth `marketId` which could lead to issues when they are not related. - - In the case where a user intends to receive a valuable token like `WETH`, but accidentally sets the `collateral` value to something like `SUSD`, then `WETH` will be sent into the contract but it will attempt to send an amount of `SUSD` out, which is valued much less. Typically this transaction would fail since `Zap` wouldn’t have a balance of `SUSD`, but if someone were to front-run this transaction, sending enough `SUSD` into `Zap`, then the transaction would execute. Then the `WETH` now in the `Zap` contract could be taken in a similar method by using a less valuable syth’s market id, and setting the `collateral` to be `WETH`, curating the parameters so the amount swapped equals the `WETH` held. - - **Remediations to Consider** - - Validate that the provided `collateral` is used by the spot market id by calling [getWrapper()](https://github.com/Synthetixio/synthetix-v3/blob/main/markets/spot-market/contracts/modules/WrapperModule.sol#L48-L54) to ensure the tokens received are as expected. - - + ``` + Reference: [Engine.sol#L367-L380](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L367-L380) + + Here all SUSD held is added to the account’s collateral, which is then able to be withdrawn and stolen when collateral is removed. + + **Remediations to Consider** + + Either: + + - Have `zap.zap()` return the amount received and use that as credit. + - Get the balance difference before and after the call to `zap.zap()`. + + This should also be resolved [here](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/Engine.sol#L421-L424). + + + + + + Authorization + critical + high + fixed + 545e98f72c52364d2613f9ad3987b253b287bf22 + + ## [C-2] Anyone can drain a users collateral for any Perp position + + Both [`unwindCollateral()`](https://github.com/Kwenta/smart-margin-v3/blob/ecf10bdb1c0bc198cbdf68e6b0cf4285c42539e3/src/Engine.sol#L461-L488) and [`unwindCollateralETH()`](https://github.com/Kwenta/smart-margin-v3/blob/ecf10bdb1c0bc198cbdf68e6b0cf4285c42539e3/src/Engine.sol#L490-L524) unwind a users Synthetix Perp position by using [`Zap`](https://github.com/Kwenta/smart-margin-v3/blob/95737447b67016af94505ac1d2bbe75e452d56c9/src/utils/zap/Zap.sol)'s [`unwind()`](https://github.com/Kwenta/smart-margin-v3/blob/95737447b67016af94505ac1d2bbe75e452d56c9/src/utils/zap/Zap.sol#L303-L352) function. `unwind()` uses an Aave flash loan to receive enough USDC which is used to cover the Perp account’s debt, then the collateral is withdrawn, and sold via swap for USDC to cover the loan, with the remaining collateral being sent to the set `_receiver`. + + However, there are no permission requirements to initiate this process via `unwindCollateralETH()` or `unwindCollateral()` , and the receiver of this accounts collateral is set to `msg.sender`, or ETH is sent to `msg.sender` in the case of `unwindCollateralETH()`. This allows for anyone to unwind and take the collateral of any Perp account that has granted the Engine contract Admin permission, which any account using the Engine would have given. + + An attacker an easily call unwindCollateral() on any account and steal all the accounts collateral minus debt. + + **Remediations to Consider** + + Add a check to ensure the caller is the account owner, or sufficiently permissioned user, for both `unwindCollateral()` and `unwindCollateralETH()` similar to what is done when [withdrawing collateral from an account](https://github.com/Kwenta/smart-margin-v3/blob/ecf10bdb1c0bc198cbdf68e6b0cf4285c42539e3/src/Engine.sol#L441). + This ensures that only authorized users can unwind a position and up up with an accounts collateral. + + + + + + Validation + high + low + fixed + 95737447b67016af94505ac1d2bbe75e452d56c9 + + ## [M-1] Collateral isn’t validated to be related to synth in Zap + + `Zap.sol` allows to either sell a token for `SUSD` or by a token with `SUSD` via `SynthetixV3`’s associated spot markets. It does this by either wrapping or unwrapping the token with its synthetic version that the Synthetix spot market interacts with. + + In the case of [_zapOut()](https://github.com/Kwenta/smart-margin-v3/blob/eb3f357fd88fb3e33124772deb5306083a71c77c/src/utils/zap/Zap.sol#L65-L89), `SUSD` is used to buy the synthetic version of the desired token out, which is defined by the `marketId` value, which is then unwrapped, exchanging the purchased synth for its associated collateral token, which then is assumed that the provided `collateral` param is the token received. However, there is no check to ensure that the `collateral` is associated with the synth `marketId` which could lead to issues when they are not related. + + In the case where a user intends to receive a valuable token like `WETH`, but accidentally sets the `collateral` value to something like `SUSD`, then `WETH` will be sent into the contract but it will attempt to send an amount of `SUSD` out, which is valued much less. Typically this transaction would fail since `Zap` wouldn’t have a balance of `SUSD`, but if someone were to front-run this transaction, sending enough `SUSD` into `Zap`, then the transaction would execute. Then the `WETH` now in the `Zap` contract could be taken in a similar method by using a less valuable syth’s market id, and setting the `collateral` to be `WETH`, curating the parameters so the amount swapped equals the `WETH` held. + + **Remediations to Consider** + + Validate that the provided `collateral` is used by the spot market id by calling [getWrapper()](https://github.com/Synthetixio/synthetix-v3/blob/main/markets/spot-market/contracts/modules/WrapperModule.sol#L48-L54) to ensure the tokens received are as expected. + + \ No newline at end of file