Revolution Protocol - Brenzee's results

Lines of code

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

Vulnerability details

Impact

Token quote calculations will be incorrect because of inflated bookkeeping of emittedTokenWad

Proof of Concept

ERC20TokenEmitter.sol contract uses vrgdac.yToX function to calculate token quote for ETH and vrgdac.xToY to calculate token quote for payment. Both of these functions rely on emittedTokenWad.

    function getTokenQuoteForEther(uint256 etherAmount) public view returns (int gainedX) {
        ... 
        return
            vrgdac.yToX({
                timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime),
                sold: emittedTokenWad,
                amount: int(etherAmount)
            });
    }

    function getTokenQuoteForPayment(uint256 paymentAmount) external view returns (int gainedX) {
        ...
        return
            vrgdac.yToX({
                timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime),
                sold: emittedTokenWad,
                amount: int(((paymentAmount - computeTotalReward(paymentAmount)) * (10_000 - creatorRateBps)) / 10_000)
            });
    }

emittedTokenWad is the value of how many ERC20 vote tokens are minted. emittedTokenWad is incremented in ERC20TokenEmitter.buyToken function - this value is incremented by the amount of tokens that are minted for buyers and if totalTokensForCreators > 0 is true, it is also incremented by the amount of tokens that would have been minted for the creators:

        emittedTokenWad += totalTokensForBuyers;
        if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;

But the issue is that if totalTokensForCreators > 0 is true, it does not always mean that the tokens for creators will be minted, because of the "if" statement line 201:

    if (totalTokensForCreators > 0 && creatorsAddress != address(0)) {
        _mint(creatorsAddress, uint256(totalTokensForCreators));
    }

Meaning that if the totalTokensForCreators > 0 and creatorsAddress === address(0), no tokens will be minted for the creators, but emittedTokenWad will still be incremented by the amount of tokens that would have been minted for the creators, which will cause incorrect calculations of token quotes.

Tools Used

Manual Review

Recommended Mitigation Steps

Move the increment of emittedTokenWad in the "if" statement where the tokens are minted for creators. This way emittedTokenWad will only be incremented if the tokens are actually minted.

    if (totalTokensForCreators > 0 && creatorsAddress != address(0)) {
        emittedTokenWad += totalTokensForCreators;
        _mint(creatorsAddress, uint256(totalTokensForCreators));
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L383-L396

Vulnerability details

Impact

Auction will never be settled if protocol decides that the creators will only receive ERC20 votes and no ETH. (entropyRateBps = 0)

Proof of Concept

When auction is settled, creators of the art piece get share from the auction amount as rewards. There are 2 type of rewards:

1. ERC20 vote tokens
2. ETH

Protocol can control the split between those rewards by setting entropyRateBps value. The closer the entropyRateBps value is to 0, the more ERC20 vote tokens creators will receive and the less ETH they will receive and vice versa. If entropyRateBps = 0, creators will only receive ERC20 vote tokens and no ETH.

ERC20 vote tokens for creators are minted via erc20TokenEmitter.buyToken function.

    if (creatorsShare > ethPaidToCreators) {
        creatorTokensEmitted = erc20TokenEmitter.buyToken{ value: creatorsShare - ethPaidToCreators }(
            vrgdaReceivers,
            vrgdaSplits,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: deployer
            })
        );
    }

The issue is that the arrays vrgdaSplits and vrgdaReceivers that are required for erc20TokenEmitter.buyToken function are not set if entropyRateBps = 0. Line 383

    //Build arrays for erc20TokenEmitter.buyToken
    uint256[] memory vrgdaSplits = new uint256[](numCreators);
    address[] memory vrgdaReceivers = new address[](numCreators);

    ...

    //Transfer creator's share to the creator, for each creator, and build arrays for erc20TokenEmitter.buyToken
    if (creatorsShare > 0 && entropyRateBps > 0) {
        for (uint256 i = 0; i < numCreators; i++) {
            ICultureIndex.CreatorBps memory creator = verbs.getArtPieceById(_auction.verbId).creators[i];
            vrgdaReceivers[i] = creator.creator;
            vrgdaSplits[i] = creator.bps;

            ...
        }
    }

This will cause the erc20TokenEmitter.buyToken function to fail, because the function requires vrgdaSplits to add up to 100% (10_000).

    for (uint256 i = 0; i < addresses.length; i++) {
        if (totalTokensForBuyers > 0) {
            // transfer tokens to address
            _mint(addresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000));
        }
        bpsSum += basisPointSplits[i];
    }

    require(bpsSum == 10_000, "bps must add up to 10_000");

As a result, the auction will never be settled if protocol decides that the creators will only receive ERC20 votes as rewards.

Tools Used

Manual review

Recommended Mitigation Steps

Update the code to the following:

    if (creatorsShare > 0) {
        for (uint256 i = 0; i < numCreators; i++) {
            ICultureIndex.CreatorBps memory creator = verbs.getArtPieceById(_auction.verbId).creators[i];
            vrgdaReceivers[i] = creator.creator;
            vrgdaSplits[i] = creator.bps;

            if (entropyRateBps == 0) continue;

            //Calculate paymentAmount for specific creator based on BPS splits - same as multiplying by creatorDirectPayment
            uint256 paymentAmount = (creatorsShare * entropyRateBps * creator.bps) / (10_000 * 10_000);
            ethPaidToCreators += paymentAmount;

            _safeTransferETHWithFallback(creator.creator, paymentAmount);
        }
    }

This will ensure that the auction can be settled with entropyRateBps = 0.

Assessed type

Context