Contract fails to deliver promised returns, but doesn't lose value
Temporary freezing of NFTs
Description
Brief/Intro
The vulnerability in the VotingEscrow contract arises due to an access control flaw within the _burn function, which inappropriately handles approval resetting, requiring broader permissions than necessary. This flaw disrupts operations such as token merging and withdrawals, as transactions initiated by users who are only approved for specific token IDs (and not globally) revert unexpectedly. On mainnet, this could lead to operational disruptions, preventing users from consolidating voting power or accessing their funds post-lock period, thereby undermining user trust and the functionality of the contract. Such issues could significantly impact user engagement and the platform's overall reliability.
In VotingEscrow.sol contracts, there are multiple functions that allow whether the msg.sender is approved for the given token ID, is an operator of the owner, or is the owner of the token by utilizing _isApprovedOrOwner function.
However, two of these functions are in question in this report; merge and withdraw, since the _burn function is used.
The _burn function within the VotingEscrow contract is designed to remove tokens from circulation by clearing approvals and performing other state updates. However, due to the way access control is implemented, only the token owner or an entity approved for all user tokens can successfully execute this function without causing a revert. This is because the function internally calls approve(address(0), _tokenId), which checks for broader permissions than those granted to entities approved for a single token.
Impact Details
This results in operational disruptions for users who are legitimately authorized to perform actions (like merge and withdraw) that rely on _burn. They may encounter transaction reverts, leading to:
Inability to merge tokens for voting power consolidation or management.
Failed attempts to withdraw tokens post-lock period, leading to user dissatisfaction and potential disruption in planned economic activities within the platform.
Recommended Mitigation
Consider refactoring approval logic in _burn, Modify the internal logic of the _burn function to handle cases where the caller is only approved for the specific token ID being burned. This could involve bypassing the approval reset or adjusting the approval check to recognize and allow this scenario.
Consider standardizing the approval reset process to use _clearApproval, especially in contexts where specific token approval is sufficient. This would align the behavior across different contract functions and improve the reliability of operations involving token transfers, burns, or modifications.
src/VotingEscrow.sol:
501 function approve(address _approved, uint256 _tokenId) public {
..SNIP..
507 // Check requirements
508 bool senderIsOwner = (owner == msg.sender);
509 bool senderIsApprovedForAll = (ownerToOperators[owner])[msg.sender];
510: require(senderIsOwner || senderIsApprovedForAll, "sender is not owner or approved");
This scenario can be replicated in a test environment to demonstrate the failure mechanism and its impact on contract usability.
Test Case (Foundry)
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.15;
import "./BaseTest.sol";
contract VotingEscrowPoC is BaseTest {
uint256 internal constant THREE_WEEKS = 3 weeks;
function setUp() public {
setupContracts(block.timestamp);
}
// Withdraw reverts for token approved address
function testWithdrawReversForTokenApprovedAddress() public {
// Create address for Alice
address alice = address(0x00001);
// Create new token for Alice
uint256 tokenId = createVeAlcx(alice, TOKEN_1, THREE_WEEKS, false);
// alice start interacions
hevm.startPrank(alice);
// Reset the token status
voter.reset(tokenId);
// Finish the epoch
hevm.warp(newEpoch());
voter.distribute();
// Start cooldown once lock is expired
veALCX.startCooldown(tokenId);
// Go to the next epoch
hevm.warp(newEpoch());
// Now the token is withdrawable
// Alice approve admin address on `tokenId`
veALCX.approve(admin, tokenId);
hevm.stopPrank();
hevm.startPrank(admin);
// Make sure admin is approved for alice token
assertTrue(veALCX.isApprovedOrOwner(admin, tokenId));
// Now if Admin try to withdraw,
// the call will revert with message that suggest the caller is not owner or approved.
// While Admin is approved for the specific tokenId.
hevm.expectRevert(abi.encodePacked("sender is not owner or approved"));
veALCX.withdraw(tokenId);
// This happen because the underline function `_burn` call `approve` with zero address to clear specific token approvals,
// which revert if the caller not owner nor approved for all.
hevm.expectRevert(abi.encodePacked("sender is not owner or approved"));
veALCX.approve(address(0), tokenId);
// However, Admin can use transferFrom function and then withdraw
veALCX.transferFrom(alice, admin, tokenId);
veALCX.withdraw(tokenId);
hevm.stopPrank();
}
}
Test Output
alchemix-v2-dao main 1m38s
❯ make test_file_test
FOUNDRY_PROFILE=default forge test --fork-url https://eth-mainnet.g.alchemy.com/v2/*** --match-path src/test/VotingEscrowPoC.t.sol --match-test testWithdrawReversForTokenApprovedAddress -vv
[⠊] Compiling...
No files changed, compilation skipped
Ran 1 test for src/test/VotingEscrowPoC.t.sol:VotingEscrowPoC
[PASS] testWithdrawReversForTokenApprovedAddress() (gas: 7657391)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 90.57s (77.12s CPU time)
Ran 1 test suite in 91.85s (90.57s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Consider a scenario where a user has received specific approval for token ID 123 to facilitate a token merge or withdrawal. Under the current implementation, if this user attempts to initiate these actions, the transaction will revert during the _burn call due to the internal function not recognizing their limited approval as sufficient.
