Revolution Protocol - KupiaSec's results

Lines of code

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

Vulnerability details

Impact

In the buyToken function of the ERC20TokenEmitter contract, when users buy tokens they send eth to treasury and creator, for creators only portion of creator reward transfered directly and remaining eth will be locked in the contract untouched.

Proof of Concept

As can be observed by looking the buyToken function of the ERC20TokenEmitter contract creator receive reward according to the entropyBps.

    function buyToken(
        address[] calldata addresses,
        uint[] calldata basisPointSplits,
        ProtocolRewardAddresses calldata protocolRewardsRecipients
    ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
        //prevent treasury from paying itself
        require(msg.sender != treasury && msg.sender != creatorsAddress, "Funds recipient cannot buy tokens");

        require(msg.value > 0, "Must send ether");
        // ensure the same number of addresses and bps
        require(addresses.length == basisPointSplits.length, "Parallel arrays required");

        // Get value left after protocol rewards
        uint256 msgValueRemaining = _handleRewardsAndGetValueToSend(
            msg.value,
            protocolRewardsRecipients.builder,
            protocolRewardsRecipients.purchaseReferral,
            protocolRewardsRecipients.deployer
        );

        //Share of purchase amount to send to treasury
        uint256 toPayTreasury = (msgValueRemaining * (10_000 - creatorRateBps)) / 10_000;

        //Share of purchase amount to reserve for creators
        //Ether directly sent to creators
        uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000;
        //Tokens to emit to creators
        int totalTokensForCreators = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0 //@audit remaining in the contract
            ? 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;

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

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

        //Mint tokens for creators
        if (totalTokensForCreators > 0 && creatorsAddress != address(0)) {
            _mint(creatorsAddress, uint256(totalTokensForCreators));
        }

        ...
    }

If entropyBps < 10_000, then (msgValueRemaining - toPayTreasury) - creatorDirectPayment > 0 and they mint tokens for creators with this amount of Eth, however, after that they don't transfer this to the creator and this will be locked in the contract forever. If more users buy, the locked amount will increase and there is no withdrawal system in this contract.

Recommended Mitigation Steps

Send remaining amount to creators or treasury when entropyBps is lower than 10_000. If these amounts aren't for creators, there should be a method to withdraw from the contract.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

The auction parameters can be changed anytime, even during ongoing auctions, and take effect immediately. Users may need time to react to the changes. The impacts maybe followings:

• some sudden changes may cause bidder's transaction fail, such as setReservePrice() and setMinBidIncrementPercentage()
• some changes may change users expectation about the auction, such as setTimeBuffer(), with different time parameters, bidders will use different strategy

Proof of Concept

These params will affect a live auction after being changed.

    File: AuctionHouse.sol
    function setTimeBuffer(uint256 _timeBuffer) external override onlyOwner {
        timeBuffer = _timeBuffer;

        emit AuctionTimeBufferUpdated(_timeBuffer);
    }

    /**
     * @notice Set the auction reserve price.
     * @dev Only callable by the owner.
     */
    function setReservePrice(uint256 _reservePrice) external override onlyOwner {
        reservePrice = _reservePrice;

        emit AuctionReservePriceUpdated(_reservePrice);
    }

    /**
     * @notice Set the auction minimum bid increment percentage.
     * @dev Only callable by the owner.
     */
    function setMinBidIncrementPercentage(uint8 _minBidIncrementPercentage) external override onlyOwner {
        minBidIncrementPercentage = _minBidIncrementPercentage;

        emit AuctionMinBidIncrementPercentageUpdated(_minBidIncrementPercentage);
    }

277-301

Recommended Mitigation Steps

• do not apply changed parameters on ongoing auctions
• add a timelock for the changes

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The _vote function of CultureIndex contract, uses the past voting power of art pieces' creation block, but users can buy tokens at the same block of art creation and quorum can be bypassed easily

Proof of Concept

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

        // TODO add security consideration here based on block created to prevent flash attacks on drops?
        maxHeap.updateValue(pieceId, totalWeight);
        emit VoteCast(pieceId, voter, weight, totalWeight);
    }

    function getPastVotes(address account, uint256 blockNumber) external view override returns (uint256) {
        return _getPastVotes(account, blockNumber);
    }

    function _getPastVotes(address account, uint256 blockNumber) internal view returns (uint256) {
        return
            _calculateVoteWeight(
                erc20VotingToken.getPastVotes(account, blockNumber),
                erc721VotingToken.getPastVotes(account, blockNumber)
            );
    }

erc20VotingToken.getPastVotes function will return the voting power of blockNumber.

• So, create art piece at block T0.
• buy some tokens at block T0.
• at block T0 + 1, vote with the token bought at T0

By doing this, totalVoteWeights[pieceId] might be greater than totalVotesSupply of that piece and quorum of vote can be easily bypassed.

Recommended Mitigation Steps

When getting past voting power creation of art block should not be contained. Also we should use a total supply of the previous block also.

    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);
+       uint256 weight = _getPastVotes(voter, pieces[pieceId].creationBlock-1);
        require(weight > minVoteWeight, "Weight must be greater than minVoteWeight");

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

        uint256 totalWeight = totalVoteWeights[pieceId];

        // TODO add security consideration here based on block created to prevent flash attacks on drops?
        maxHeap.updateValue(pieceId, totalWeight);
        emit VoteCast(pieceId, voter, weight, totalWeight);
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The buyToken function of the ERC20TokenEmitter contract, enables users to buy tokens for list of addresses. The function doesn't have any type of slippage control, so if the price of the token is much higher or gets higher than users' expectation, the amount of token received in return may vary indefinitely while the request is waiting to be excuted.

Also the users will have no defense against price manipulation attacks.

Proof of Concept

As can be observed by looking the buyToken function of the ERC20TokenEmitter contract has on slippage protection.

    function buyToken(
        address[] calldata addresses,
        uint[] calldata basisPointSplits,
        ProtocolRewardAddresses calldata protocolRewardsRecipients
    ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
        //prevent treasury from paying itself
        require(msg.sender != treasury && msg.sender != creatorsAddress, "Funds recipient cannot buy tokens");

        require(msg.value > 0, "Must send ether");
        // ensure the same number of addresses and bps
        require(addresses.length == basisPointSplits.length, "Parallel arrays required");

        // Get value left after protocol rewards
        uint256 msgValueRemaining = _handleRewardsAndGetValueToSend(
            msg.value,
            protocolRewardsRecipients.builder,
            protocolRewardsRecipients.purchaseReferral,
            protocolRewardsRecipients.deployer
        );

        //Share of purchase amount to send to treasury
        uint256 toPayTreasury = (msgValueRemaining * (10_000 - creatorRateBps)) / 10_000;

        //Share of purchase amount to reserve for creators
        //Ether directly sent to creators
        uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000;
        //Tokens to emit to creators
        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;

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

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

        //Mint tokens for creators
        if (totalTokensForCreators > 0 && creatorsAddress != address(0)) {
            _mint(creatorsAddress, uint256(totalTokensForCreators));
        }

        uint256 bpsSum = 0;

        //Mint tokens to buyers

        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");

        emit PurchaseFinalized(
            msg.sender,
            msg.value,
            toPayTreasury,
            msg.value - msgValueRemaining,
            uint256(totalTokensForBuyers),
            uint256(totalTokensForCreators),
            creatorDirectPayment
        );

        return uint256(totalTokensForBuyers);
    }

And this means users have no idea of how many tokens they will get in return.

The amount of token users will receive, depends on the price of the token and the number of tokens sold so far. getTokenQuoteForEther function of ERC20TokenEmitter contract, shows the number of tokens with given etherAmount.

    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, //@audit the number of tokens sold
                amount: int(etherAmount)
            });
    }

If tokens are sold more than planned, the price gets much higher. So, if users try to buy tokens, malicious users can front run and buy the tokens first. And this means the emittedTokenWad increases as well as the price. Since there is no slippage protection in the buyToken function, in worst case users will get no tokens and just wasting their funds.

Recommended Mitigation Steps

Adding additional parameter could be added to the buyToken function, to let users decide the minimum amount of tokens to be received.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The buyToken function of the ERC20TokenEmitter contract, splits buying process into 2 steps, one for creators and one for users. But if buying is splitted like this users and creators will get more tokens than paid amount. In other words more tokens will be minted than the paid amount and this is contrary of price logic.

Proof of Concept

As we can se from the PoC, the curren implementation mints more tokens than expected.

function getTokenQuoteForEtherHelper(uint256 etherAmount, int256 supply) 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
            erc20TokenEmitter.vrgdac().yToX({
                timeSinceStart: toDaysWadUnsafe(block.timestamp - erc20TokenEmitter.startTime()),
                sold: supply,
                amount: int(etherAmount)
            });
    }

    function test_correctEmitted() public {
        uint256 creatorRateBps = 2_000;
        uint256 entropyRateBps = 8_000;

        vm.startPrank(erc20TokenEmitter.owner());
        //set creatorRate and entropyRate
        erc20TokenEmitter.setCreatorRateBps(creatorRateBps);
        erc20TokenEmitter.setEntropyRateBps(entropyRateBps);
        vm.stopPrank();

        vm.deal(address(this), 100000 ether);

        emit log_address(erc20TokenEmitter.creatorsAddress());
        emit log_uint(erc20Token.balanceOf(erc20TokenEmitter.creatorsAddress()));

        //expect balance to start out at 0
        assertEq(erc20Token.balanceOf(erc20TokenEmitter.creatorsAddress()), 0, "Balance should start at 0");

        address[] memory recipients = new address[](1);
        recipients[0] = address(1);

        uint256[] memory bps = new uint256[](1);
        bps[0] = 10_000;

        //expect recipient0 balance to start out at 0
        assertEq(erc20Token.balanceOf(address(1)), 0, "Balance should start at 0");

        //get msg value remaining
        uint256 msgValueRemaining = 1 ether - erc20TokenEmitter.computeTotalReward(1 ether);

        //Share of purchase amount to send to treasury
        uint256 toPayTreasury = (msgValueRemaining * (10_000 - creatorRateBps)) / 10_000;

        //Ether directly sent to creators
        uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000;

        //get expected tokens for creators
        int256 expectedAmountForCreators = erc20TokenEmitter.getTokenQuoteForEther(
            msgValueRemaining - toPayTreasury - creatorDirectPayment
        );

        //get expected tokens for recipient0
        int256 expectedAmountForRecipient0 = getTokenQuoteForEtherHelper(toPayTreasury, expectedAmountForCreators);

        erc20TokenEmitter.buyToken{ value: 1 ether }(
            recipients,
            bps,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: address(0)
            })
        );

        //log creatorsAddress balance
        emit log_uint(erc20Token.balanceOf(erc20TokenEmitter.creatorsAddress()));

        //assert that creatorsAddress balance is correct
        assertEq(
            uint(erc20Token.balanceOf(erc20TokenEmitter.creatorsAddress())),
            uint(expectedAmountForCreators),
            "Creators should have correct balance"
        );

        //log recipient0 balance
        emit log_uint(erc20Token.balanceOf(address(1)));

        // assert that recipient0 balance is correct
        assertEq(
            uint(erc20Token.balanceOf(address(1))),
            uint(expectedAmountForRecipient0),
            "Recipient0 should have correct balance"
        );
    }

Recommended Mitigation Steps

You can update emittedTokenWad before buying tokens for users

        int totalTokensForCreators = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0
            ? getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment)
            : int(0);
+       if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;
        // 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;

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If AuctionHouse.entropyRateBps is zero, the auction can't be settled

Proof of Concept

    function _settleAuction() internal {
        IAuctionHouse.Auction memory _auction = auction;


        require(_auction.startTime != 0, "Auction hasn't begun");
        require(!_auction.settled, "Auction has already been settled");
        //slither-disable-next-line timestamp
        require(block.timestamp >= _auction.endTime, "Auction hasn't completed");


        auction.settled = true;


        uint256 creatorTokensEmitted = 0;
        // Check if contract balance is greater than reserve price
        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;


                uint256 numCreators = verbs.getArtPieceById(_auction.verbId).creators.length;
                address deployer = verbs.getArtPieceById(_auction.verbId).sponsor;


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


                //Transfer auction amount to the DAO treasury
                _safeTransferETHWithFallback(owner(), auctioneerPayment);


                uint256 ethPaidToCreators = 0;


                //Transfer creator's share to the creator, for each creator, and build arrays for erc20TokenEmitter.buyToken
                if (creatorsShare > 0 && entropyRateBps > 0) { //@audit if entropyRateBps is zero this condition will not met
                    for (uint256 i = 0; i < numCreators; i++) {
                        ICultureIndex.CreatorBps memory creator = verbs.getArtPieceById(_auction.verbId).creators[i];
                        vrgdaReceivers[i] = creator.creator; //@audit and these will be zero addresses
                        vrgdaSplits[i] = creator.bps; //@audit and these will be zero values


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


                //Buy token from ERC20TokenEmitter for all the creators
                if (creatorsShare > ethPaidToCreators) { //@audit and here try to buy tokens to receivers
                    creatorTokensEmitted = erc20TokenEmitter.buyToken{ value: creatorsShare - ethPaidToCreators }(
                        vrgdaReceivers,
                        vrgdaSplits,
                        IERC20TokenEmitter.ProtocolRewardAddresses({
                            builder: address(0),
                            purchaseReferral: address(0),
                            deployer: deployer
                        })
                    );
                }
            }
        }


        emit AuctionSettled(_auction.verbId, _auction.bidder, _auction.amount, creatorTokensEmitted);
    }

If entropyRateBps is zero, vrgdaSplits, vrgdaReceivers are zero values and zero addresses and these will be provided as parameters of erc20TokenEmitter.buyToken

File: ERC20TokenEmitter.sol

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

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

209-217 Here it will revert because of minting to zero address and check at L217 So, if entropyRateBps is not set, the auction can't be settled and AuctionHouse will be bricked

Recommended Mitigation Steps

vrgdaSplits and vrgdaReceivers should be set properly when entropyRateBps == 0.

Assessed type

Other