Arcade.xyz - ladboy233's results

Judge has assessed an item in Issue #513 as 2 risk. The relevant finding follows:

Issue 2

Lines of code

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

Vulnerability details

Line of code:

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

Impact

Lose of previous spending power for same address in TreasuryContract

Proof of Concept

the AracheTreasury, there are three spending relate function:

approveSmallSpend, approveMediumSpend, approveLargeSpend

both of these function call

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

or the approved amount is spendThresholds[token].medium or spendThresholds[token].large

Then inside the function _approve, approve a certain amount

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

emit TreasuryApproval(token, spender, amount);

However, if a spender approve a largeSpending amount, then approve the mdiumSpending amount, the unconsumed largeSpending amount is lost

for example largeSpending amount is 1000 ETH, mediumSpending amount is 300 ETH

a spender has 1000 ETH spending allowance after call approveLargeSpending

he never spend the allowance

then a proposal executes and caled approveMediumSpending

now the spender's largeSpending amount 1000 ETH is lost, he only has 300 ETH spending allowance left

Tools Used

Manual Review

Recommended Mitigation Steps

Use increaseAllowance instead of calling approve

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/external/council/CoreVoting.sol#L15

Vulnerability details

Impact

Incorrectly assume the block generation time is 13.3 seconds, while the block generation in ethereum in 12 seconds per block,

result in a gradually delayed voting period

Proof of Concept

ArcadeGSCCoreVoting.sol is in-scope and it is inherit from the CoreVoting contract

contract ArcadeGSCCoreVoting is CoreVoting {

In CoreVoting Contract:

the DAY_IN_BLOCK is hardcoded to 6496,

    // Assumes avg block time of 13.3 seconds. May be longer or shorter due
    // to ice ages or short term changes in hash power.
    // @audit
    // 12 seconds
    uint256 public constant DAY_IN_BLOCKS = 6496;

as the comment suggest, the code assume the avg block time is 13.3 seconds

In one day we assume it has 86400 seconds / 6496 block = 13.3 seconds

However, the block production time in post merge is 12 seconds

https://ethereum.org/en/developers/docs/consensus-mechanisms/pos/#:~:text=Whereas%20under%20proof%2Dof%2Dwork,block%20proposer%20in%20every%20slot

Then one can day can produce: 86400 / 12 = 7200 blocks

the parameter DAY_IN_BLOCKS is used when derving the lock duration and the voting time of the proposal

// minimum time a proposal must be active for before executing
// Default to 3 days, this avoids weekend surprise proposals
uint256 public lockDuration = DAY_IN_BLOCKS * 3;

// The number of blocks after the proposal is unlocked during which
// voting can continue. Max vote time = lockDuration + extraVoteTime
// Default to ~5 days of blocks, ie 8 days max vote time
uint256 public extraVoteTime = DAY_IN_BLOCKS * 5;

and the DAY_IN_BLOCKS is off by (7200 - 6496) = 704 blocks, which result in a much shorter lockDuration and voting time then expected

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol to use the block.timestamp (one day has 86400 seconds) instead of using the DAY_IN_BLOCK to measure the time elapse when deriving lockDuration and vote time in ArcadeGSCCoreVoting.sol

Assessed type

Other

Issue 1:

merkle leaf value should not be 64 bytes before hashing in MerkleReward Distribution

Issue 2:

_syncVotingPower can revert in underflow in NFTBoostVault.sol and ARCDVestingVault.sol

---

merkle leaf value should not be 64 bytes before hashing in MerkleReward Distribution

line of code:

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/external/council/libraries/MerkleRewards.sol#L85

Impact

merkle leaf value should not be 64 bytes before hashing

Proof of Concept

In merkle reward distribution code, we have to follow function:

   function _validateWithdraw(
        uint256 amount,
        uint256 totalGrant,
        bytes32[] memory merkleProof
    ) internal {
        // Hash the user plus the total grant amount
        bytes32 leafHash = keccak256(abi.encodePacked(msg.sender, totalGrant));

        // Verify the proof for this leaf
        require(
            MerkleProof.verify(merkleProof, rewardsRoot, leafHash),
            "Invalid Proof"
        );
        // Check that this claim won't give them more than the total grant then
        // increase the stored claim amount
        require(claimed[msg.sender] + amount <= totalGrant, "Claimed too much");
        claimed[msg.sender] += amount;
    }

note:

// Hash the user plus the total grant amount
bytes32 leafHash = keccak256(abi.encodePacked(msg.sender, totalGrant));

// Verify the proof for this leaf
require(
	MerkleProof.verify(merkleProof, rewardsRoot, leafHash),
	"Invalid Proof"
);

we cancat the address msg.sender and totalGrant as merkle tree, which is 64 bytes exactly

In Openzepplein library, we have the following comments

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/02ea01765a9964541dd9cdcffc4a7f8b403c2ff6/contracts/utils/cryptography/MerkleProof.sol#L13

 * WARNING: You should avoid using leaf values that are 64 bytes long prior to
 * hashing, or use a hash function other than keccak256 for hashing leaves.
 * This is because the concatenation of a sorted pair of internal nodes in
 * the merkle tree could be reinterpreted as a leaf value.

The merkle tree intermediary nodes are just concatenated and hashed during verification, and since they are also of size bytes32/uint256 this type of collision is possible because the leaf of the tree is of the same form in this case (address, uint256) as the internal nodes.

then if the attacker control two msg.sender address + the totalGrant, he can claim the reward for free

Tools Used

Manual Review

Recommended Mitigation Steps

Add one bytes to the hashed leaf

---

_syncVotingPower can revert in underflow in NFTBoostVault.sol and ARCDVestingVault.sol

line of code:

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

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

Impact

_syncVotingPower can revert in underflow in NFTBoostVault.sol

Proof of Concept

note this function:

   function _syncVotingPower(address who, NFTBoostVaultStorage.Registration storage registration) internal {
        History.HistoricalBalances memory votingPower = _votingPower();
        uint256 delegateeVotes = votingPower.loadTop(registration.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));
        } else {
            // if the change is negative, we multiply by -1 to avoid underflow when casting
            votingPower.push(registration.delegatee, delegateeVotes - uint256(change * -1));
        }

        registration.latestVotingPower = uint128(newVotingPower);

        emit VoteChange(who, registration.delegatee, change);
    }

We need to pay special attention to this line of code:

  votingPower.push(registration.delegatee, delegateeVotes - uint256(change * -1));

it is possible that delegateeVotes - changes can revert in underflow

this only happens when we try to reduce the voting power

In _withdrawNFT if a NFT with high multipler is removed in NFTBoostVault.sol and cause the voting power to decrease

// update the delegatee's voting power based on multiplier removal
_syncVotingPower(msg.sender, registration);

Or in ARCDVestingVault.sol when a admin try to revoke a user's grant

// update the grant's withdrawn amount
if (amount == withdrawable) {
	grant.withdrawn += uint128(withdrawable);
} else {
	grant.withdrawn += uint128(amount);
	withdrawable = amount;
}

// update the user's voting power
_syncVotingPower(msg.sender, grant);

Tools Used

Manual Review

Recommended Mitigation Steps

Before calling delegateeVotes - uint256(change * -1), make sure delegateeVotes > uint256(change * -1), otherwise just set to 0