Revolution Protocol - zhaojie's results

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L226 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L313

Vulnerability details

Impact

In CultureIndex, malicious users buy governance tokens immediately after createPiece to allow voting to pass early, because quorumVotes are lower than expected.

Proof of Concept

quorumVotes are calculated using the totalVotesSupply of governance tokens:

    function createPiece(
        ArtPieceMetadata calldata metadata,
        CreatorBps[] calldata creatorArray
    ) public returns (uint256) {
        .....
        newPiece.creationBlock = block.number;
        newPiece.totalVotesSupply = _calculateVoteWeight(
            erc20VotingToken.totalSupply(),
            erc721VotingToken.totalSupply()
        );
        ....
        newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000;
    }

If the malicious user mint token after createPiece, totalSupply has actually increased, but the quorumVotes calculated previously still use the old value, so quorumVotes are lower than they actually are.

A malicious user can create piece in the same block first and then buy tokens, so that his Piece will be more advantageous, In the early days of the protocol's launch, when totalSupply is low, malicious users can mint tokens that exceed the threshold of quorumVotes and can immediately pass the vote.

When voting, the BlockNumber used at the time of piece creation is used to obtain a snapshot of voting power, so minted's token(voting power) after createPiece in the same block can be used for voting.

    function _vote(uint256 pieceId, address voter) internal {
        require(pieceId < _currentPieceId, "Invalid piece ID");
        require(voter != address(0), "Invalid voter address");
        require(!pieces[pieceId].isDropped, "Piece has already been dropped");
        require(!(votes[pieceId][voter].voterAddress != address(0)), "Already voted");

@>      uint256 weight = _getPastVotes(voter, pieces[pieceId].creationBlock);
        require(weight > minVoteWeight, "Weight must be greater than minVoteWeight");

        votes[pieceId][voter] = Vote(voter, weight);
        totalVoteWeights[pieceId] += weight;

        uint256 totalWeight = totalVoteWeights[pieceId];

        maxHeap.updateValue(pieceId, totalWeight);
        emit VoteCast(pieceId, voter, weight, totalWeight);
    }

Tools Used

vscode manual

Recommended Mitigation Steps

    function _vote(uint256 pieceId, address voter) internal {
        .....
-        uint256 weight = _getPastVotes(voter, pieces[pieceId].creationBlock);
+        uint256 weight = _getPastVotes(voter, pieces[pieceId].creationBlock - 1);
    }

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L152

Vulnerability details

Impact

ERC20TokenEmitter#buyToken lacks slippage protection, users may pay a higher price than expected.

Proof of Concept

buyToken uses variable price (VRGDAC), when the total supply increases, the price is likely to increase, especially when the threshold is reached, the price will increase very quickly (according to VRGDAC).

For example, after the user initiates a transaction, a large transaction is inserted in front of the total supply increases a lot, because the large transaction is executed before the user's transaction, so the number of tokens obtained by the user will be far less than the expected value, which is the user does not want to see.

Therefore, it is necessary to set a minimum value when purchasing tokens. If the price changes cause the purchased tokens to be less than this minimum value, the transaction will be cancelled.

Tools Used

vscode manual

Recommended Mitigation Steps

add slippage protection

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L258

Vulnerability details

Impact

The timeSinceStart in the vrgdac.xToY function will revert over a certain value, resulting in the ERC20TokenEmitter#buyToken function always revert.

Proof of Concept

Initialize the VRGDAC using the parameters in the test code.

    VRGDAC vrgdac = new VRGDAC(1 ether, 1e18 / 10, 1_000 * 1e18);

The timeSinceStart is set to 394 days in the test code:

    function testVRGDAC_time() public {
        VRGDAC vrgdac = new VRGDAC(1 ether, 1e18 / 10, 1_000 * 1e18);
        int256 x = vrgdac.yToX({
            timeSinceStart: toDaysWadUnsafe(86400 * 400),
            sold: 1000 ether,
            amount: 1 ether
        });
        uint256 xx = uint256(x);
        console.log(xx + 1);
        console.log(xx / 1e18);
    }

Run the forge test -vvvv, console to output:

[FAIL. Reason: UNDEFINED] testVRGDAC_time() (gas: 554525)
Traces:
  [106719] CounterTest::setUp() 
    ├─ [49499] → new Counter@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   └─ ← 247 bytes of code
    ├─ [2390] Counter::setNumber(0) 
    │   └─ ← ()
    └─ ← ()

  [554525] CounterTest::testVRGDAC_time() 
    ├─ [517512] → new VRGDAC@0x2e234DAe75C793f67A35089C9d99245E1C58470b
    │   └─ ← 2578 bytes of code
    ├─ [3617] VRGDAC::yToX(400000000000000000000, 1000000000000000000000, 1000000000000000000) [staticcall]
    │   └─ ← "UNDEFINED"
    └─ ← "UNDEFINED"

Changing the timeSinceStart to toDaysWadUnsafe(86400 * 365) will work.

When this function is used in ERC20TokenEmitter#buyToken, the timeSinceStart is: block.timestamp-startTime

    function buyTokenQuote(uint256 amount) public view returns (int spentY) {
        require(amount > 0, "Amount must be greater than 0");
        return
            vrgdac.xToY({
                timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime),
                sold: emittedTokenWad,
                amount: int(amount)
            });
    }

startTime is set during the initialization of the ERC20TokenEmitter contract:

    function initialize(
        address _initialOwner,
        address _erc20Token,
        address _treasury,
        address _vrgdac,
        address _creatorsAddress
    ) external initializer {
        .....
        startTime = block.timestamp;
    }

In other words, if the ERC20TokenEmitter contract is deployed and initialized and becomes unavailable after 400 days(400 days is the test value, the actual value will be affected by other parameters), calling the buyToken function will always revert.

Tools Used

vscode manual

Recommended Mitigation Steps

Optimize the vrgdac.yToX function, or set a minimum timeSinceStart value, which is used when the minimum is exceeded.

Assessed type

Math