30694 - [SC - Low] Users approved for a single token id cannot wit...

Submitted on May 5th 2024 at 00:02:52 UTC by @imsrybr0 for Boost | Alchemix

Report ID: #30694

Report type: Smart Contract

Report severity: Low

Target: https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/VotingEscrow.sol

Impacts:

• Contract fails to deliver promised returns, but doesn't lose value

Description

Brief/Intro

Users approved for a single token id cannot withdraw or merge for that token id.

Vulnerability Details

    function _isApprovedOrOwner(address _spender, uint256 _tokenId) internal view returns (bool) {
        address owner = idToOwner[_tokenId];
        bool spenderIsOwner = owner == _spender;
        bool spenderIsApproved = _spender == idToApprovals[_tokenId];
        bool spenderIsApprovedForAll = (ownerToOperators[owner])[_spender];
        return spenderIsOwner || spenderIsApproved || spenderIsApprovedForAll;
    }

    function merge(uint256 _from, uint256 _to) external {
        //...
        require(_isApprovedOrOwner(msg.sender, _from), "not approved or owner");
        require(_isApprovedOrOwner(msg.sender, _to), "not approved or owner");
        // ...

        _burn(_from, value0);
        _depositFor(_to, value0, end, _locked1.maxLockEnabled, _locked1, DepositType.MERGE_TYPE);
    }

    function withdraw(uint256 _tokenId) public nonreentrant {
        require(_isApprovedOrOwner(msg.sender, _tokenId), "not approved or owner");

       // ...

        _burn(_tokenId, value);

        emit Withdraw(msg.sender, _tokenId, value, block.timestamp);
    }

    function _burn(uint256 _tokenId, uint256 _value) internal {
        // ...
        approve(address(0), _tokenId);
        // ...
    }

    function approve(address _approved, uint256 _tokenId) public {
        address owner = idToOwner[_tokenId];
        // Throws if `_tokenId` is not a valid token
        require(owner != address(0), "owner not found");
        // Throws if `_approved` is the current owner
        require(_approved != owner, "Approved is already owner");
        // Check requirements
        bool senderIsOwner = (owner == msg.sender);
        bool senderIsApprovedForAll = (ownerToOperators[owner])[msg.sender];
        require(senderIsOwner || senderIsApprovedForAll, "sender is not owner or approved");
        // Set the approval
        idToApprovals[_tokenId] = _approved;
        emit Approval(owner, _approved, _tokenId);
    }

Both withdraw and merge check if the msg.sender is owner of the given token id or is approved to use it (either for all tokens from the same owner of specifically the one being used).

They will also burn the token after carrying on their logic and clear its approvals.

Approvals are cleared using approve(address(0), _tokenId) which will fail if a msg.sender is only approved for that token id specifically.

Impact Details

• User approved for a single token cannot withdraw or merge.

• Users need to give permission for all their tokens to another user when they want that user to carry withdraw or merge operations for them.

Recommendation

Use the _clearApproval(owner, _tokenId) to clear the approvals in the _burn function.

References

• https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/VotingEscrow.sol#L741-L775

• https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/VotingEscrow.sol#L618-L651

• https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/VotingEscrow.sol#L826-L832

• https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/VotingEscrow.sol#L501-L514

• https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/VotingEscrow.sol#L1558-L1574

Proof of Concept

    function testApproveSingleAndMerge() public {
        uint256 tokenId1 = createVeAlcx(admin, TOKEN_1, MAXTIME, false);
        uint256 tokenId2 = createVeAlcx(beef, TOKEN_1, MAXTIME, false);

        hevm.prank(admin);
        veALCX.approve(beef, tokenId1);

        hevm.prank(beef);
        veALCX.merge(tokenId1, tokenId2);

        assertEq(veALCX.lockedAmount(tokenId2), TOKEN_1 + TOKEN_1);
    }