ArcadeTreasury.sol allowance may be override

[code423n4]

Lines of code

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

Vulnerability details

Impact

direct use of IERC20(token).approve(spender, amount); causes the same spender allowances to be overridden by each other

Proof of Concept

In the gscApprove() method it is possible to give spender a certain allowance

The code is as follows:

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

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

        emit TreasuryApproval(token, spender, amount);
    }    

From the above code we can see that when executed gscApprove consumes gscAllowance[] and ultimately uses IERC20(token).approve(); to give the spender allowance

Since the direct use is IERC20.approve(spender, amount), the amount of the allowance is overwritten, whichever comes last

In the other methods approveSmallSpend,approveMediumSpend,approveLargeSpend also use IERC20(token).approve();, which causes them to override each other if targeting the same spender.

Even if there is a malicious GSC_CORE_VOTING_ROLE, it is possible to execute gscApprove(amount=1 wei) after approveLargeSpend() to reset to an allowance of only 1 wei.

The recommendation is to use accumulation to avoid, intentionally or unintentionally, overwriting each other

Tools Used

Recommended Mitigation Steps

    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);
+      uint256 old = IERC20(token).allowance(address(this),spender);
+      IERC20(token).approve(spender,  old + amount);

        emit TreasuryApproval(token, spender, amount);
    } 

Assessed type

Context

[c4-pre-sort]

141345 marked the issue as primary issue

[141345]

https://github.com/code-423n4/2023-07-arcade-findings/issues/58 talks about the issue with internal accounting of gscAllowance. Another aspect of the issue.

And this report shows some unexpected result due to external erc20 allowance overwrite.

[c4-sponsor]

PowVT marked the issue as sponsor confirmed

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report

[Nabeel-javaid]

Doesn't it falls under the admin mistake, admin would be aware and can change these to desired values anytime.

[0xean]

Doesn't it falls under the admin mistake, admin would be aware and can change these to desired values anytime.

I don't believe this should be view as input sanitization as the core functionality is written in a way that has unexpected consequences that could affect the functionality of the protocol. I can see arguing for QA, I welcome more comments from other wardens or sponsor ( @c4-sponsor )

[nevillehuang]

Doesn't it falls under the admin mistake, admin would be aware and can change these to desired values anytime.

I don't believe this should be view as input sanitization as the core functionality is written in a way that has unexpected consequences that could affect the functionality of the protocol. I can see arguing for QA, I welcome more comments from other wardens or sponsor ( @c4-sponsor )

Correct me if im wrong, I think it is intended for allowance to be overriden for spender when it is decreased, given changing of approval needs to go through a GSC proposal

But since this report also highlights the potential inability to reduce allowance due to underflow in the gscAllowance mapping, it should be duplicated with #58

Again correct me if im wrong, If the above reasoning is correct, #55, #146 and #535 should be invalidated since they do not mention the possible DoS when decreasing allowance.

[jingyi2811]

Hi Oxean. I don't think the item on qa report 506 is the duplicate of this issue. Tqs