Revolution Protocol - 0xDING99YA's results

Lines of code

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

Vulnerability details

Impact

When buyToken(), the msg.value can be split into 4 ways. 1). Go to protocolRewardsRecipients by _handleRewardsAndGetValueToSend() 2). Be used to mint ERC20 to buyer (and go to treasury) 3). Be used to mint ERC20 to creators (and go to treasury) 4). Directly go to creators However, the function fails to do the third point, it only mint ERC20 token to creators and do not pay that amount to treasury, as a result, that amount will be stuck in the contract forever.

Proof of Concept

Assuming the buyer pays msg.value, the first part will go to protocolRewardsRecipients by _handleRewardsAndGetValueToSend().

uint256 msgValueRemaining = _handleRewardsAndGetValueToSend(
            msg.value,
            protocolRewardsRecipients.builder,
            protocolRewardsRecipients.purchaseReferral,
            protocolRewardsRecipients.deployer
        );

The second part of msg.value will go to treasury

(bool success, ) = treasury.call{ value: toPayTreasury }(new bytes(0));
        require(success, "Transfer failed.");

The third pard go to the creators (amount paid directly to creators)

if (creatorDirectPayment > 0) {
            (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0));
            require(success, "Transfer failed.");
        }

As we can see (msgValueRemaining - toPayTreasury - creatorDirectPayment) amount of Ether will stuck in the contract. This is because buyToken() only mint the ERC20 token for creators but forget to transfer that amount of Ether to treasury. Basically, when minting the token, treasury should receive all the Ether. In this case, we can see it in this way, the user pays the Ether to treasury for all ERC20 tokens, part of the tokens are for himself and the other are for creators.

Tools Used

Manual Review

Recommended Mitigation Steps

Transfer the Ether used to mint ERC20 tokens for creators to treasury.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/base/VotesUpgradeable.sol#L164-L167 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/base/VotesUpgradeable.sol#L172-L175 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/base/VotesUpgradeable.sol#L180-L199 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/base/VotesUpgradeable.sol#L233-L253

Vulnerability details

Impact

VerbsToken is a ERC721 token designed so that the owner can have votes and delegate their votes. Under current implementation owner can delegate the votes to address(0), however, in this way they will lose their votes and in the future when they want to transfer the token it will revert.

Proof of Concept

VerbsToken inherits from ERC721CheckpointableUpgradeable, and ERC721CheckpointableUpgradeable inherits from VotesUpgradeable. Within VotesUpgradeable, user can delegate votes by delegate() and delegateBySig(), both of these functions allow delegate to address(0) and will then call _delegate()

    function delegate(address delegatee) public virtual {
        address account = _msgSender();
        _delegate(account, delegatee);
    }

function delegateBySig(
        address delegatee,
        uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual {
        if (block.timestamp > expiry) {
            revert VotesExpiredSignature(expiry);
        }
        address signer = ECDSA.recover(
            _hashTypedDataV4(keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry))),
            v,
            r,
            s
        );
        _useCheckedNonce(signer, nonce);
        _delegate(signer, delegatee);
    }

Within _delegate(), it will just set the new delegates and move the votes:

function _delegate(address account, address delegatee) internal virtual {
        VotesStorage storage $ = _getVotesStorage();
        address oldDelegate = delegates(account);
        $._delegatee[account] = delegatee;

        emit DelegateChanged(account, oldDelegate, delegatee);
        _moveDelegateVotes(oldDelegate, delegatee, _getVotingUnits(account));
    }

Let's say Alice delegate all her votes to address(0), when she wants to delegate to other address it will revert, this is because in _delegate(), when it trying to get Alice's delegatee, if the delegatee is address(0) it will always return Alice's address:

function delegates(address account) public view virtual returns (address) {
        VotesStorage storage $ = _getVotesStorage();
        return $._delegatee[account] == address(0) ? account : $._delegatee[account];
    }

Since Alice's votes is zero now, the transaction will revert because of overflow and Alice cannot delegate to others.

It will also block transferring the token since the same logic is used during the transfer.

Note VotesUpgradeable is out of scope but VerbsToken.sol which inherits from it is in the scope, so it should be a valid finding.

Tools Used

Manual Review

Recommended Mitigation Steps

Don't allow owners to delegate their votes to address(0).

Assessed type

Governance

Lines of code

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

Vulnerability details

Impact

In _settleAuction(), it will determine if address(this).balance is larger than reservePrice, if so and if the bidder is not zero address it will distribute the _auction.amount to owner and creators accordingly. However, the address(this).balance is not always equal to _auction.amount, so _auction.amount can less than reservePrice while address(this).balance is larger than, and as a result the auction can still be settled and creators/owners will get less than they should.

Proof of Concept

In _settleAuction(), if address(this).balance >= reserPrice and _auction.amoint > 0 , the auction can settle and _auction.amount will be distributed accordingly.

if (address(this).balance < reservePrice) {
            // If contract balance is less than reserve price, refund to the last bidder
            if (_auction.bidder != address(0)) {
                _safeTransferETHWithFallback(_auction.bidder, _auction.amount);
            }

            // And then burn the Noun
            verbs.burn(_auction.verbId);
        } else {
            //If no one has bid, burn the Verb
            if (_auction.bidder == address(0))
                verbs.burn(_auction.verbId);
                //If someone has bid, transfer the Verb to the winning bidder
            else verbs.transferFrom(address(this), _auction.bidder, _auction.verbId);

            if (_auction.amount > 0) {
                // Ether going to owner of the auction
                uint256 auctioneerPayment = (_auction.amount * (10_000 - creatorRateBps)) / 10_000;

                //Total amount of ether going to creator
                uint256 creatorsShare = _auction.amount - auctioneerPayment;

However, it's very easy to make address(this).balance >= reservePrice, like by self destruct and force send Ether to this contract address, so _auction.amount can still less than reservePrice, in this case the auction shouldn't be settle but it will settle now, and creators and owner will get less token.

Tools Used

Manual Review

Recommended Mitigation Steps

Don't use address(this).balance to compare to the reservePrice, use _auction.amount

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Within the buyToken(), this function will use the msg.value to calculate the tokens that the receivers should get. However, there is no mechanism to provide a slippage control, as a result, the buyer can get much less token than they thought and suffer a loss.

Proof of Concept

Within buyToken(), it will use getTokenQuoteForEther() to evaluate how much token the buyer can get. getTokenQuoteForEther() will use VRGDAC to do the computation, which means if the sold token amount is larger than the expected sold amount at that time, the price will increase and vice versa. The issue here is, buyer has no way to specify the minimum token amount they need to receive, so no slippage control.

Here is the scenario, Alice wants to buy some token, she called getTokenQuoteForPayment() with the amount of Ether she wants to pay and get the token she can obtain, then she call buyToken() with the same Ether amount. When waiting for the transaction to be confirmed, another buyer buys lots of token before Alice's purchase, so the sold amount of token is much larger than expected amount, and the token price increase a lot. When Alice's purchase get processed, the latest high price is used, so Alice get much less token than she expected and suffer a loss.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a uint256 parameter to the buyToken() function so that the buyer can specify the minimum amount they need to receive, if not, revert the transaction.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

extractMax() is implemented incorrectly and can break the entire max heap.

Proof of Concept

When extractMax(), it just replace the top item with the last item and then do a maxHeapify():

function extractMax() external onlyAdmin returns (uint256, uint256) {
        require(size > 0, "Heap is empty");

        uint256 popped = heap[0];
        heap[0] = heap[--size];
        maxHeapify(0);

        return (popped, valueMapping[popped]);
    }

Here is a scenario can be exploited. pos -- 0; pieceId -- 0; value -- 10; pos -- 1; pieceId -- 1; value -- 5; pos -- 2; pieceId -- 2; value -- 5; pos -- 3; pieceId -- 3; value -- 5; pos -- 4; pieceId -- 4; value -- 5; size = 5;

We call extractMax(), pieceId 0 will be extracted, pos 0 will be pieceId 4 now and maxHeapify(0) won't make any change because pos 0 has the same value with pos 1 and 2. Now the heap looks like this:

pos -- 0; pieceId -- 4; value -- 5; pos -- 1; pieceId -- 1; value -- 5; pos -- 2; pieceId -- 2; value -- 5; pos -- 3; pieceId -- 3; value -- 5; size = 4; pos -- 4; pieceId -- 4; value -- 5;

Assuming now call updateValue(1, 3), we update the pieced 1 and change it value from 5 to 3. left pos = 3 with value of 5, right pos = 4 with value of 5 (because after extractMax() heap[4] is still 4), now swap(1, 4) will be called, the heap will looks like this:

pos -- 0; pieceId -- 4; value -- 5; pos -- 1; pieceId -- 4; value -- 5; pos -- 2; pieceId -- 2; value -- 5; pos -- 3; pieceId -- 3; value -- 5; size = 4; pos -- 4; pieceId -- 1; value -- 3;

Two pieceId 4 will appear in the max heap and the position mapping will be a mess. The entire max heap is now broken.

Tools Used

Manual Review

Recommended Mitigation Steps

Within extractMax(), when replace the top item with the last item, remember to reset heap[lastElement] to 0.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Contract can be paused by any user by passing special amount of gas for the call of settleCurrentAndCreateNewAuction (which consists of two internal calls of _settleAuction and _createAuction functions).

Proof of Concept

There is a function _createAuction in Auction contract.

It consists of the following logic:

try verbs.mint() returns (uint256 verbId) {
            uint256 startTime = block.timestamp;
            uint256 endTime = startTime + duration;

            auction = Auction({
                verbId: verbId,
                amount: 0,
                startTime: startTime,
                endTime: endTime,
                bidder: payable(0),
                settled: false
            });

            emit AuctionCreated(verbId, startTime, endTime);
        } catch {
            _pause();
        }

According to the EIP-150 call opcode can consume as most 63/64 of parrent calls’ gas. That means token.mint() can fail since there will be no gas.

All in all, if token.mint() fail on gas and the rest gas is enough for pausing the contract by calling _pause in catch statement the contract will be paused.

Please note, that a bug can be exploitable if the token.mint() consume more than 1.500.000 of gas, because 1.500.000 / 64 > 20.000 that need to pause the contract. Also, the logic of token.mint() includes traversing the array up to 100 times, that’s heavy enough to reach 1.500.000 gas limit.

Also note, this is a bug which is exactly same in the Nouns report which can be found here ([M-15]): https://code4rena.com/reports/2022-09-nouns-builder

Tools Used

Manual Review

Recommended Mitigation Steps

Right now there is a check to make sure the gas is larger than 750000 gas. Can update that based on the art piece creators number. For example, if creators is larger than 80, set the minimum gas to 2000000, etc.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When computing the amount of tokens one can get with certain amount of Ether, it's important to make sure getTokenQuoteForEther() always uses the latest sold amount. However, within buyToken(), the function calls getTokenQuoteForEther() for creators and buyers separately using the same sold token amount, then updating the sold amount, this means the token amount computed for buyers uses the sold amount which doesn't take the token for creators into consideration, and based on the mechanism of VRGDAC, this means buyers can get more token than they should.

Proof of Concept

When buyToken(), part of the msg.value will be used to buy tokens for creators and part of msg.value will be used to buy tokens for buyers. The function evaluates these two values, then update the emittedTokenWad.

int totalTokensForCreators = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0
            ? getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment)
            : int(0);

        // Tokens to emit to buyers
        int totalTokensForBuyers = toPayTreasury > 0 ? getTokenQuoteForEther(toPayTreasury) : int(0);

        //Transfer ETH to treasury and update emitted
        emittedTokenWad += totalTokensForBuyers;
        if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;

We can see here it uses getTokenQuoteForEther() to compute the token amount.

function getTokenQuoteForEther(uint256 etherAmount) public view returns (int gainedX) {
        require(etherAmount > 0, "Ether amount must be greater than 0");
        // Note: By using toDaysWadUnsafe(block.timestamp - startTime) we are establishing that 1 "unit of time" is 1 day.
        // solhint-disable-next-line not-rely-on-time
        return vrgdac.yToX({ timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime), sold: emittedTokenWad, amount: int(etherAmount) });
    }

Since we're using VRGDAC, it's important to make sure that we always use the latest emittedTokenWad in getTokenQuoteForEther(), if we use an outdated value, that means we are buying token at a cheap price (in VRGDAC basically the more we sold the higher the price).

But here in buyToken() we can see after calculating the token amount for creators, it doesn't update the emittedTokenAmount, so the token for buyers is computed with outdated sold amount and buyers will be minted more tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Immediately updating the emittedTokenWad after calculating the totalTokensForCreators.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In _settleAuction() the auctioned art piece can at most have 100 creators, when distribute the _auction.amount to these creators, the contract will send Ether to each creator along with 50000 gas. It's possible that a malicious art piece is created and voted to the auction, and each creators can consume all the 50000 gas sent to it. As a result, to settle the auction, at least 50000 * 100 = 5000000 gas should be used (not even considering other considerations like minting ERC20 tokens for those 100 creators), current upper limit of a block is 30000000, so 5000000 gas is really expensive so maybe no one want to call and settle the auction, and thus a DOS of the protocol.

Proof of Concept

In _settleAuction() it will iterate through all creators and distribute the Ether:

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;

                        //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;

                        //Transfer creator's share to the creator
                        _safeTransferETHWithFallback(creator.creator, paymentAmount);
                    }
                }

And _safeTransferETHWithFallback() will sent Ether to each creator with 50000 gas:

assembly {
            // Transfer ETH to the recipient
            // Limit the call to 50,000 gas
            success := call(50000, _to, _amount, 0, 0, 0, 0)
        }

At most 100 creators for an art piece is allowed, so it's possible that 5000000 gas is required to settle an auction (and there are many other operations also). So maybe no one has the incentive to pay for that gas and the protocol will be DOSed.

Tools Used

Manual Review

Recommended Mitigation Steps

100 creators seems meaningless, I recommend to set the upper limit for creators to 10, And when send the Ether limit the gas cost to 10000.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In buyToken(), when computing the mint amount of the ERC20 token, getTokenQuoteForEther() will be called and use "emittedTokenWad" as the token amount already sold to perform the calculation. However, due to rounding errors the "emittedTokenWad" can be larger than the actual sold token, so getTokenQuoteForEther() can return an inaccurate value.

Proof of Concept

In buyToken(), once "totalTokensForCreators" and "totalTokensForBuyers" are calculated, "emittedTokenWad" will be added with these two values:

//Transfer ETH to treasury and update emitted
        emittedTokenWad += totalTokensForBuyers;
        if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;

And we can see getTokenQuoteForEther() uses "emittedTokenWad" to do the calculation:

function getTokenQuoteForEther(uint256 etherAmount) public view returns (int gainedX) {
        require(etherAmount > 0, "Ether amount must be greater than 0");
        // Note: By using toDaysWadUnsafe(block.timestamp - startTime) we are establishing that 1 "unit of time" is 1 day.
        // solhint-disable-next-line not-rely-on-time
        return vrgdac.yToX({ timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime), sold: emittedTokenWad, amount: int(etherAmount) });
    }

However, when minting tokens for buyers, the actual mint amount may be a little different due to rounding error.

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];
        }

As we can see, during the iteration, totalTokensForBuyers * int(basisPointSplits[i])) / 10_000 can round down, for one buyer, the maximum difference is around 1e4, and in a purchase by calling buyToken() there can be many buyers. So actual mint amount of token will be less than "emittedTokenWad".

Since this project using VRGDAC, if the token sold is far more ahead of time, the price can increase exponentially. So if "emittedTokenWad" is used instead of the actual mint amount, the function will compute the token amount one can get with a much higher price and the user will suffer a loss.

Tools Used

Manual Review

Recommended Mitigation Steps

Considering using token.totalSupply() instead of "emittedTokenWad", totalSupply is exactly same as the total token being sold.

Assessed type

Other