Arcade.xyz - Juntao's results

Lines of code

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

Vulnerability details

Impact

Approved gscApprove allowance to an address may not able to be decreased.

Proof of Concept

GSC_CORE_VOTING_ROLE calls gscApprove(...) function to approve tokens to be pulled from the treasury:

    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);
    }

Each approval will decrease the gscAllowance[token] by the approved allowance amount, this is problematic as GSC_CORE_VOTING_ROLE may not able to decrease the approved allowance.

Consider the following scenario:

1. gscAllowance[token] is 100;
2. GSC_CORE_VOTING_ROLE calls gscApprove(...) to give 60 allowance to a third party;
3. gscAllowance[token] is 40 now;
4. Later GSC_CORE_VOTING_ROLE finds the approved allowance is a bit too high and want to decrease the allowance to 50;
5. gscApprove(...) is called again but the ransaction reverts due to underflow error (gscAllowance[token] is less than 50)

Please see the tests:

contract ArcadeTreasuryTest is Test {
    address timelock = address(1);
    address coreVoting = address(2);
    address gscCoreVoting = address(3);
    address other = address(4);

    ArcadeTreasury treasury;
    MockToken mockToken;

    function setUp() public {
        treasury = new ArcadeTreasury(timelock);

        vm.startPrank(timelock);
        treasury.grantRole(treasury.CORE_VOTING_ROLE(), coreVoting);
        treasury.grantRole(treasury.GSC_CORE_VOTING_ROLE(), gscCoreVoting);

        mockToken = new MockToken("Mock Token", "MT");
        IArcadeTreasury.SpendThreshold memory threshold = IArcadeTreasury.SpendThreshold(100 ether, 200 ether, 300 ether);
        treasury.setThreshold(address(mockToken), threshold);
        vm.stopPrank();
    }

    function testGscApprove() public {
        vm.warp(7 days);

        vm.prank(timelock);
        treasury.setGSCAllowance(address(mockToken), 100 ether);

        vm.startPrank(gscCoreVoting);
        treasury.gscApprove(address(mockToken), address(4), 60 ether);
        vm.expectRevert(stdError.arithmeticError);
        treasury.gscApprove(address(mockToken), address(4), 50 ether);
        vm.stopPrank();
    }
}

contract MockToken is ERC20 {
    constructor(string memory name_, string memory symbol_) ERC20(name_, symbol_) {}

    function mint(address account, uint256 amount) public {
        _mint(account, amount);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

When approve to a particular address, compare the new allowance with current allowance (IERC20.allowance(...)):

1. If the current allowance is equal to new allowance, do nothing;
2. If the current allowance is less than new allowance, gscAllowance[token] -= (new allowance - current allowance);
3. IF the current allowance is larger than new allowance, gscAllowance[token] += (current allowance - new allowance).

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Tokens may be spent without CORE_VOTING_ROLE approval.

Proof of Concept

ArcadeTreasury transfers or approves them based on invocations from governance. GSC_CORE_VOTING_ROLE may be authorized to spend or approve smaller amounts (gscAllowance) from their own voting contract, all other amounts must be authorized by full community votes.

ADMIN_ROLE can set the gscAllowance for a token by calling setGSCAllowance(...) function:

    function setGSCAllowance(address token, uint256 newAllowance) external onlyRole(ADMIN_ROLE) {
        if (token == address(0)) revert T_ZeroAddress("token");
        if (newAllowance == 0) revert T_ZeroAmount();


        // enforce cool down period
        if (uint48(block.timestamp) < lastAllowanceSet[token] + SET_ALLOWANCE_COOL_DOWN) {
            revert T_CoolDownPeriod(block.timestamp, lastAllowanceSet[token] + SET_ALLOWANCE_COOL_DOWN);
        }


        uint256 spendLimit = spendThresholds[token].small;
        // new limit cannot be more than the small threshold
        if (newAllowance > spendLimit) {
            revert T_InvalidAllowance(newAllowance, spendLimit);
        }


        // update allowance state
        lastAllowanceSet[token] = uint48(block.timestamp);
        gscAllowance[token] = newAllowance;


        emit GSCAllowanceUpdated(token, newAllowance);
    }

However, part of gscAllowance may be approved to third parties and the allowance is not reset, thus tokens may be spent without CORE_VOTING_ROLE approval.

Consider the following scenario:

1. GSC_CORE_VOTING_ROLE approves 100 ether to a third party;
2. ADMIN_ROLE set the gscAllowance to 80 ether;
3. Third party pulls 100 ether from treasury, results in 20 ether being spent without CORE_VOTING_ROLE approval.

Please see the tests:

contract ArcadeTreasuryTest is Test {
    address timelock = address(1);
    address coreVoting = address(2);
    address gscCoreVoting = address(3);
    address other = address(4);

    ArcadeTreasury treasury;
    MockToken mockToken;

    function setUp() public {
        treasury = new ArcadeTreasury(timelock);

        vm.startPrank(timelock);
        treasury.grantRole(treasury.CORE_VOTING_ROLE(), coreVoting);
        treasury.grantRole(treasury.GSC_CORE_VOTING_ROLE(), gscCoreVoting);

        mockToken = new MockToken("Mock Token", "MT");
        IArcadeTreasury.SpendThreshold memory threshold = IArcadeTreasury.SpendThreshold(100 ether, 200 ether, 300 ether);
        treasury.setThreshold(address(mockToken), threshold);
        vm.stopPrank();

        uint256 gscAllowance = 100 ether;

        mockToken.mint(address(treasury), 300 ether);
        uint256 treasuryBalanceBefore = mockToken.balanceOf(address(treasury));
        assertEq(treasuryBalanceBefore, 300 ether);

        vm.warp(7 days);
        vm.prank(timelock);
        treasury.setGSCAllowance(address(mockToken), gscAllowance);
    }

    function testDecreaseGscAllowance() public {
        vm.prank(gscCoreVoting);
        treasury.gscApprove(address(mockToken), other, gscAllowance);

        vm.warp(14 days);
        uint256 gscAllowanceNew = 80 ether;

        vm.prank(timelock);
        treasury.setGSCAllowance(address(mockToken), gscAllowanceNew);

        vm.prank(other);
        mockToken.transferFrom(address(treasury), other, gscAllowance);

        uint256 treasuryBalanceAfter = mockToken.balanceOf(address(treasury));
        assertTrue(treasuryBalanceAfter < treasuryBalanceBefore - gscAllowanceNew);
        assertEq(mockToken.balanceOf(address(treasury)), 200 ether);
    }

}

contract MockToken is ERC20 {

    constructor(string memory name_, string memory symbol_) ERC20(name_, symbol_) {}

    function mint(address account, uint256 amount) public {
        _mint(account, amount);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Allowance to third parties should be revoked when set the gscAllowance.

Assessed type

Access Control