Canto Liquidity Mining Protocol - HChang26's results

Lines of code

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/callpaths/LiquidityMiningPath.sol#L26

Vulnerability details

Impact

protocolCmd() will brick as it's not marked payable.

Proof of Concept

protocolCmd() is not marked as payable.

    function protocolCmd(bytes calldata cmd) public virtual {
        (uint8 code, bytes32 poolHash, uint32 weekFrom, uint32 weekTo, uint64 weeklyReward) =
            abi.decode(cmd, (uint8, bytes32, uint32, uint32, uint64));

        if (code == ProtocolCmd.SET_CONC_REWARDS_CODE) {
            setConcRewards(poolHash, weekFrom, weekTo, weeklyReward);
        } else if (code == ProtocolCmd.SET_AMB_REWARDS_CODE) {
            setAmbRewards(poolHash, weekFrom, weekTo, weeklyReward);
        } else {
            revert("Invalid protocol command");
        }
    }

Proxy contracts often rely on delegateCall or similar mechanisms to forward function calls and data to an underlying implementation contract. When a function call is forwarded, it's executed in the context of the proxy contract. If the function in the implementation contract is marked as payable, it can accept Ether (via msg.value) if the forwarded function call contains any Ether. If the function in the proxy contract is not marked as payable, forwarding a call to a payable function in the implementation contract would fail, even if the implementation contract is capable of handling Ether.

So, marking proxy entry points as payable ensures that the proxy contract can forward function calls, including those that may involve Ether, to the implementation contract without issues. It's a defensive measure to ensure that Ether transfers are properly handled by the underlying contract, even if the proxy itself doesn't deal with Ether directly.

Tools Used

Manual Review

Recommended Mitigation Steps

-   function protocolCmd(bytes calldata cmd) public virtual {
+   function protocolCmd(bytes calldata cmd) public payable virtual {
        (uint8 code, bytes32 poolHash, uint32 weekFrom, uint32 weekTo, uint64 weeklyReward) =
            abi.decode(cmd, (uint8, bytes32, uint32, uint32, uint64));

        if (code == ProtocolCmd.SET_CONC_REWARDS_CODE) {
            setConcRewards(poolHash, weekFrom, weekTo, weeklyReward);
        } else if (code == ProtocolCmd.SET_AMB_REWARDS_CODE) {
            setAmbRewards(poolHash, weekFrom, weekTo, weeklyReward);
        } else {
            revert("Invalid protocol command");
        }
    }

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L256

Vulnerability details

Impact

claimAmbientRewards() can expectedly revert when called at certain block.timestamp.

Proof of Concept

In the claimAmbientRewards() function, there's an issue related to how it checks and handles the weeks for which users can claim rewards. Users can specify which weeks they want to claim rewards for, and the function validates these selections. Specifically, it checks if the selected weeks have completed(in the past), using the condition week + WEEK < block.timestamp.

            require(week + WEEK < block.timestamp, "Week not over yet");
            require(
                !ambLiquidityRewardsClaimed_[poolIdx][posKey][week],
                "Already claimed"
            );

In most situations, this check works correctly. However, in certain edge cases, it can unexpectedly cause the function to revert. The problem arises when block.timestamp aligns exactly with the end of a week. Even though the week is technically completed, the user is unable to claim rewards due to the require statement.

Imagine following scenario:

1. A user has provided liquidity from week 0 to week 3 and intends to claim rewards from all 3 weeks.
2. The user calls claimAmbientRewards() precisely at the end of week 3/start of week 4, where block.timestamp = 1,814,400.
3. All rewards for week 3 have been fully accrued in the accrueAmbientPositionTimeWeightedLiquidity() and accrueAmbientGlobalTimeWeightedLiquidity() functions.
4. Despite week 3 being completed, the call reverts due to the condition week + WEEK < block.timestamp.

Tools Used

Manual Review

Recommended Mitigation Steps

    function claimAmbientRewards(
        address owner,
        bytes32 poolIdx,
        uint32[] memory weeksToClaim
    ) internal {
        CurveMath.CurveState memory curve = curves_[poolIdx];
        accrueAmbientPositionTimeWeightedLiquidity(payable(owner), poolIdx);
        accrueAmbientGlobalTimeWeightedLiquidity(poolIdx, curve);
        bytes32 posKey = encodePosKey(owner, poolIdx);
        uint256 rewardsToSend;
        for (uint256 i; i < weeksToClaim.length; ++i) {
            uint32 week = weeksToClaim[i];
-           require(week + WEEK < block.timestamp, "Week not over yet");
+           require(week + WEEK <= block.timestamp, "Week not over yet");
            require(
                !ambLiquidityRewardsClaimed_[poolIdx][posKey][week],
                "Already claimed"
            );
            uint256 overallTimeWeightedLiquidity = timeWeightedWeeklyGlobalAmbLiquidity_[
                    poolIdx
                ][week];
            if (overallTimeWeightedLiquidity > 0) {
                uint256 rewardsForWeek = (timeWeightedWeeklyPositionAmbLiquidity_[
                    poolIdx
                ][posKey][week] * ambRewardPerWeek_[poolIdx][week]) /
                    overallTimeWeightedLiquidity;
                rewardsToSend += rewardsForWeek;
            }
            ambLiquidityRewardsClaimed_[poolIdx][posKey][week] = true;
        }
        if (rewardsToSend > 0) {
            (bool sent, ) = owner.call{value: rewardsToSend}("");
            require(sent, "Sending rewards failed");
        }
    }

Assessed type

Invalid Validation