Arcade.xyz - Sathish9098's results

LOW FINDINGS

[L-1] Use safeInceaseAllowance()/safeDecreaseAllowance() instead of approve() or deprecated safeApprove() Functions

Impact

The safeIncreaseAllowance() and safeDecreaseAllowance() functions also prevent front-running attacks by atomically updating the allowance and emitting an Approval event. This means that the allowance cannot be changed by another transaction between the time the allowance is updated and the Approval event is emitted.

The safeApprove() function was deprecated because it was not gas-efficient and it did not prevent front-running attacks. You should use safeIncreaseAllowance() or safeDecreaseAllowance() instead of safeApprove()

POC

FILE: Breadcrumbs2023-07-arcade/contracts/ArcadeTreasury.sol

200:  _approve(token, spender, amount, spendThresholds[token].small);

219: _approve(token, spender, amount, spendThresholds[token].small);

238:  _approve(token, spender, amount, spendThresholds[token].medium);

257: _approve(token, spender, amount, spendThresholds[token].large);

391:   IERC20(token).approve(spender, amount);

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ArcadeTreasury.sol#L391

Recommended Mitigation

Use safeIncreaseAllowance() or safeDecreaseAllowance() instead of approve() or safeApprove()

[L-2] Don’t use payable.transfer()

Impact

The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient is either an EOA account, or is a contract that has a payable callback. For the contract case, the transfer() call only provides 2300 gas for the contract to complete its operations. This means the following cases can cause the transfer to fail:

• The contract does not have a payable callback
• The contract’s payable callback spends more than 2300 gas (which is only enough to emit something)

POC

FILE: 2023-07-arcade/contracts/ArcadeTreasury.sol

367: payable(destination).transfer(amount);

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ArcadeTreasury.sol#L367

FILE: 2023-07-arcade/contracts/nft/ReputationBadge.sol

171: payable(recipient).transfer(balance);

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/nft/ReputationBadge.sol#L171

[L-3] Hardcoded cooldown period may cause problem in future

Impact

Hardcoding a cooldown period in the contract may lead to potential problems in the future. The hardcoded value of SET_ALLOWANCE_COOL_DOWN is currently set to 7 days, which means the cooldown period for updating the GSC allowance is fixed and cannot be changed without redeploying the contract.

POC

FILE: 2023-07-arcade/contracts/ArcadeTreasury.sol

56: uint48 public constant SET_ALLOWANCE_COOL_DOWN = 7 days;

Recommended Mitigation

Cooldown period can be set by the contract owner or by a governance mechanism. This allows the cooldown period to be adjusted as needed, without having to modify the contract code.

[L-4] Lack of nonReentrant modifier in critical withdraw(),reclaim() functions

Impact

The withdraw function should have a reentrancy modifier to protect against reentrancy attacks. Although the function already checks for the available balance (unassigned.data) and transfers the tokens using token.safeTransfer, it is still vulnerable to reentrancy attacks if the token.safeTransfer function itself or any other function it calls contains external contract calls

POC

FILE: 2023-07-arcade/contracts/ARCDVestingVault.sol

function withdraw(uint256 amount, address recipient) external override onlyManager {
        Storage.Uint256 storage unassigned = _unassigned();
        if (unassigned.data < amount) revert AVV_InsufficientBalance(unassigned.data);
        // update unassigned value
        unassigned.data -= amount;

        token.safeTransfer(recipient, amount);
    }

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ARCDVestingVault.sol#L211-L218

FILE: Breadcrumbs2023-07-arcade/contracts/token/ArcadeAirdrop.sol

function reclaim(address destination) external onlyOwner {
        if (block.timestamp <= expiration) revert AA_ClaimingNotExpired();
        if (destination == address(0)) revert AA_ZeroAddress("destination");

        uint256 unclaimed = token.balanceOf(address(this));
        token.safeTransfer(destination, unclaimed);
    }

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/token/ArcadeAirdrop.sol#L62

Recommended Mitigation

Use nonReentrant to avoid reentrancy attacks. And this gives extra layer of safety .

[L-5] The addGrantAndDelegate function not checked the new delegatee already has an active grant

Impact

The function addGrantAndDelegate allows the manager to create a new grant without checking if the grant recipient already has an active grant. If a user already has an active grant, this function will overwrite the existing grant, which might lead to unintended behavior. It would be better to check if the recipient already has a grant and handle this situation accordingly.

POC

FILE: 2023-07-arcade/contracts/ARCDVestingVault.sol

function addGrantAndDelegate(
        address who,
        uint128 amount,
        uint128 cliffAmount,
        uint128 startTime,
        uint128 expiration,
        uint128 cliff,
        address delegatee
    ) external onlyManager {
        // input validation
        if (who == address(0)) revert AVV_ZeroAddress("who");
        if (amount == 0) revert AVV_InvalidAmount();

        // if no custom start time is needed we use this block.
        if (startTime == 0) {
            startTime = uint128(block.number);
        }
        // grant schedule check
        if (cliff >= expiration || cliff < startTime) revert AVV_InvalidSchedule();

        // cliff check
        if (cliffAmount >= amount) revert AVV_InvalidCliffAmount();

        Storage.Uint256 storage unassigned = _unassigned();
        if (unassigned.data < amount) revert AVV_InsufficientBalance(unassigned.data);

        // load the grant
        ARCDVestingVaultStorage.Grant storage grant = _grants()[who];

        // if this address already has a grant, a different address must be provided
        // topping up or editing active grants is not supported.
        if (grant.allocation != 0) revert AVV_HasGrant();

        // load the delegate. Defaults to the grant owner
        delegatee = delegatee == address(0) ? who : delegatee;

        // calculate the voting power. Assumes all voting power is initially locked.
        uint128 newVotingPower = amount;

        // set the new grant
        grant.allocation = amount;
        grant.cliffAmount = cliffAmount;
        grant.withdrawn = 0;
        grant.created = startTime;
        grant.expiration = expiration;
        grant.cliff = cliff;
        grant.latestVotingPower = newVotingPower;
        grant.delegatee = delegatee;

        // update the amount of unassigned tokens
        unassigned.data -= amount;

        // update the delegatee's voting power
        History.HistoricalBalances memory votingPower = _votingPower();
        uint256 delegateeVotes = votingPower.loadTop(grant.delegatee);
        votingPower.push(grant.delegatee, delegateeVotes + newVotingPower);

        emit VoteChange(grant.delegatee, who, int256(uint256(newVotingPower)));
    }

Recommended Mitigation

Add a check to ensure that the recipient does not already have an active grant

 ARCDVestingVaultStorage.Grant storage existingGrant = _grants()[who];
 if (existingGrant.allocation != 0) revert AVV_HasGrant();

[L-6] No same value input control in critical setMinter() functions

The same address value should be avoided

POC

FILE: 2023-07-arcade/contracts/token/ArcadeToken.sol

 function setMinter(address _newMinter) external onlyMinter {
        if (_newMinter == address(0)) revert AT_ZeroAddress("newMinter");

        minter = _newMinter;
        emit MinterUpdated(minter);
    }

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/token/ArcadeToken.sol#L132-L137

Recommended Mitigation

Add same value input control

require(_newMinter!=minter, "Same address value ")

[L-7] Project Upgrade and Stop Scenario should be

At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an ” EMERGENCY STOP (CIRCUIT BREAKER) PATTERN “.

https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol

[L-8] Accidental token transfers can't be recovered from contract

Impact

Accidental token transfers are typically irreversible and cannot be recovered by the contract. Once tokens are transferred out of a contract, they are under the control of the recipient, and the contract has no authority to reverse or undo the transaction.

Recommended Mitigation

function recoverToken(
        address tokenAddress,
        uint256 tokenId,
        address recipient,
        uint256 amount
    ) external onlyOwnerOrAuthorized {
        if (tokenAddress == address(0)) {
            // Recover ERC20 tokens
            IERC20(tokenAddress).transfer(recipient, amount);
        } else {
            // Recover ERC1155 NFTs
            IERC1155(tokenAddress).safeTransferFrom(address(this), recipient, tokenId, amount, bytes(""));
        }
    }

[L-9] Lack of checks for critical baseURI string value

Impact

Empty base URI, which could lead to incorrect behavior when generating metadata URLs for ERC1155 NFTs.

The setBaseURI function lacks proper checks for critical operations on the baseURI string value. It's important to validate and handle inputs to prevent potential vulnerabilities

POC

FILE: 2023-07-arcade/contracts/nft/BadgeDescriptor.sol

  function setBaseURI(string memory newBaseURI) external onlyOwner {
        baseURI = newBaseURI;

        emit SetBaseURI(msg.sender, newBaseURI);
    }

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/nft/BadgeDescriptor.sol#L57-L61

Recommended Mitigation

 require(bytes(newBaseURI).length > 0, "Base URI cannot be empty");

[L-10] No time lock mechanism to delay the distribution used critical toTreasury(),toDevPartner(),toCommunityRewards(),toCommunityAirdrop(), toTeamVesting(),toPartnerVesting() Functions

Impact

If the functions is mistakenly called (e.g., due to human error or miscommunication) or maliciously triggered by an unauthorized entity, the tokens are transferred instantly to the addresses. Once the transfer is executed, it is irreversible, and there's no way to recover the tokens unless the receiver address voluntarily returns them.

POC

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/token/ArcadeTokenDistributor.sol#L72-L164

Recommended Mitigation

The contract owner might set a time locks

[L-11] Don't use ERC1155

Impact

The main limitation of ERC1155 is the lack of individual scarcity for each token. Since ERC1155 tokens can represent multiple types of assets, they do not possess the same level of uniqueness as ERC721 tokens .

POC

FILE: Breadcrumbs2023-07-arcade/contracts/NFTBoostVault.sol

5: import "@openzeppelin/contracts/token/ERC1155/IERC1155.sol";

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/NFTBoostVault.sol#L5

Recommended Mitigation

Use ERC721 instead ERC1155

[L-12] batchCalls function, if a single call fails, the entire transaction will be reverted

n the batchCalls function, if a single call fails, the entire transaction will be reverted, and the gas spent on previous successful calls will not be refunded. Consider using a different design that allows partial success and refunds gas for successful calls while reverting only the failed ones.

FILE: 2023-07-arcade/contracts/ArcadeTreasury.sol

function batchCalls(
        address[] memory targets,
        bytes[] calldata calldatas
    ) external onlyRole(ADMIN_ROLE) nonReentrant {
        if (targets.length != calldatas.length) revert T_ArrayLengthMismatch();
        // execute a package of low level calls
        for (uint256 i = 0; i < targets.length; ++i) {
            if (spendThresholds[targets[i]].small != 0) revert T_InvalidTarget(targets[i]);
            (bool success, ) = targets[i].call(calldatas[i]);
            // revert if a single call fails
            if (!success) revert T_CallFailed();
        }
    }

GAS OPTIMIZATIONS

Gas optimizations can be done based on opcodes by using the following techniques

• Reducing the SLOTs as much as possible.

  • Pack state variables efficient way
  • If possible try down casting state variables to pack with same SLOT
  • Pack structs efficient way
  • Use immutable when ever possible

• Using cheaper opcodes: Some opcodes are more expensive than others, so using the cheaper ones can save gas. For example, the SLOAD opcode is cheaper than the SSTORE opcode, so if you only need to read a value from storage, you should use SLOAD instead of SSTORE.

• Caching data: If you need to access the same data multiple times, you can cache it in memory. This will save gas because you won't have to pay to access the data from storage each time.

• Minimizing stack usage: The stack is a data structure that is used to store temporary values. Each value that is pushed onto the stack costs gas, so minimizing stack usage can save gas.

TRADITIONAL GAS FINDINGS

[G-1] State variables can be packed to use fewer storage slots

Saves 2000 GAS, 1 SLOT

The EVM works with 32 byte words. Variables less than 32 bytes can be declared next to eachother in storage and this will pack the values together into a single 32 byte storage slot (if the values combined are <= 32 bytes). If the variables packed together are retrieved together in functions we will effectively save ~2000 gas with every subsequent SLOAD for that storage slot. This is due to us incurring a Gwarmaccess (100 gas) versus a Gcoldsload (2100 gas)

ArcadeToken.sol : minter,mintingAllowedAfter can be packed together : Saves 2000 GAS, 1 SLOT

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/token/ArcadeToken.sol#L93-L97

mintingAllowedAftercan be uint96 since this always stores the epoch time. This means that a uint96 can store timestamps for up to 2^32-1 years, which is approximately 2147483648 years. The Unix epoch is the number of seconds since January 1, 1970, 00:00:00 UTC. There are approximately 2^64 seconds in a year, so a uint96 can store timestamps for approximately 2^32 years without overflow.

FILE: 2023-07-arcade/contracts/token/ArcadeToken.sol

93:  /// @notice Minter contract address responsible for minting future tokens
94:    address public minter;
95:
96:    /// @notice The timestamp after which minting may occur
- 97:    uint256 public mintingAllowedAfter;
+ 97:    uint96 public mintingAllowedAfter;

[G-2] State variables should be cached in stack variables rather than re-reading them from storage

Saves 3000 GAS, 30 SLODs

Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read.

mintingAllowedAfter should be cached avoid 1 SLOD , 100 GAS

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/token/ArcadeToken.sol#L146

FILE: Breadcrumbs2023-07-arcade/contracts/token/ArcadeToken.sol

+ uint256 mintingAllowedAfter_ = mintingAllowedAfter;
- 146: if (block.timestamp < mintingAllowedAfter) revert AT_MintingNotStarted(mintingAllowedAfter, block.timestamp);
+ 146: if (block.timestamp < mintingAllowedAfter_ ) revert AT_MintingNotStarted(mintingAllowedAfter_ , block.timestamp);
147:  if (_to == address(0)) revert AT_ZeroAddress("to");

unassigned.data, grant.allocation, grant.cliff, grant.delegatee, grant.latestVotingPower can be cached : Saves 1500 GAS, 15 SLODs

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ARCDVestingVault.sol#L114-L148

FILE: 2023-07-arcade/contracts/ARCDVestingVault.sol

Saves 4 SLODs

        Storage.Uint256 storage unassigned = _unassigned();
+      uint256 _data = unassigned.data ;
-        if (unassigned.data < amount) revert AVV_InsufficientBalance(unassigned.data);
+        if (_data  < amount) revert AVV_InsufficientBalance(_data);

        // load the grant
        ARCDVestingVaultStorage.Grant storage grant = _grants()[who];

        // if this address already has a grant, a different address must be provided
        // topping up or editing active grants is not supported.
        if (grant.allocation != 0) revert AVV_HasGrant();

        // load the delegate. Defaults to the grant owner
        delegatee = delegatee == address(0) ? who : delegatee;

        // calculate the voting power. Assumes all voting power is initially locked.
        uint128 newVotingPower = amount;

        // set the new grant
        grant.allocation = amount;
        grant.cliffAmount = cliffAmount;
        grant.withdrawn = 0;
        grant.created = startTime;
        grant.expiration = expiration;
        grant.cliff = cliff;
        grant.latestVotingPower = newVotingPower;
        grant.delegatee = delegatee;

        // update the amount of unassigned tokens
-        unassigned.data -= amount;
+        unassigned.data =_data + amount;
        // update the delegatee's voting power
        History.HistoricalBalances memory votingPower = _votingPower();
-        uint256 delegateeVotes = votingPower.loadTop(grant.delegatee);
+        uint256 delegateeVotes = votingPower.loadTop(delegatee);
-        votingPower.push(grant.delegatee, delegateeVotes + newVotingPower);
+        votingPower.push(delegatee, delegateeVotes + newVotingPower);
        emit VoteChange(grant.delegatee, who, int256(uint256(newVotingPower)));

Saves 1 SLOD

+  uint128 _allocation = grant.allocation ;
-  if (grant.allocation == 0) revert AVV_NoGrantSet();
+  if (_allocation  == 0) revert AVV_NoGrantSet();
        // get the amount of withdrawable tokens
        uint256 withdrawable = _getWithdrawableAmount(grant);
        grant.withdrawn += uint128(withdrawable);
        token.safeTransfer(who, withdrawable);

        // transfer the remaining tokens to the vesting manager
-        uint256 remaining = grant.allocation - grant.withdrawn;
+        uint256 remaining = _allocation  - grant.withdrawn;

Saves 2 SLODs

212:   Storage.Uint256 storage unassigned = _unassigned();
+   uint256 data_= unassigned.data;
- 213: if (unassigned.data < amount) revert AVV_InsufficientBalance(unassigned.data);
+ 213: if (data_ < amount) revert AVV_InsufficientBalance(data_);
214:        // update unassigned value
- 215:        unassigned.data -= amount;
+ 215:        unassigned.data =data_- amount;

Saves 1 SLOD

+  uint128 cliff_ =  grant.cliff ;
- 234:   if (grant.cliff > block.number) revert AVV_CliffNotReached(grant.cliff);
+ 234:   if (cliff_ > block.number) revert AVV_CliffNotReached(cliff_);

Saves 5 SLODs

  ARCDVestingVaultStorage.Grant storage grant = _grants()[msg.sender];
+  address delegatee_= grant.delegatee;
-        if (to == grant.delegatee) revert AVV_AlreadyDelegated();
+        if (to == delegatee_) revert AVV_AlreadyDelegated();

        History.HistoricalBalances memory votingPower = _votingPower();
-        uint256 oldDelegateeVotes = votingPower.loadTop(grant.delegatee);
+        uint256 oldDelegateeVotes = votingPower.loadTop(delegatee_);

        // Remove old delegatee's voting power and emit event
+    uint256 latestVotingPower_= grant.latestVotingPower;
-        votingPower.push(grant.delegatee, oldDelegateeVotes - grant.latestVotingPower);
+        votingPower.push(delegatee_, oldDelegateeVotes - latestVotingPower_);
-        emit VoteChange(grant.delegatee, msg.sender, -1 * int256(grant.latestVotingPower));
+        emit VoteChange(delegatee_, msg.sender, -1 * int256(latestVotingPower_));

        // Note - It is important that this is loaded here and not before the previous state change because if
        // to == grant.delegatee and re-delegation was allowed we could be working with out of date state.
        uint256 newDelegateeVotes = votingPower.loadTop(to);

        // add voting power to the target delegatee and emit event
-         votingPower.push(to, newDelegateeVotes + grant.latestVotingPower);
+         votingPower.push(to, newDelegateeVotes + latestVotingPower_);
        // update grant delgatee info
        grant.delegatee = to;

-        emit VoteChange(to, msg.sender, int256(uint256(grant.latestVotingPower)));
+        emit VoteChange(to, msg.sender, int256(uint256(latestVotingPower_)));

Saves 1 SLOD
 
+ address delegatee_= grant.delegatee;
-  uint256 delegateeVotes = votingPower.loadTop(grant.delegatee);
+  uint256 delegateeVotes = votingPower.loadTop(delegatee_);

        uint256 newVotingPower = grant.allocation - grant.withdrawn;

        // get the change in voting power. voting power can only go down
        // since the sync is only called when tokens are claimed or grant revoked
        int256 change = int256(newVotingPower) - int256(grant.latestVotingPower);
        // we multiply by -1 to avoid underflow when casting
-        votingPower.push(grant.delegatee, delegateeVotes - uint256(change * -1));
+        votingPower.push(delegatee_, delegateeVotes - uint256(change * -1));
        grant.latestVotingPower = newVotingPower;

-        emit VoteChange(grant.delegatee, who, change);
+        emit VoteChange(delegatee_, who, change);

registration.delegatee,registration.latestVotingPower,balance.data,registration.tokenId, should be cached : Saves 1200 GAS, 12 SLOD

FILE: Breadcrumbs2023-07-arcade/contracts/NFTBoostVault.sol

Saves 1 SLOD

+ address delegatee_ = registration.delegatee ;
- 152: if (registration.delegatee == address(0)) {
+ 152: if (delegatee_ == address(0)) {
153:            _registerAndDelegate(user, amount, 0, address(0), delegatee);
154:        } else {
155:            // if user supplies new delegatee address revert
- 156:            if (delegatee != registration.delegatee) revert NBV_WrongDelegatee(delegatee, registration.delegatee);
+ 156:  if (delegatee != delegatee_) revert NBV_WrongDelegatee(delegatee, delegatee_);

Saves 3 SLODs


185: NFTBoostVaultStorage.Registration storage registration = _getRegistrations()[msg.sender];
186:
187:        // If to address is already the delegate, don't send the tx
+  address delegatee_ = registration.delegatee;
- 188:        if (to == registration.delegatee) revert NBV_AlreadyDelegated();
+ 188:        if (to == delegatee_ ) revert NBV_AlreadyDelegated();
189:
190:        History.HistoricalBalances memory votingPower = _votingPower();
- 191:        uint256 oldDelegateeVotes = votingPower.loadTop(registration.delegatee);
+ 191:        uint256 oldDelegateeVotes = votingPower.loadTop(delegatee_ );
192:
193:        // Remove voting power from old delegatee and emit event
+ uint128 latestVotingPower_ = registration.latestVotingPower;
- 194:        votingPower.push(registration.delegatee, oldDelegateeVotes - registration.latestVotingPower);
+ 194:        votingPower.push(delegatee_ , oldDelegateeVotes - latestVotingPower_ );
- 195:        emit VoteChange(msg.sender, registration.delegatee, -1 * int256(uint256(registration.latestVotingPower)));
+ 195:        emit VoteChange(msg.sender, delegatee_ , -1 * int256(uint256(latestVotingPower_ )));

Saves 1 SLOD

231: Storage.Uint256 storage balance = _balance();
+ uint256 data_=balance.data;
- 232: if (balance.data < amount) revert NBV_InsufficientBalance();
+ 232: if (data_ < amount) revert NBV_InsufficientBalance();
233:
234:        // get the withdrawable amount
235:        uint256 withdrawable = _getWithdrawableAmount(registration);
236:        if (withdrawable < amount) revert NBV_InsufficientWithdrawableBalance(withdrawable);
237:
238:        // update contract balance
- 239:        balance.data -= amount;
+ 239:        balance.data =data_- amount;

Saves 4 SLODs

+  address tokenAddress_ = registration.tokenAddress;
+  uint128 tokenId_ = registration.tokenId ; 
- 552:        if (registration.tokenAddress == address(0) || registration.tokenId == 0)
+ 552:        if (tokenAddress_  == address(0) || tokenId_  == 0)
- 553:            revert NBV_InvalidNft(registration.tokenAddress, registration.tokenId);
+ 553:            revert NBV_InvalidNft(tokenAddress_ , tokenId_ );
554:
555:        // transfer ERC1155 back to the user
- 556:        IERC1155(registration.tokenAddress).safeTransferFrom(
+ 556:        IERC1155(tokenAddress_ ).safeTransferFrom(
557:            address(this),
558:            msg.sender,
- 559:            registration.tokenId,
+ 559:            tokenId_ ,
560:            1,
561:            bytes("")
562:        );

Saves 3 SLODs

+ address delegatee_=registration.delegatee;
- uint256 delegateeVotes = votingPower.loadTop(registration.delegatee);
+ uint256 delegateeVotes = votingPower.loadTop(delegatee_);

        uint256 newVotingPower = _currentVotingPower(registration);
        // get the change in voting power. Negative if the voting power is reduced
        int256 change = int256(newVotingPower) - int256(uint256(registration.latestVotingPower));

        // do nothing if there is no change
        if (change == 0) return;
        if (change > 0) {
-            votingPower.push(registration.delegatee, delegateeVotes + uint256(change));
+            votingPower.push(delegatee_, delegateeVotes + uint256(change));
        } else {
            // if the change is negative, we multiply by -1 to avoid underflow when casting
-             votingPower.push(registration.delegatee, delegateeVotes - uint256(change * -1));
+             votingPower.push(delegatee_, delegateeVotes - uint256(change * -1));
        }

        registration.latestVotingPower = uint128(newVotingPower);

-        emit VoteChange(who, registration.delegatee, change);
+        emit VoteChange(who, delegatee_, change);

mintingAllowedAfter should be cached : Saves 100 GAS,1 SLOD

FILE: 2023-07-arcade/contracts/token/ArcadeToken.sol

Saves 1 SLOD

+ uint256 mintingAllowedAfter_ = mintingAllowedAfter;
- 146: if (block.timestamp < mintingAllowedAfter) revert AT_MintingNotStarted(mintingAllowedAfter, block.timestamp);
+ 146: if (block.timestamp < mintingAllowedAfter_ ) revert AT_MintingNotStarted(mintingAllowedAfter_ , block.timestamp);

baseURI should be cached : Saves 100 GAS, 1 SLOD

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/nft/BadgeDescriptor.sol#L49

FILE: Breadcrumbs2023-07-arcade/contracts/nft/BadgeDescriptor.sol

+ string baseURI_ = baseURI;
- 49:  return bytes(baseURI).length > 0 ? string(abi.encodePacked(baseURI, tokenId.toString())) : "";
+ 49:  return bytes(baseURI_ ).length > 0 ? string(abi.encodePacked(baseURI_, tokenId.toString())) : "";

[G-3] <x> += <y> costs more gas than <x> = <x> + <y> for state variables (<x> -= <y> )

Saves 1695 GAS, 15 Instances

Using the addition operator instead of plus-equals saves 113 gas

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ArcadeTreasury.sol#L117

FILE: Breadcrumbs2023-07-arcade/contracts/ArcadeTreasury.sol

- 117: gscAllowance[token] -= amount;
+ 117: gscAllowance[token] = gscAllowance[token]- amount;

- 198: gscAllowance[token] -= amount;
+ 198: gscAllowance[token] = gscAllowance[token] - amount;

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ARCDVestingVault.sol#L166

FILE: Breadcrumbs2023-07-arcade/contracts/ARCDVestingVault.sol

- 166: grant.withdrawn += uint128(withdrawable);
+ 166: grant.withdrawn =grant.withdrawn + uint128(withdrawable);

- 171: grant.withdrawn += uint128(remaining);
+ 171: grant.withdrawn =grant.withdrawn + uint128(remaining);

- 200: unassigned.data += amount;
+ 200: unassigned.data = unassigned.data + amount;

- 242: grant.withdrawn += uint128(withdrawable);
+ 242: grant.withdrawn =grant.withdrawn + uint128(withdrawable);

- 244: grant.withdrawn += uint128(amount);
+ 244: grant.withdrawn =grant.withdrawn + uint128(amount);

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/nft/ReputationBadge.sol#L116

FILE: 2023-07-arcade/contracts/nft/ReputationBadge.sol

- 116: amountClaimed[recipient][tokenId] += amount;
+ 116: amountClaimed[recipient][tokenId] =amountClaimed[recipient][tokenId] + amount;

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/NFTBoostVault.sol#L161

FILE: Breadcrumbs2023-07-arcade/contracts/NFTBoostVault.sol

- 161:  balance.data += amount;
+ 161:  balance.data =balance.data + amount;

- 164: registration.amount += amount;
+ 164: registration.amount =registration.amount + amount;

- 239: balance.data -= amount;
+ 239: balance.data =balance.data - amount;

- 241: registration.withdrawn += amount;
+ 241: registration.withdrawn =registration.withdrawn + amount;

- 278: balance.data += amount;
+ 278: balance.data =balance.data + amount;

- 281: registration.amount += amount;
+ 281: registration.amount =registration.amount +  amount;

[G-4] IF’s/require() statements that check input arguments should be at the top of the function

Saves 3000 GAS

FAIL CHEEPLY INSTEAD OF COSTLY

Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.

Cheaper to check the registration.delegatee == address(0) before making an external function call to IERC1155(newTokenAddress).balanceOf(msg.sender, newTokenId) : Saves 2100 GAS

FILE: 2023-07-arcade/contracts/NFTBoostVault.sol

306: if (newTokenAddress == address(0) || newTokenId == 0) revert NBV_InvalidNft(newTokenAddress, newTokenId);
307:
- 308:        if (IERC1155(newTokenAddress).balanceOf(msg.sender, newTokenId) == 0) revert NBV_DoesNotOwn();
309:
310:        NFTBoostVaultStorage.Registration storage registration = _getRegistrations()[msg.sender];
311:
312:        // If the registration does not have a delegatee, revert because the Registration
313:        // is not initialized
314:        if (registration.delegatee == address(0)) revert NBV_NoRegistration();
+ 308:        if (IERC1155(newTokenAddress).balanceOf(msg.sender, newTokenId) == 0) revert NBV_DoesNotOwn();
315:
316:       // if the user already has an ERC1155 registered, withdraw it

Function parameters should be checked before state variables. If any revert after state variable check this will cost : Saves 900 GAS

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/token/ArcadeTokenDistributor.sol#L74-L75

Any failure after state variable check this will costly failure . So _treasury should be checked first then state variable check

FILE: 2023-07-arcade/contracts/token/ArcadeTokenDistributor.sol

- 74: if (treasurySent) revert AT_AlreadySent();
75: if (_treasury == address(0)) revert AT_ZeroAddress("treasury");
+ 74: if (treasurySent) revert AT_AlreadySent();

- 90: if (devPartnerSent) revert AT_AlreadySent();
91: if (_devPartner == address(0)) revert AT_ZeroAddress("devPartner");
+ 90: if (devPartnerSent) revert AT_AlreadySent();

- 106: if (communityRewardsSent) revert AT_AlreadySent();
107: if (_communityRewards == address(0)) revert AT_ZeroAddress("communityRewards");
+ 106: if (communityRewardsSent) revert AT_AlreadySent();

- 122: if (communityAirdropSent) revert AT_AlreadySent();
123: if (_communityAirdrop == address(0)) revert AT_ZeroAddress("communityAirdrop");
+ 122: if (communityAirdropSent) revert AT_AlreadySent();

- 139: if (vestingTeamSent) revert AT_AlreadySent();
140: if (_vestingTeam == address(0)) revert AT_ZeroAddress("vestingTeam");
+ 139: if (vestingTeamSent) revert AT_AlreadySent();

- 156: if (vestingPartnerSent) revert AT_AlreadySent();
157: if (_vestingPartner == address(0)) revert AT_ZeroAddress("vestingPartner");
+ 156: if (vestingPartnerSent) revert AT_AlreadySent();

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/token/ArcadeToken.sol#L146-L148

FILE: Breadcrumbs2023-07-arcade/contracts/token/ArcadeToken.sol

- 146: if (block.timestamp < mintingAllowedAfter) revert AT_MintingNotStarted(mintingAllowedAfter, block.timestamp);
147: if (_to == address(0)) revert AT_ZeroAddress("to");
148: if (_amount == 0) revert AT_ZeroMintAmount();
+ 146: if (block.timestamp < mintingAllowedAfter) revert AT_MintingNotStarted(mintingAllowedAfter, block.timestamp);

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/token/ArcadeAirdrop.sol#L63-L64

FILE: 2023-07-arcade/contracts/token/ArcadeAirdrop.sol

- 63: if (block.timestamp <= expiration) revert AA_ClaimingNotExpired();
64: if (destination == address(0)) revert AA_ZeroAddress("destination");
+ 63: if (block.timestamp <= expiration) revert AA_ClaimingNotExpired();

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/NFTBoostVault.sol#L224

FILE: 2023-07-arcade/contracts/NFTBoostVault.sol

- 224: if (getIsLocked() == 1) revert NBV_Locked();
225: if (amount == 0) revert NBV_ZeroAmount();
+ 224: if (getIsLocked() == 1) revert NBV_Locked();

[G-5] Multiple accesses of a mapping/array should use a local variable cache

Saves 200 GAS, 2 SLODs

The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping’s value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations. Caching an array’s struct avoids recalculating the array offsets into memory/calldata

lastAllowanceSet[token] should be cached : Saves 100 GAS, 1 SLOT

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ArcadeTreasury.sol#L308-L310

FILE: 2023-07-arcade/contracts/ArcadeTreasury.sol

Saves 100 GAS,1 SLOD

307: // enforce cool down period
+ uint48 _token = lastAllowanceSet[token];
- 308:        if (uint48(block.timestamp) < lastAllowanceSet[token] + SET_ALLOWANCE_COOL_DOWN) {
+ 308:        if (uint48(block.timestamp) < _token  + SET_ALLOWANCE_COOL_DOWN) {
- 309:            revert T_CoolDownPeriod(block.timestamp, lastAllowanceSet[token] + SET_ALLOWANCE_COOL_DOWN);
+ 309:            revert T_CoolDownPeriod(block.timestamp, _token  + SET_ALLOWANCE_COOL_DOWN);
310:        }

amountClaimed[recipient][tokenId] should be cached : Saves 100 GAS, 1 SLOD

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/nft/ReputationBadge.sol#L111-L116

FILE: Breadcrumbs2023-07-arcade/contracts/nft/ReputationBadge.sol

 uint48 claimExpiration = claimExpirations[tokenId];
+  uint256 recipienttokenId = amountClaimed[recipient][tokenId] ;
        if (block.timestamp > claimExpiration) revert RB_ClaimingExpired(claimExpiration, uint48(block.timestamp));
        if (msg.value < mintPrice) revert RB_InvalidMintFee(mintPrice, msg.value);
        if (!_verifyClaim(recipient, tokenId, totalClaimable, merkleProof)) revert RB_InvalidMerkleProof();
-         if (amountClaimed[recipient][tokenId] + amount > totalClaimable) {
+         if (recipienttokenId + amount > totalClaimable) {
            revert RB_InvalidClaimAmount(amount, totalClaimable);
        }

        // increment amount claimed
-         amountClaimed[recipient][tokenId] += amount;
+         amountClaimed[recipient][tokenId] = recipienttokenId + amount;

[G-6] Don't emit state variable when stack variable available

Saves 200 GAS, 2 SLODs

If stack can be emitted when ever available to save gas

_newMinter can be used instead of minter state variable : Saves 100 GAS,1 SLOD

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/token/ArcadeToken.sol#L136

FILE: Breadcrumbs2023-07-arcade/contracts/token/ArcadeToken.sol

135:   minter = _newMinter;
- 136:   emit MinterUpdated(minter);
+ 136:   emit MinterUpdated(_newMinter);

delegatee can be used instead of grant.delegatee state variable : Saves 100 GAS,1 SLOD

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ARCDVestingVault.sol#L148

FILE: Breadcrumbs2023-07-arcade/contracts/ARCDVestingVault.sol

138: grant.delegatee = delegatee;

- 148: emit VoteChange(grant.delegatee, who, int256(uint256(newVotingPower)));
+ 148: emit VoteChange(delegatee, who, int256(uint256(newVotingPower)));

[G-7] Use calldata instead of memory for function parameters

Saves 572 GAS

Note: Missed instance in bot race

calldata can be used instead of memory : Saves 572 GAS

If you are not modifying the function parameters, consider using calldata instead of memory. This will save gas.

FILE: Breadcrumbs2023-07-arcade/contracts/nft/BadgeDescriptor.sol

- 57: function setBaseURI(string memory newBaseURI) external onlyOwner {
+ 57: function setBaseURI(string calldata newBaseURI) external onlyOwner {
58:        baseURI = newBaseURI;
59:
60:        emit SetBaseURI(msg.sender, newBaseURI);
61:    }

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ArcadeTreasury.sol#L334

FILE: 2023-07-arcade/contracts/ArcadeTreasury.sol

333: function batchCalls(
- 334:        address[] memory targets,
+ 334:        address[] calldata targets,
335:        bytes[] calldata calldatas
336:    ) external onlyRole(ADMIN_ROLE) nonReentrant {

NEW GAS SUGGESTIONS

[G-8] batchCalls function, if a single call fails, the entire transaction will be reverted

In the batchCalls function, if a single call fails, the entire transaction will be reverted, and the gas spent on previous successful calls will not be refunded. Consider using a different design that allows partial success and refunds gas for successful calls while reverting only the failed ones.

FILE: 2023-07-arcade/contracts/ArcadeTreasury.sol

function batchCalls(
        address[] memory targets,
        bytes[] calldata calldatas
    ) external onlyRole(ADMIN_ROLE) nonReentrant {
        if (targets.length != calldatas.length) revert T_ArrayLengthMismatch();
        // execute a package of low level calls
        for (uint256 i = 0; i < targets.length; ++i) {
            if (spendThresholds[targets[i]].small != 0) revert T_InvalidTarget(targets[i]);
            (bool success, ) = targets[i].call(calldatas[i]);
            // revert if a single call fails
            if (!success) revert T_CallFailed();
        }
    }

[G-9] Use safeIncreaseAllowance() and safeDecreaseAllowance() instead of approve()

safeIncreaseAllowance()/safeDecreaseAllowance() functions are more gas-efficient consider with approve() function. The approve() function is a standard ERC20 function that allows you to set the allowance for another account to spend your tokens. However, the approve() function is not gas-efficient.

FILE: Breadcrumbs2023-07-arcade/contracts/ArcadeTreasury.sol

200:  _approve(token, spender, amount, spendThresholds[token].small);

219: _approve(token, spender, amount, spendThresholds[token].small);

238:  _approve(token, spender, amount, spendThresholds[token].medium);

257: _approve(token, spender, amount, spendThresholds[token].large);

391:   IERC20(token).approve(spender, amount);

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/ArcadeTreasury.sol#L391

[G-10] Functions like smallSpend, mediumSpend, and largeSpend have similar logic can be refactored to save large volume of GAS

Consider refactoring the code to combine the common logic into a single function and call it from each respective function

FILE: 2023-07-arcade/contracts/ArcadeTreasury.sol

 function smallSpend(
        address token,
        uint256 amount,
        address destination
    ) external onlyRole(CORE_VOTING_ROLE) nonReentrant {
        if (destination == address(0)) revert T_ZeroAddress("destination");
        if (amount == 0) revert T_ZeroAmount();

        _spend(token, amount, destination, spendThresholds[token].small);
    }

function mediumSpend(
        address token,
        uint256 amount,
        address destination
    ) external onlyRole(CORE_VOTING_ROLE) nonReentrant {
        if (destination == address(0)) revert T_ZeroAddress("destination");
        if (amount == 0) revert T_ZeroAmount();

        _spend(token, amount, destination, spendThresholds[token].medium);
    }

 function largeSpend(
        address token,
        uint256 amount,
        address destination
    ) external onlyRole(CORE_VOTING_ROLE) nonReentrant {
        if (destination == address(0)) revert T_ZeroAddress("destination");
        if (amount == 0) revert T_ZeroAmount();

        _spend(token, amount, destination, spendThresholds[token].large);
    }

Recommended Mitigation

FILE: Breadcrumbs2023-07-arcade/contracts/ArcadeTreasury.sol

function spend(
        address token,
        uint256 amount,
        address destination,
        SpendingThreshold threshold
    ) external onlyRole(CORE_VOTING_ROLE) nonReentrant {

        if (destination == address(0)) revert T_ZeroAddress("destination");
        if (amount == 0) revert T_ZeroAmount();
        uint256 spendingThreshold;
        if (threshold == SpendingThreshold.Small) {
            spendingThreshold = spendThresholds[token].small;
        } else if (threshold == SpendingThreshold.Medium) {
            spendingThreshold = spendThresholds[token].medium;
        } else if (threshold == SpendingThreshold.Large) {
            spendingThreshold = spendThresholds[token].large;
        } else {
            revert("Invalid spending threshold");
        }

        _spendWithThreshold(token, amount, destination, spendingThreshold);
    }

Analysis - Arcade.xyz

Summary

Overview

Arcade.xyz is a platform for autonomous borrowing, lending, and escrow of NFT collateral on EVM blockchains. This governance system is built on the Council Framework

Arcade governance classified in 4 types

• Voting Vaults : Each voting vault contract is a separate deployment, which handles its own deposits and vote-counting mechanisms for those deposits
• Core Voting: These contracts can be used to submit and vote on proposed governance transactions.core voting contracts may either administrate the protocol directly, or may be intermediated by a Timelock contract
• Token: The ERC20 governance token, along with contracts required for initial deployment and distribution of the token (airdrop contract, initial distributor contract)
• NFT: The ERC1155 token contract along with its tokenURI descriptor contract. The ERC1155 token used in governance to give a multiplier to a user's voting power

My Thoughts

Arcade.xyz is a platform change the future in following areas

1. it provides a way for users to have a say in how the protocol is governed. This is important because it gives users more control over their assets and ensures that the protocol is aligned with their interests

2. The Arcade governance system is designed to be scalable. This means that it can be used to govern large and complex DeFi protocols. This is important because it will allow DeFi protocols to grow and evolve without sacrificing decentralization.

3. The Arcade governance system is secure. This is because it is built on the Council Framework, which is a battle-tested governance system. This means that the Arcade governance system is less likely to be exploited by malicious actors

Audit approach

I followed below steps while analyzing and auditing the code base.

1. Read the contest Readme.md and took the required notes.

• Arcade.xyz platform uses
• Inheritance
• NFT
• Timelock Functions
• ERC20
• The test coverage is 99%

2. Analyzed the over all codebase one iterations very fast

3. Study of documentation to understand each contract purpose, its functionality, how it is connected with other contracts, etc.

4. Then i read old audits and already known findings. Then go through the bot races findings

5. Then setup my testing environment things. Run the tests to checks all test passed.

6. Finally, I started with the auditing the code base in depth way I started understanding line by line code and took the necessary notes to ask some questions to sponsors.

Stages of audit

• The first stage of the audit

During the initial stage of the audit for Arcade.xyz platform, the primary focus was on analyzing gas usage and quality assurance (QA) aspects. This phase of the audit aimed to ensure the efficiency of gas consumption and verify the robustness of the platform.

• The second stage of the audit

In the second stage of the audit for Arcade.xyz platform, the focus shifted towards understanding the protocol usage in more detail. This involved identifying and analyzing the important contracts and functions within the system. By examining these key components, the audit aimed to gain a comprehensive understanding of the protocol's functionality and potential risks.

• The third stage of the audit

During the third stage of the audit for Arcade.xyz platform, the focus was on thoroughly examining and marking any doubtful or vulnerable areas within the protocol. This stage involved conducting comprehensive vulnerability assessments and identifying potential weaknesses in the system. Found 60-70 vulnerable and weakness code parts all marked with @audit tags.

• The fourth stage of the audit

During the fourth stage of the audit for Arcade.xyz platform, a comprehensive analysis and testing of the previously identified doubtful and vulnerable areas were conducted. This stage involved diving deeper into these areas, performing in-depth examinations, and subjecting them to rigorous testing, including fuzzing with various inputs. Finally concluded findings after all research's and tests. Then i reported C4 with proper formats.

Learnings

Arcade.xyz's governance system, we can understand several key learnings

1. Decentralized Governance : Arcade.xyz allows token holders to vote on platform decisions, ensuring that no single entity or group has complete control. It's a democratic way of making choices for the platform.

2. Voting Vaults and Delegation: Users can deposit their voting tokens in separate vaults and also delegate their voting power to someone they trust, making it easier for more people to participate in voting.

3. ERC20 and ERC1155 Tokens: Arcade.xyz uses two types of tokens - ERC20 for basic voting and ERC1155 for more advanced governance features, like giving some tokens more voting influence.

4. Council Framework : The governance system follows a specific Council Framework, which outlines rules and guidelines for how decisions are made.

5. Protocol Administrations and Timelock : Proposals can directly impact the platform or go through a Timelock first, adding a delay to allow community review and transparency.

6. Autonomy and Smart Contracts: Once set up, the governance system operates autonomously using smart contracts, without needing central control.

7. Transparency and Openness: Arcade.xyz is transparent about how things work, providing clear technical details for users to understand and participate effectively.

8. NFT Integration : Non-fungible tokens (NFTs) can enhance voting power, incentivizing community involvement and participation.

Possible Systemic Risks

Centralization Risk: Voting power could concentrate among a few, leading to centralization.

Delegate Control Concerns: Delegation may reduce voter engagement and representativeness.

Low Participation: Low turnout may undermine governance legitimacy.

Ineffective Proposals: Poorly designed proposals may hinder progress.

Voting Manipulation: Malicious actors may try to influence outcomes.

Token Concentration: Unequal token distribution may skew decisions.

NFT Impact Uncertainty: ERC1155 NFTs' effects may have unintended consequences.

Technical Vulnerabilities: Smart contract bugs could compromise governance.

Community Disagreements: Conflicts may impede decision-making.

Governance Rule Updates: Changing rules may be challenging and require consensus.

Code Commentary

1. Instead of using bytes32 constants for role identifiers, consider using an enum for better readability and code organization
2. Functions like smallSpend, mediumSpend, and largeSpend have similar logic. Consider refactoring the code to combine the common logic into a single internal function and call it from each respective function. This can reduce code duplication
3. For events like TreasuryTransfer and TreasuryApproval, consider emitting only relevant data instead of emitting the entire amount and destination/spender
4. immutable variable should be in upper case
5. Instead of using error messages in revert statements, consider using an enum to define error codes. This can make error handling more structured and easier to understand
6. Ensure that error messages are clear and informative to help developers and users understand contract behavior and potential issues
7. The contract could benefit from more detailed comments and documentation to explain the purpose of various functions, their expected behavior, and any potential risks or constraints. This would make it easier for developers to understand and interact with the contract
8. Consider reordering the modifiers in the function declarations to improve readability. For example, move the onlyManager modifier to be the last one, as it's the most specific and should be applied after other access control checks.
9. Consider using a struct to represent grant data instead of individual storage variables. This can make the code cleaner and easier to manage
10. Instead of importing the entire ERC20 contract, consider using an ERC20 interface with only the necessary functions. This helps to reduce contract deployment costs.
11. The contract uses a mix of camelCase and snake_case for function and variable names. It's advisable to use a consistent naming convention throughout the contract to enhance readability.
12. There are some duplicated code segments (e.g., handling delegation and voting power updates) that could be abstracted into separate helper functions to improve code readability and maintainability.
13. While the contract has some comments, additional documentation could be provided to clarify the purpose and behavior of certain functions and variables. 14.In functions where multiple events are emitted, consider consolidating them into a single emit statement for better gas efficiency

Centralization risks

A single point of failure is not acceptable for this project Centrality risk is high in the project, the role of onlyOwnerdetailed below has very critical and important powers

Project and funds may be compromised by a malicious or stolen private key onlyOwner msg.sender

File: contracts/nft/BadgeDescriptor.sol

57:     function setBaseURI(string memory newBaseURI) external onlyOwner {

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/nft/BadgeDescriptor.sol#L57

File: contracts/token/ArcadeAirdrop.sol

62:     function reclaim(address destination) external onlyOwner {

75:     function setMerkleRoot(bytes32 _merkleRoot) external onlyOwner {

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/token/ArcadeAirdrop.sol#L62

File: contracts/token/ArcadeTokenDistributor.sol

73:     function toTreasury(address _treasury) external onlyOwner {

89:     function toDevPartner(address _devPartner) external onlyOwner {

105:     function toCommunityRewards(address _communityRewards) external onlyOwner {

121:     function toCommunityAirdrop(address _communityAirdrop) external onlyOwner {

138:     function toTeamVesting(address _vestingTeam) external onlyOwner {

155:     function toPartnerVesting(address _vestingPartner) external onlyOwner {

174:     function setToken(IArcadeToken _arcadeToken) external onlyOwner {

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/token/ArcadeTokenDistributor.sol#L73

Gas Optimizations

During the audit of the smart contracts on Arcade.xyz's governance platform, several key gas optimization issues were identified. These issues could potentially lead to increased transaction costs and inefficiencies on the Ethereum network. One of the notable findings was related to state variables, where it was observed that they can be packed together to utilize fewer storage slots. By organizing related variables in this way, the number of storage operations, such as SSTORE and SLOAD opcodes, can be reduced, resulting in significant gas savings.

Another prevalent optimization opportunity involved caching state variables in stack variables instead of repeatedly re-reading them from storage. This approach can eliminate redundant storage operations, leading to improved gas efficiency. Utilizing opcodes like SWAP, DUP, and MLOAD efficiently allows for cost-effective access to state variables during contract execution.

In addition, a gas-saving technique was found in arithmetic operations on state variables. It was observed that using the = operator rather than += and -= could result in reduced gas consumption for the ADD, SUB, and MSTORE opcodes. Moving IF statements or require() functions that check input arguments to the top of the functions was another recommendation to avoid unnecessary code execution, thereby saving gas. This optimization takes advantage of the JUMP and JUMPI opcodes, which control the flow of execution within the contract.

Furthermore, multiple accesses of mappings or arrays were identified as potential areas for improvement. By employing local variable caches, redundant read operations from storage can be minimized, reducing gas costs. The relevant opcodes in this context include MLOAD, MSTORE, and SLOAD.

Moreover, it was noticed that emitting state variables when stack variables are available could lead to unnecessary gas consumption. Emitting state variables involves storage operations, while stack variables are more gas-efficient. The LOG opcode is used for emitting event logs.

Lastly, optimizing function parameters by using calldata instead of memory can save gas. The CALLDATASIZE opcode is used to access the size of the input data, and CALLDATALOAD is used to retrieve specific function parameters from the input.

https://code4rena.com/contests/2023-07-arcadexyz/submit?issue=151

Possible Risks as per my analysis

• The batchCalls function allows executing multiple low-level calls in a single transaction. If a large number of calls are included or if any of the calls fail, the entire transaction will be reverted, leading to gas wastage. This can result in higher transaction costs and failed transactions due to gas limits.

• The smallSpend, mediumSpend, largeSpend, approveSmallSpend, approveMediumSpend, and approveLargeSpend functions do not perform input validation to check whether the amount parameter exceeds the token balance or allowance. This could lead to unintended spending or approval of more tokens than the treasury holds or intends to allow.

• The _spend and _approve functions enforce a per-block spending/approval limit (limit) to avoid excessive spending. However, there is no mechanism to reset the blockExpenditure storage variable when a new block starts, which could lead to inconsistencies if the contract is active across multiple blocks.

• In the _spend function, if the destination is a contract without a receive function, the transfer will fail, resulting in a loss of funds. The function should include checks to handle such cases appropriately.

• Hardcoded cooldown period may cause problem in future

• Lack of same value input control in critical setMinter() functions

• Use safeInceaseAllowance()/safeDecreaseAllowance() instead of approve() or deprecated safeApprove() Functions

• Don't use payable(address).tranfer() to avoid unexpected fund lose

• The function addGrantAndDelegate allows the manager to create a new grant without checking if the grant recipient already has an active grant. If a user already has an active grant, this function will overwrite the existing grant, which might lead to unintended behavior. It would be better to check if the recipient already has a grant and handle this situation accordingly.

• In the _getWithdrawableAmount function, there are multiple arithmetic calculations involving subtraction, addition, and multiplication. Ensure that these calculations do not result in underflow or overflow situations, which could lead to unintended behavior or vulnerabilities

• The contract uses a custom state management system with the Storage library, which might make it harder to understand for developers not familiar with this approach. Consider using standardized storage patterns to improve code readability and maintainability.

• Some functions, such as _lockNft, _lockTokens, and _grantVotingPower, are marked as internal but are not strictly necessary to be internal. It's essential to assess whether these functions need to be called from other contracts or external interactions

Time spent on analysis

15 Hours

Time spent:

15 hours