Arcade.xyz - QiuhaoLi's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

To prevent the admin calling tokens' addresses directly and bypassing spending functions, we check if spendThresholds[targets[i]].small != 0 in the batchCalls():

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

However, the check can be bypassed, e.g.:

1. Admin makes AcradeTreasury calls token A contract, which is not setThreshold yet.
2. AcradeTreasury calls A.approve(spender, amount).
3. AcradeTreasury is deposited with token A.
4. The spender spends the tokens, bypassing the spend functions.

Tools Used

Manual Review

Recommended Mitigation Steps

An ad-hoc fix may be forbidding any token (including ones not set thresholds yet) contract calls. But in the long term, I think we should limit what admin can call with finer-grained functions and void this batchCalls funciton.

Assessed type

ERC20