Arcade.xyz - caventa's results

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/NFTBoostVault.sol#L182-L212

Vulnerability details

Impact

The value of voting power for a delegate can be incorrect because it uses the old latestVotingPower value.

Proof of Concept

Refer to the following code

function setMultiplier(address tokenAddress, uint128 tokenId, uint128 multiplierValue) public override onlyManager {
        if (multiplierValue > MAX_MULTIPLIER) revert NBV_MultiplierLimit();

        NFTBoostVaultStorage.AddressUintUint storage multiplierData = _getMultipliers()[tokenAddress][tokenId];
        // set multiplier value
        multiplierData.multiplier = multiplierValue;

        emit MultiplierSet(tokenAddress, tokenId, multiplierValue);
    }

function delegate(address to) external override {
        if (to == address(0)) revert NBV_ZeroAddress("to");

        NFTBoostVaultStorage.Registration storage registration = _getRegistrations()[msg.sender];

        // If to address is already the delegate, don't send the tx
        if (to == registration.delegatee) revert NBV_AlreadyDelegated();

        History.HistoricalBalances memory votingPower = _votingPower();
        uint256 oldDelegateeVotes = votingPower.loadTop(registration.delegatee);

        // Remove voting power from old delegatee and emit event
        votingPower.push(registration.delegatee, oldDelegateeVotes - registration.latestVotingPower);
        emit VoteChange(msg.sender, registration.delegatee, -1 * int256(uint256(registration.latestVotingPower)));

        // Note - It is important that this is loaded here and not before the previous state change because if
        // to == registration.delegatee and re-delegation was allowed we could be working with out of date state
        uint256 newDelegateeVotes = votingPower.loadTop(to);
        // return the current voting power of the Registration. Varies based on the multiplier associated with the
        // user's ERC1155 token at the time of txn
        uint256 addedVotingPower = _currentVotingPower(registration);

        // add voting power to the target delegatee and emit event
        votingPower.push(to, newDelegateeVotes + addedVotingPower);

        // update registration properties
        registration.latestVotingPower = uint128(addedVotingPower);
        registration.delegatee = to;

        emit VoteChange(msg.sender, to, int256(addedVotingPower));
    }

Manager may update multiplier by calling NFTBoostVault#setMultiplier from time to time and it does not update all the latestVotingPower that belongs to every registration. When user calls NFTBoostVault#delegate to remove an existing delegate, it still uses the old latestVotingPower that use the old multiplier.

See the following code snippet from NFTBoostVault#delegate

  History.HistoricalBalances memory votingPower = _votingPower();
  uint256 oldDelegateeVotes = votingPower.loadTop(registration.delegatee);

  // Remove voting power from old delegatee and emit event
  votingPower.push(registration.delegatee, oldDelegateeVotes - registration.latestVotingPower);

Tools Used

Manual and wrote a simple hardhat test

Recommended Mitigation Steps

We need to use the new latestVotingPower

Change NFTBoostVault#delegate

 function delegate(address to) external override {
        if (to == address(0)) revert NBV_ZeroAddress("to");

        NFTBoostVaultStorage.Registration storage registration = _getRegistrations()[msg.sender];

        // If to address is already the delegate, don't send the tx
        if (to == registration.delegatee) revert NBV_AlreadyDelegated();

        History.HistoricalBalances memory votingPower = _votingPower();
        uint256 oldDelegateeVotes = votingPower.loadTop(registration.delegatee);

        +++ _syncVotingPower(msg.sender, registration); // @audit

        // Remove voting power from old delegatee and emit event
        votingPower.push(registration.delegatee, oldDelegateeVotes - registration.latestVotingPower);
        emit VoteChange(msg.sender, registration.delegatee, -1 * int256(uint256(registration.latestVotingPower)));

        // Note - It is important that this is loaded here and not before the previous state change because if
        // to == registration.delegatee and re-delegation was allowed we could be working with out of date state
        uint256 newDelegateeVotes = votingPower.loadTop(to);
        // return the current voting power of the Registration. Varies based on the multiplier associated with the
        // user's ERC1155 token at the time of txn
        uint256 addedVotingPower = _currentVotingPower(registration);

        // add voting power to the target delegatee and emit event
        votingPower.push(to, newDelegateeVotes + addedVotingPower);

        // update registration properties
        registration.latestVotingPower = uint128(addedVotingPower);
        registration.delegatee = to;

        emit VoteChange(msg.sender, to, int256(addedVotingPower));
    }

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/ArcadeTreasury.sol#L189-L201 https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/ArcadeTreasury.sol#L384-L394

Vulnerability details

Impact

Calling ArcadeTreasury#gscApprove more than 1 times should not reset approval

Proof of Concept

Refer to the following code

 function gscApprove(
        address token,
        address spender,
        uint256 amount
    ) external onlyRole(GSC_CORE_VOTING_ROLE) nonReentrant {
        if (spender == address(0)) revert T_ZeroAddress("spender");
        if (amount == 0) revert T_ZeroAmount();

        // Will underflow if amount is greater than remaining allowance
        gscAllowance[token] -= amount; // @audit1 see the deduction here

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

   function _approve(address token, address spender, uint256 amount, uint256 limit) internal {
        // check that after processing this we will not have spent more than the block limit
        uint256 spentThisBlock = blockExpenditure[block.number];
        if (amount + spentThisBlock > limit) revert T_BlockSpendLimit();
        blockExpenditure[block.number] = amount + spentThisBlock;

        // approve tokens
        IERC20(token).approve(spender, amount); // @audit2 always use the latest amount

        emit TreasuryApproval(token, spender, amount);
    }

Admins with GSC_CORE_VOTING_ROLE role can call ArcadeTreasury#gscApprove for many times and gscAllowance should approve the ACCUMULATED ammount like how gscAllowance[token] is calculated

(See @audit1)

gscAllowance[token] -= amount;

However, right now, system always overwrites the approval amount with the final amount and not with the ACCUMULATED amount.

(See @audit2)

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

Tools Used

Manual and wrote a simple hardhat test

Recommended Mitigation Steps

Create a new function named _approveAccumulate which is modified from the existing _approve function. Approve the new amount together with the old allowance value. See @audit.

  function _approveAccumulate(address token, address spender, uint256 amount, uint256 limit) internal {
        // check that after processing this we will not have spent more than the block limit
        uint256 spentThisBlock = blockExpenditure[block.number];
        if (amount + spentThisBlock > limit) revert T_BlockSpendLimit();
        blockExpenditure[block.number] = amount + spentThisBlock;

        uint256 allowance = IERC20(token).allowance(address(this), spender); // @audit
        // approve tokens
        IERC20(token).approve(spender, amount + allowance); // @audit

        emit TreasuryApproval(token, spender, amount);
    }

Change ArcadeTreasury#gscApprove to use the new _approveAccumulate function

 function gscApprove(
        address token,
        address spender,
        uint256 amount
    ) external onlyRole(GSC_CORE_VOTING_ROLE) nonReentrant {
        if (spender == address(0)) revert T_ZeroAddress("spender");
        if (amount == 0) revert T_ZeroAmount();

        // Will underflow if amount is greater than remaining allowance
        gscAllowance[token] -= amount;

        --- _approve(token, spender, amount, spendThresholds[token].small);
        +++ _approveAccumulate(token, spender, amount, spendThresholds[token].small);
    }

Assessed type

Other