Upgrades to EdgeChallengeManager changing staking requirement can cause `funds loss`

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/challengeV2/EdgeChallengeManager.sol#L429 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/challengeV2/EdgeChallengeManager.sol#L580

Vulnerability details

The fact that EdgeChallengeManager.sol is upgradeble, if an Admin would be to upgrade the contract and update stakeToken and/or stakeAmounts, that would result in critical consequences.

This report is related to TOB-ARBCH-31 from the TrailOfBits audit which was identified as Informational. Reader should read that finding prior to continue reading this report to get proper context. This report will provide additional information how this would have a critical impacts which increase the severity of the original finding and how the current contrat doesn't prevent an Admin todo such upgrade, coupled with the fact that not all the team member seems to be fully aware of this (see discussion from Discord, refer to last section), which seems to warrant Medium severity overall.

Impacts (stakeAmounts)

An upgrade implying a change in the staking requirement would be problematic (using WETH as stakeToken):

1. No stake required --> 100 WETH stake required: A user which have created a level edge zero before the upgrade might be able to be refunded 100 WETH after the upgrade even if he didn't stake any at the edge creation (see POC).
2. 50 WETH stake required --> 100 WETH stake required: This is very similar to impact 1. A user which have created a level edge zero before the upgrade might be able to be refunded 100 WETH (instead of his 50 WETH staked), so the difference betweem the new stake requirement - the old stake requirement. This impact is the most realistic to be happening more often in production.
3. 100 WETH stake required --> 50 WETH stake required: A user which have created a level edge zero before the upgrade will not be able to get refunded his 100 WETH even if he did stake such amount at the edge creation since now the contract will refund only the new value, which is 50 WETH, so half of his original value.
4. 100 WETH stake required --> no stake required: A user which have created a level edge zero before the upgrade will not be able to get refunded his 100 WETH even if he did stake such amount at the edge creation since now the contract will skip the transfer, so he will get nothing.

    function refundStake(bytes32 edgeId) public {
        ChallengeEdge storage edge = store.get(edgeId);
        // setting refunded also do checks that the edge cannot be refunded twice
        edge.setRefunded();

        IERC20 st = stakeToken;
        uint256 sa = stakeAmounts[edge.level];
        // no need to refund with the token or amount where zero'd out
        if (address(st) != address(0) && sa != 0) {                                // <------------------------- THIS condition will SKIP the transfer, as identified also in the TrailOfBits audit
            st.safeTransfer(edge.staker, sa);
        }

        emit EdgeRefunded(edgeId, store.edges[edgeId].mutualId(), address(st), sa);
    }

Impacts (stakeToken)

5. address(0) --> WETH: Same as Impact #1.
6. WETH --> address(0): Same as Impact #4.

7. WETH --> WBTC: This will be ok for direct users, but for pool's user that will be catastrophic as switching to a new token would break the staking pool logic (both, assertion and edge) and users partipating in such pool would lost their entire stake. The reason is since the stakeToken is stored at pool creation (eg: WETH), when EdgeChallengeManager get upgraded to a new stakeToken (eg: WBTC), those tokens will still be transfered to the pool's contract when refunding/withdrawing occurs, but they will be stuck there forever, as the safeTransfer from withdrawFromPool will fail, as it will try to transfer the initial token (WETH) while the contract will now own new one (WBTC).

   function withdrawFromPool(uint256 amount) public {
       if (amount == 0) {
           revert ZeroAmount();
       }
       uint256 balance = depositBalance[msg.sender];
       if (amount > balance) {
           revert AmountExceedsBalance(msg.sender, amount, balance);
       }
   
       depositBalance[msg.sender] = balance - amount;
       IERC20(stakeToken).safeTransfer(msg.sender, amount); // <--- THIS will fail as the contract will not own any WETH, but WBTC
   
       emit StakeWithdrawn(msg.sender, amount);
   }

Be aware that all the mentionned impacts are also affecting EdgeStakingPool contract users, which ultimatelly could represent Alice or Bob in the following PoC. There is a twist thought, in case of Impact #2, the additional token transfered to EdgeStakingPool would be stuck in it forever. The reason being that depositIntoPool and withdrawFromPool will be limited to the amount having being accounted only, so the developer assumption here would be broken.

///         Tokens sent directly to this contract will be lost.
///         It is assumed that the challenge manager will not return more tokens than the amount deposited by the pool.
///         Any tokens exceeding the deposited amount to the pool will be stuck in the pool forever.

Proof of Concept

This is showcasing Impact 1. We have 3 actors in play: Admin, Alice and Bob. 1) Admin deploy ECM contract without staking required to create level zero edge. 2) Alice decide to create a challenge calling ECM::createLayerZeroEdge, no token are transfered as it's stake free. 3) Afterward (but before Alice challenge is completed), Admin decides to upgrade the contract changing the staking requirement, now requiring 100 WETH to create a level zero edge. 4) Bob decide to create a level zero edge for different claim/assertion (so not an Alice's rival edge). 5) Once Alice's challenge period is completed, she is able to confirm his claim, and afterward call ECM::refundStake. Even if she haven't staked any token, the ECM will send her back the 100 WETH, and since the contract only own 100 WETH (staked by Bob in the previous step), so effectivelly stealing the Bob's stake. (user funds loss) 6) Once Bob has his claim confirmed later on (assuming he wins his claim/assertion too), calling ECM::refundStake will be an odd surprise as the call will revert, as the contract will lack funds. (protocol insolvency)

Recommended Mitigation Steps

If an upgrade is required for the stakeToken and/or stakeAmounts, normal upgrade can't be used, and only a complete upgrade which would also upgrade the proxy (so not re-use the storage) and officialize it with RollupAdminLogic::setChallengeManager afterward. That should be documented very clearly and explicitly as the current setup as the potential to break things down the road, but the proper fix would be to make those state variables immutable, which would prevent this completely.

Otherwise, you could track the staked amount/token inside the ChallengeEdge struct instead of having a single global instance and integrate it properly into ECM, that should allow to do normal upgrade. Here would be a draft implementation.

struct ChallengeEdge {
    ... // omitting to save space

    uint8 level;
    /// @notice Set to true when the staker has been refunded. Can only be set to true if the status is Confirmed
    ///         and the staker is non zero.
    bool refunded;
+   /// @notice The amount staked for this edge.
+   ///         Only populated on zero layer edges
+   uint256 stakedAmount;
+   /// @notice The token staked.
+   ///         Only populated on zero layer edges
+   address stakeToken;    
    /// @notice TODO
    uint64 totalTimeUnrivaledCache;
}

    function refundStake(bytes32 edgeId) public {
        ChallengeEdge storage edge = store.get(edgeId);
        // setting refunded also do checks that the edge cannot be refunded twice
        edge.setRefunded();

-       IERC20 st = stakeToken;
-       uint256 sa = stakeAmounts[edge.level];
+       IERC20 st = edge.stakeToken;
+       uint256 sa = edge.stakedAmount;
        // no need to refund with the token or amount where zero'd out
        if (address(st) != address(0) && sa != 0) {
            st.safeTransfer(edge.staker, sa);
        }

        emit EdgeRefunded(edgeId, store.edges[edgeId].mutualId(), address(st), sa);
    }

Discord private discussion

Here is a snapshot from my discussion with godzillaba, which as I interpret it, by using "i believe the standard procedure" means to me that it's not 100% clear that a normal upgrade can't be done for staking requirements.

dontonka — 05/18/2024 8:10 PM
Reminder: this is a private channel.

Since EdgeChallengeManager is an upgradable contract and based on the following comment. The following secnarios are possible?
        // The contract initializer can disable staking by setting zeros for token or amount, to change
        // this a new challenge manager needs to be deployed and its address updated in the assertion chain
        if (address(st) != address(0) && sa != 0) {

Initial deployment (t0): stakeToken == address(0), stakeAmounts with valid values. So this means mini-bond free to create edges.
Upgrade One (t0 + 5 days) : stakeToken is now valid and stakeAmounts remains the same. So this means there is a mini-bond cost to create an edge.
Upgrade Two (t0 + 30 days): stakeToken remains the same, but stakeAmounts are increased as they were too low (all the levels).
Upgrade Three (t0 + 90 days): stakeToken is changed to another token (WETH --> WBTC), and stakeAmounts are adjusted downward as asset is worth more.

godzillaba — 05/19/2024 2:43 PM
this sounds possible, yes. although i believe the standard procedure for upgrades and parameter updates of the challenge manager will be to actually deploy an entirely new version and call RollupAdminLogic::setChallengeManager

Assessed type

Upgradable

[c4-sponsor]

gzeoneth (sponsor) disputed

[gzeoneth]

Invalid. Hypothetical broken upgrade is out-of-scope and all proxy upgrade have upgrade risk. Upgrading ECM directly is also not standard procedure as setChallengeManager should be used instead.

[Picodes]

This report is a speculation on a future upgrade so is invalid under C4 rules

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid