sherlock-audit / 2024-06-boost-aa-wallet-judging

3 stars 1 forks source link

ZanyBonzy - Users can claim multiple times #441

Open sherlock-admin4 opened 2 months ago

sherlock-admin4 commented 2 months ago

ZanyBonzy

Medium

Users can claim multiple times

Summary

The claimed mapping is not updated for users in CGDAIncentive.sol, and ERC20VariableIncentive.sol as a result users can claim multiple times without repercussions.

Vulnerability Detail

These contracts define the _isClaimable function which checks that the recipient has not claimed and claims are less than the limit.

Using CGDAIncentive.sol as an example.

    function _isClaimable(address recipient_) internal view returns (bool) {
        uint256 reward = currentReward();
        return reward > 0 && asset.balanceOf(address(this)) >= reward && !claimed[recipient_];
    }

However, this mapping is not updated when the users claim, only the number of claims is increased.

    function claim(address claimTarget, bytes calldata) external virtual override onlyOwner returns (bool) {
        if (!_isClaimable(claimTarget)) revert NotClaimable();
        claims++;

        // Calculate the current reward and update the state
        uint256 reward = currentReward();
        cgdaParams.lastClaimTime = block.timestamp;
        cgdaParams.currentReward =
            reward > cgdaParams.rewardDecay ? reward - cgdaParams.rewardDecay : cgdaParams.rewardDecay;

        // Transfer the reward to the recipient
        asset.safeTransfer(claimTarget, reward);

        emit Claimed(claimTarget, abi.encodePacked(asset, claimTarget, reward));
        return true;
    }

As a result, a user can theoretically claim as much as the limit bypassing the check (albeit there's a time limit).

The same can be observed in ERC20VariableIncentive.sol which doesn't even enforce that a user should only be able to claim once in its _isClaimable function. As a result, a user can claim as much as he wants, up to the limit.

A small test for this is to add the snippet below to ERC20VariableIncentive.t.sol and run with the command forge test --mt test_doubleClaim -vvvv. It should pass.

    function test_doubleClaim() public {
        _initialize(address(mockAsset), 1 ether, 5 ether);

        // First claim
        incentive.claim(CLAIM_RECIPIENT, _encodeBoostClaim(1 ether));

        assertEq(mockAsset.balanceOf(CLAIM_RECIPIENT), 1 ether);
        assertTrue(incentive.isClaimable(CLAIM_RECIPIENT, abi.encode(1 ether)));

        // Second claim
        incentive.claim(CLAIM_RECIPIENT, _encodeBoostClaim(1 ether));

        assertEq(mockAsset.balanceOf(CLAIM_RECIPIENT), 2 ether);
        assertTrue(incentive.isClaimable(CLAIM_RECIPIENT, abi.encode(2 ether)));

        // And so on
    }

Impact

Multple claimings at the expense of other users and bypassing the _isClaimable function check.

Code Snippet

https://github.com/sherlock-audit/2024-06-boost-aa-wallet/blob/78930f2ed6570f30e356b5529bd4bcbe5194eb8b/boost-protocol/packages/evm/contracts/incentives/ERC20VariableIncentive.sol#L62

https://github.com/sherlock-audit/2024-06-boost-aa-wallet/blob/78930f2ed6570f30e356b5529bd4bcbe5194eb8b/boost-protocol/packages/evm/contracts/incentives/ERC20VariableIncentive.sol#L93

https://github.com/sherlock-audit/2024-06-boost-aa-wallet/blob/78930f2ed6570f30e356b5529bd4bcbe5194eb8b/boost-protocol/packages/evm/contracts/incentives/CGDAIncentive.sol#L133

https://github.com/sherlock-audit/2024-06-boost-aa-wallet/blob/78930f2ed6570f30e356b5529bd4bcbe5194eb8b/boost-protocol/packages/evm/contracts/incentives/CGDAIncentive.sol#L85

Tool used

Manual Code Review

Recommendation

Recommend introducing checks to prevent multiple claims in both contracts.