This report details a vulnerability discovered in the VotingEscrow.sol contract of the Alchemix V2 DAO. The issue arises when users withdraw their locked veALCX tokens. During this process, unclaimed rewards are intended to be claimed, but the system fails to account for the potential bribes earned through voting interactions. Consequently, users who withdraw their veALCX tokens lose their right to claim these bribes, leading to permanent loss of rewards.
Vulnerability Details
When a user withdraws their locked veALCX tokens (interacting with VotingEscrow::withdraw), the contract ensures that any unclaimed ALCX rewards and FLUX are claimed before the token is burned, resulting in the user losing their control over the token as they are no longer its owner.
src/VotingEscrow.sol:741:functionwithdraw(uint256_tokenId) publicnonreentrant {..SNIP..@>767:// Claim any unclaimed ALCX rewards and FLUX768:IRewardsDistributor(distributor).claim(_tokenId,false);769:IFluxToken(FLUX).claimFlux(_tokenId,IFluxToken(FLUX).getUnclaimedFlux(_tokenId));770:771:// Burn the token@>772:_burn(_tokenId, value);773:774:emitWithdraw(msg.sender, _tokenId, value, block.timestamp);775: }..SNIP..1558:function_burn(uint256_tokenId,uint256_value) internal {1559:address owner =ownerOf(_tokenId);..SNIP..1570:// Remove token@>1571:_removeTokenFrom(owner, _tokenId);1572:emitTransfer(owner,address(0), _tokenId);1573:emitSupply(supplyBefore, supplyAfter);1574: }
While this procedure is generally acceptable, an issue arises when the user has interacted with the Voter contract and voted for pools (the user may have used their FLUX to boost their votes). The bribes earned from these votes will be lost if the user withdraws their token and subsequently attempts to claim their bribes. This is because Voter::claimBribes checks the ownership status of the token, and after the token is burned, the user is no longer considered its owner, preventing them from claiming their rewards.
src/Voter.sol:332:functionclaimBribes(address[] memory_bribes,address[][] memory_tokens,uint256_tokenId) external {@>333:require(IVotingEscrow(veALCX).isApprovedOrOwner(msg.sender, _tokenId));334:335:for (uint256 i =0; i < _bribes.length; i++) {336:IBribe(_bribes[i]).getRewardForOwner(_tokenId, _tokens[i]);337: }338: }
Impact Details
The primary impact of this vulnerability is the permanent loss of bribes for users who withdraw their veALCX tokens. This occurs because the ownership check in the Voter::claimBribes function fails after the token is burned. Consequently, users are unable to claim rewards they have rightfully earned, leading to dissatisfaction and potential loss of trust in the protocol.
Mitigation Analysis
To mitigate this issue, it is recommended to enhance the withdrawal process to ensure that there are no unclaimed bribes before allowing the token to be burned. Here are a few suggested approaches:
Prevent Withdrawal if Bribes are Unclaimed: Implement a check in the VotingEscrow::withdraw function to prevent the withdrawal if there are unclaimed bribes. This ensures that users must claim their bribes before they can withdraw and burn their veALCX tokens.
Force Bribe Claiming During Withdrawal: Similar to how unclaimed ALCX rewards and FLUX are claimed during withdrawal, modify the withdrawal process to enforce the claiming of any unclaimed bribes. This would involve adding logic to claim bribes within the withdraw function, ensuring users receive all due rewards before their token is burned.
New Restriction of ClaimBribes Function: A new layer of security replaces the current check, could involve restricting who can call the Voter::claimBribes function to ensure that only valid claims are processed. However, this might be less effective than ensuring bribes are claimed during the withdrawal process. Also, it may has some drawbacks and establish a new way to manipulate the flow of voter contract specialty for cases like this issue where the veALCX is burned or ended (I remember proofing a vulnerability and it was prevent by the check of ownabilty of the token in claimBribes).
The test can be added to a new file under the current test suite src/test/VotingPoC.t.sol, then specify the file name in FILE flag under Makefile configuration. Run using make test_file
// SPDX-License-Identifier: UNLICENSEDpragmasolidity ^0.8.15;import"./BaseTest.sol";contractVotingPoCTestisBaseTest {addresspublic alice;addresspublic bob;functionsetUp() public {setupContracts(block.timestamp);// Setup Alice address alice = hevm.addr(uint256(keccak256(abi.encodePacked('Alice')))); vm.label(alice,'Alice'); }functiontestParmentFreesingOfBribesAfterWithdrowLocks() public {uint256 firstPeriodStart = minter.activePeriod();// Forwards 1 day from the begining of the current epoch. hevm.warp(firstPeriodStart +1days);// Mint new veALCX token for Alice with 1e18 amount and 3 weeks locks time.uint256 tokenId =createVeAlcx(alice, TOKEN_1,3weeks,false);address bribeAddress = voter.bribes(address(sushiGauge));address[] memory pools =newaddress[](1); pools[0] = sushiPoolAddress;uint256[] memory weights =newuint256[](1); weights[0] =10000;address[] memory bribes =newaddress[](1); bribes[0] =address(bribeAddress);address[][] memory tokens =newaddress[][](1); tokens[0] =newaddress[](1); tokens[0][0] = bal;// Alice Vote hevm.prank(alice); voter.vote(tokenId, pools, weights,0);// Reward amountuint256 rewardAmount = TOKEN_100K;// Notify bribe for reward amountcreateThirdPartyBribe(bribeAddress, bal, rewardAmount);// Next epoch started hevm.warp(firstPeriodStart +2weeks+1seconds ); voter.distribute();// Check Alice has earned a rewardassertEq(IBribe(bribeAddress).earned(bal, tokenId), rewardAmount);// Forwards to the end of Alice's veALCX locks end hevm.warp(firstPeriodStart +3weeks+1seconds );// Check that Alice's veALCX had endedassertGt(block.timestamp, veALCX.lockEnd(tokenId));// Alice decided to withdraw his locks// First he need to reset his vote statues hevm.prank(alice); voter.reset(tokenId);// Then, start cooldown of his veALCX hevm.prank(alice); veALCX.startCooldown(tokenId);// Now he should wait for 1 week untill cooldowd ends to be able to withdraw hevm.warp(firstPeriodStart +4weeks+1seconds);// Since we enter the next epoch, distribute (not needed but it will called in real world scenario) voter.distribute();// Save the earned bribes before withdrawinguint256 bribesErnedBeforeWithdraw =IBribe(bribeAddress).earned(bal, tokenId);// Check that the rewards equal to the first epoch reward amount.assertEq(bribesErnedBeforeWithdraw, rewardAmount);// Now Alice's veALCX is withdrawable at current moment. hevm.prank(alice); veALCX.withdraw(tokenId);// Compare the reward earned before and after withdrawing.uint256 bribesErnedAfterWithdraw =IBribe(bribeAddress).earned(bal, tokenId);assertEq(bribesErnedBeforeWithdraw, bribesErnedAfterWithdraw);// Make sure that the current reward earnd is more than zeroassertGt(bribesErnedAfterWithdraw,0);// Now if Alice try to claim his bribes, it will revert. hevm.prank(alice); hevm.expectRevert(); voter.claimBribes(bribes, tokens, tokenId);// This happning because after withdrawing he lost his rights on the veALCX token, since it will be burned.// Here we check that he is no longer the owner of the token.assertFalse(veALCX.isApprovedOrOwner(alice, tokenId));// While this is not an issue, he should lose the rights of controling the token after withdrawing his locks,// but the issue is that he lost his bribes. }}