Revolution Protocol - Sathish9098's results

GAS OPTIMIZATION

[G-1] dropperAdmin and quorumVotesBPS state variables can be packed within same slot : Saves 2000 GAS , 1 SLOT

Use uint96 for quorumVotesBPS instead of uint256 is indeed appropriate and efficient, given the constraint that quorumVotesBPS will not exceed 6,000 (MAX_QUORUM_VOTES_BPS).

This constraint is enforced by the check require(_cultureIndexParams.quorumVotesBPS <= MAX_QUORUM_VOTES_BPS, "invalid quorum bps");, ensuring that the value of quorumVotesBPS remains within the bounds that uint96 can comfortably handle.

FILE: 2023-12-revolutionprotocol/packages/revolution/src/CultureIndex.sol

  /// @notice The basis point number of votes in support of a art piece required in order for a quorum to be reached and for an art piece to be dropped.
-    uint256 public quorumVotesBPS;

    /// @notice The name of the culture index
    string public name;

    /// @notice A description of the culture index - can include rules or guidelines
    string public description;

    // The list of all pieces
    mapping(uint256 => ArtPiece) public pieces;

    // The internal piece ID tracker
    uint256 public _currentPieceId;

    // The mapping of all votes for a piece
    mapping(uint256 => mapping(address => Vote)) public votes;

    // The total voting weight for a piece
    mapping(uint256 => uint256) public totalVoteWeights;

    // Constant for max number of creators
    uint256 public constant MAX_NUM_CREATORS = 100;

    // The address that is allowed to drop art pieces
    address public dropperAdmin;
+    uint96 public quorumVotesBPS;

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

[G-2] creatorRateBps and treasury , entropyRateBps,creatorsAddress state variables can be packed within same slot : Saves 4000 GAS , 2 SLOTs

creatorRateBps and entropyRateBps are guaranteed not to exceed 10,000 based on the checks in the setter functions (_entropyRateBps <= 10_000 and _creatorRateBps <= 10_000), then using uint96 for these variables instead of uint256 is a more efficient choice.

FILE: 2023-12-revolutionprotocol/packages/revolution/src
/ERC20TokenEmitter.sol

 // treasury address to pay funds to
    address public treasury;
+    uint96 public creatorRateBps;

    // The token that is being minted.
    NontransferableERC20Votes public token;

    // The VRGDA contract
    VRGDAC public vrgdac;

    // solhint-disable-next-line not-rely-on-time
    uint256 public startTime;

    /**
     * @notice A running total of the amount of tokens emitted.
     */
    int256 public emittedTokenWad;

    // The split of the purchase that is reserved for the creator of the Verb in basis points
-    uint256 public creatorRateBps;

    // The split of (purchase proceeds * creatorRate) that is sent to the creator as ether in basis points
-    uint256 public entropyRateBps;
+    uint96 public entropyRateBps;

    // The account or contract to pay the creator reward to
    address public creatorsAddress;

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

[G-3] creatorRateBps,verbs,minCreatorRateBps,erc20TokenEmitter, entropyRateBps,WETH can be packed with same slot : Saves 6000 GAS , 3 SLOT

creatorRateBps,minCreatorRateBps,entropyRateBps can be uint96 instead of uint256. As per setter function checks _creatorRateBps >= minCreatorRateBps, _creatorRateBps <= 10_000, _entropyRateBps <= 10_000 the values not exceeds 10000 .

FILE: 2023-12-revolutionprotocol/packages/revolution/src
/AuctionHouse.sol

// The Verbs ERC721 token contract
    IVerbsToken public verbs;
+    uint96 public creatorRateBps;

    // The ERC20 governance token
    IERC20TokenEmitter public erc20TokenEmitter;
+    uint96 public minCreatorRateBps;

    // The address of the WETH contract
    address public WETH;
+    uint96 public entropyRateBps;

    // The minimum amount of time left in an auction after a new bid is created
    uint256 public timeBuffer;

    // The minimum price accepted in an auction
    uint256 public reservePrice;

    // The minimum percentage difference between the last bid amount and the current bid
    uint8 public minBidIncrementPercentage;

    // The split of the winning bid that is reserved for the creator of the Verb in basis points
-    uint256 public creatorRateBps;

    // The all time minimum split of the winning bid that is reserved for the creator of the Verb in basis points
-    uint256 public minCreatorRateBps;

    // The split of (auction proceeds * creatorRate) that is sent to the creator as ether in basis points
-    uint256 public entropyRateBps;

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

[G-4] Caching the result of the calculation (msgValueRemaining - toPayTreasury) - creatorDirectPayment in a local variable

The expression (msgValueRemaining - toPayTreasury) - creatorDirectPayment is computed once and stored in the local variable calculatedValue. calculatedValue is then used in the conditional statement to check if it's greater than 0 and also as an argument in the getTokenQuoteForEther function call. This change ensures the calculation is done only once, saving gas that would otherwise be used for repeated computation

FILE: 2023-12-revolutionprotocol/packages/revolution/src/ERC20TokenEmitter.sol

 uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000;

+ uint256 results = (msgValueRemaining - toPayTreasury) - creatorDirectPayment) ;
        //Tokens to emit to creators
-        int totalTokensForCreators = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0
+        int totalTokensForCreators = results > 0
-            ? getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment)
+            ? getTokenQuoteForEther(results)
            : int(0);

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

[G-5] creatorsAddress and treasury storage variables should be cached with stack variable

This caching creatorsAddress and treasury reduces the number of read operations on the storage, which can be significantly more expensive than memory operations. Saves 400 GAS , 4 SLOD

FILE: 2023-12-revolutionprotocol/packages/revolution/src/ERC20TokenEmitter.sol

 ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
        //prevent treasury from paying itself
+     address creatorsAddress_ = creatorsAddress ;
+     address treasury_ = treasury ;
-        require(msg.sender != treasury && msg.sender != creatorsAddress, "Funds recipient cannot buy tokens");
+        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));
+        (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));
+            (success, ) = creatorsAddress_ .call{ value: creatorDirectPayment }(new bytes(0));
            require(success, "Transfer failed.");
        }

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

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

[G-6] _vote function can be optimized for better gas efficiency

FILE: 2023-12-revolutionprotocol/packages/revolution/src
/CultureIndex.sol

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);
+       uint256 totalVoteWeights_ = totalVoteWeights[pieceId] + weight ;
-        totalVoteWeights[pieceId] += weight;
+        totalVoteWeights[pieceId] = totalVoteWeights_;

-        uint256 totalWeight = totalVoteWeights[pieceId];

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

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

[G-7] parent(current) function return value should be cached with stack variable instead of calling second time

Caching the parent(current) value inside the while loop is indeed a good practice for optimizing the function. This change reduces the number of times the parent function is called, which can lead to gas savings

FILE: 2023-12-revolutionprotocol/packages/revolution/src/MaxHeap.sol

124: uint256 current = size;
+ uint256 parent_;
        while (current != 0 && valueMapping[heap[current]] > valueMapping[heap[parent(current)]]) {
+           parent_ = parent(current) ;
-            swap(current, parent(current));
+            swap(current, parent_);
-            current = parent(current);
+            current = parent_;
        }


143: // Decide whether to perform upwards or downwards heapify
        if (newValue > oldValue) {
            // Upwards heapify
            while (position != 0 && valueMapping[heap[position]] > valueMapping[heap[parent(position)]]) {
+      uint256 parent_ = parent(current) ;          
-                swap(position, parent(position));
+                swap(position, parent_);
-                position = parent(position);
+                position = parent_;
            }
        } else if (newValue < oldValue) maxHeapify(position); // Downwards heapify

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

[G-8] Check Arguments Early

Checks that require() or revert() statements that check input arguments are at the top of the function. Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a gas load in a function that may ultimately revert in the unhappy case.

FILE: 2023-12-revolutionprotocol/packages/revolution/src/ERC20TokenEmitter.sol

) 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");
+   require(msg.sender != treasury && msg.sender != creatorsAddress, "Funds recipient cannot buy tokens");

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

FILE: 2023-12-revolutionprotocol/packages/revolution/src
/CultureIndex.sol
function _vote(uint256 pieceId, address voter) internal {
+        require(voter != address(0), "Invalid voter address");
  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");

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

[G-9] Optimize the maxHeapify() function for better gas efficiency

• Move the check for the position being a leaf node to the beginning of the function to avoid unnecessary computations.

• Delay fetching the values of left and right until after you've confirmed that these positions are valid and need to be accessed. This can save gas if the function exits early or if one of the child nodes does not need to be compared.

• Store results of repeated calculations in local variables to avoid recalculating them.

FILE: 2023-12-revolutionprotocol/packages/revolution/src/MaxHeap.sol

-function maxHeapify(uint256 pos) internal {
-        uint256 left = 2 * pos + 1;
-        uint256 right = 2 * pos + 2;
-
-        uint256 posValue = valueMapping[heap[pos]];
-        uint256 leftValue = valueMapping[heap[left]];
-        uint256 rightValue = valueMapping[heap[right]];
-
-        if (pos >= (size / 2) && pos <= size) return;
-
-        if (posValue < leftValue || posValue < rightValue) {
-           if (leftValue > rightValue) {
-                swap(pos, left);
-                maxHeapify(left);
-            } else {
-                swap(pos, right);
-                maxHeapify(right);
-            }
-        }
-    }

+ function maxHeapify(uint256 pos) internal {
+    // Early exit if 'pos' is a leaf node
+    if (pos >= (size / 2) && pos <= size) return;
+
+    uint256 left = 2 * pos + 1;
+    uint256 right = 2 * pos + 2;
+    uint256 size_ = size; // Cache the 'size' to avoid repeated storage access
+
+    // Only fetch 'posValue' once
+    uint256 posValue = valueMapping[heap[pos]];
+
+    // Check if children are within bounds before accessing their values
+    uint256 leftValue = left < size_ ? valueMapping[heap[left]] : 0;
+    uint256 rightValue = right < size_ ? valueMapping[heap[right]] : 0;
+
+    if (posValue < leftValue || posValue < rightValue) {
+        if (leftValue > rightValue) {
+            swap(pos, left);
+            maxHeapify(left);
+        } else {
+            swap(pos, right);
+            maxHeapify(right);
+        }
+    }
+ }

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

Modified code version

• The function immediately checks if pos is a leaf node and exits if so.
• The size variable is cached in a local variable size_.
• The values of leftValue and rightValue are only fetched if left and right are within the bounds of the heap.
• The function only accesses the valueMapping for pos, left, and right when necessary.

[G-10] Writing state variables inside the emit blocks is not gas efficient

Writing state variables inside the emit block as shown in emit is not an efficient practice in Solidity, particularly from a gas usage perspective. The correct approach is to first assign the new value to the state variable and then emit the event. This separation ensures clarity and efficiency in the contract's operations. As per remix tests mitigation steps saves 17 GAS for every call.

SampleTest

contract UncheckedINEDECTest {

    uint a=10;
    event hit(uint256 h);

    function test(uint256 i) public { //3797 GAS

      emit hit(a=i); 
     
    }

    function test1(uint256 i) public { //3780 GAS
a=i;
      emit hit(i); 

    }

FILE: Breadcrumbs2023-12-revolutionprotocol/packages/revolution/src/ERC20TokenEmitter.sol

function setEntropyRateBps(uint256 _entropyRateBps) external onlyOwner {
        require(_entropyRateBps <= 10_000, "Entropy rate must be less than or equal to 10_000");
+   entropyRateBps = _entropyRateBps ; 
-        emit EntropyRateBpsUpdated(entropyRateBps = _entropyRateBps);
+        emit EntropyRateBpsUpdated(_entropyRateBps);
    }

function setCreatorRateBps(uint256 _creatorRateBps) external onlyOwner {
        require(_creatorRateBps <= 10_000, "Creator rate must be less than or equal to 10_000");
+   creatorRateBps = _creatorRateBps ;
-        emit CreatorRateBpsUpdated(creatorRateBps = _creatorRateBps);
+        emit CreatorRateBpsUpdated(_creatorRateBps);
    }

function setCreatorsAddress(address _creatorsAddress) external override onlyOwner nonReentrant {
        require(_creatorsAddress != address(0), "Invalid address");
+    creatorsAddress = _creatorsAddress ;
-        emit CreatorsAddressUpdated(creatorsAddress = _creatorsAddress);
+        emit CreatorsAddressUpdated(_creatorsAddress);
    }

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

[G-11] newPiece.totalVotesSupply state variable value should be cached

FILE: 2023-12-revolutionprotocol/packages/revolution/src/CultureIndex.sol

       newPiece.creationBlock = block.number;
+   uint256 totalVotesSupply_ = newPiece.totalVotesSupply;
-        newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000;
+        newPiece.quorumVotes = (quorumVotesBPS * totalVotesSupply_ ) / 10_000;

        for (uint i; i < creatorArrayLength; i++) {
            newPiece.creators.push(creatorArray[i]);
        }

-        emit PieceCreated(pieceId, msg.sender, metadata, newPiece.quorumVotes, newPiece.totalVotesSupply);
+        emit PieceCreated(pieceId, msg.sender, metadata, newPiece.quorumVotes, totalVotesSupply_ );

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

[G-12] computePurchaseRewards functions can be more gas optimized

Combine Calculations: Calculate each reward component once and use these calculations both for populating the RewardsSettings struct and for computing the total reward. This avoids duplicating the same multiplication and division operations.

Direct Total Reward Computation: Instead of calling computeTotalReward, directly compute the total reward in computePurchaseRewards using the already calculated components.

Original Code

FILE : 2023-12-revolutionprotocol/packages/protocol-rewards/src/abstract
/RewardSplits.sol

function computeTotalReward(uint256 paymentAmountWei) public pure returns (uint256) {
        if (paymentAmountWei <= minPurchaseAmount || paymentAmountWei >= maxPurchaseAmount) revert INVALID_ETH_AMOUNT();

        return
            (paymentAmountWei * BUILDER_REWARD_BPS) /
            10_000 +
            (paymentAmountWei * PURCHASE_REFERRAL_BPS) /
            10_000 +
            (paymentAmountWei * DEPLOYER_REWARD_BPS) /
            10_000 +
            (paymentAmountWei * REVOLUTION_REWARD_BPS) /
            10_000;
    }

    function computePurchaseRewards(uint256 paymentAmountWei) public pure returns (RewardsSettings memory, uint256) {
        return (
            RewardsSettings({
                builderReferralReward: (paymentAmountWei * BUILDER_REWARD_BPS) / 10_000,
                purchaseReferralReward: (paymentAmountWei * PURCHASE_REFERRAL_BPS) / 10_000,
                deployerReward: (paymentAmountWei * DEPLOYER_REWARD_BPS) / 10_000,
                revolutionReward: (paymentAmountWei * REVOLUTION_REWARD_BPS) / 10_000
            }),
            computeTotalReward(paymentAmountWei)
        );
    }

Optimized Code

This approach eliminates the need for the separate computeTotalReward function call, reducing the gas cost associated with redundant calculations

function computePurchaseRewards(uint256 paymentAmountWei) public pure returns (RewardsSettings memory, uint256) {
    // Check if paymentAmountWei is within valid range
    if (paymentAmountWei <= minPurchaseAmount || paymentAmountWei >= maxPurchaseAmount) {
        revert INVALID_ETH_AMOUNT();
    }

    // Calculate each reward component
    uint256 builderReferralReward = (paymentAmountWei * BUILDER_REWARD_BPS) / 10_000;
    uint256 purchaseReferralReward = (paymentAmountWei * PURCHASE_REFERRAL_BPS) / 10_000;
    uint256 deployerReward = (paymentAmountWei * DEPLOYER_REWARD_BPS) / 10_000;
    uint256 revolutionReward = (paymentAmountWei * REVOLUTION_REWARD_BPS) / 10_000;

    // Compute the total reward
    uint256 totalReward = builderReferralReward + purchaseReferralReward + deployerReward + revolutionReward;

    return (
        RewardsSettings({
            builderReferralReward: builderReferralReward,
            purchaseReferralReward: purchaseReferralReward,
            deployerReward: deployerReward,
            revolutionReward: revolutionReward
        }),
        totalReward
    );
}

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/protocol-rewards/src/abstract/RewardSplits.sol#L40-L64

[G-13] timeBuffer storage variables should be cached with stack variable

FILE: 2023-12-revolutionprotocol/packages/revolution/src/AuctionHouse.sol

// Extend the auction if the bid was received within `timeBuffer` of the auction end time
+  uint256 timeBuffer_ = timeBuffer ;
-        bool extended = _auction.endTime - block.timestamp < timeBuffer;
+        bool extended = _auction.endTime - block.timestamp < timeBuffer_;
-        if (extended) auction.endTime = _auction.endTime = block.timestamp + timeBuffer;
+        if (extended) auction.endTime = _auction.endTime = block.timestamp + timeBuffer_ ;

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

[G-14] size storage variable should be cached with stack variable

Caching the size storage variable on the stack can lead to gas savings, especially in a context where size is accessed multiple times within a function. Saves 100 GAS

FILE: 2023-12-revolutionprotocol/packages/revolution/src/MaxHeap.sol

 uint256 rightValue = valueMapping[heap[right]];
+   uint256 size_ = size ;
-        if (pos >= (size / 2) && pos <= size) return;
+        if (pos >= (size_ / 2) && pos <= size_) return;

        if (posValue < leftValue || posValue < rightValue) {

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

[G-15] msgValueRemaining - toPayTreasury cache the computation results with local variable instead of repeated computations

FILE: 2023-12-revolutionprotocol/packages/revolution/src
/ERC20TokenEmitter.sol

//Share of purchase amount to reserve for creators
        //Ether directly sent to creators
+    uint256 msgValueRemainingmsgValueRemainingResult = msgValueRemaining - msgValueRemaining ;
-        uint256 creatorDirectPayment = ((msgValueRemaining - msgValueRemaining) * entropyRateBps) / 10_000;
+        uint256 creatorDirectPayment = (msgValueRemainingmsgValueRemainingResult * entropyRateBps) / 10_000;
        //Tokens to emit to creators
-        int totalTokensForCreators = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0
+        int totalTokensForCreators = (msgValueRemainingmsgValueRemainingResult - creatorDirectPayment) > 0
-            ? getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment)
+            ? getTokenQuoteForEther(msgValueRemainingmsgValueRemainingResult - creatorDirectPayment)
            : int(0);

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

Revolution Protocol Analysis

Overview

Revolution Protocol is a blockchain initiative that builds upon the principles of Nouns DAO. It focuses on making governance token ownership more accessible, particularly for creators and builders. Revolution distinguishes itself by aiming to balance the dynamics between culture and capital and commits to a constant governance inflation schedule. Its ultimate goal is to achieve fair ownership distribution within a community movement, enabling broader participation in decision-making processes. This approach is designed to empower a wider range of participants in the governance of the collective.

Architecture assessment of business logic

MaxHeap Architecture

![Re

The MaxHeap contract in the Revolution Protocol is a implementation of a max heap data structure, designed for efficient retrieval and management of prioritized data. It uses arrays to represent the heap, with heap storing the heap structure, valueMapping holding values of items, and positionMapping tracking item positions. Key operations include insert for adding elements, updateValue for adjusting item values, extractMax for removing the top element, and getMax for viewing the maximum value without removal. Internal functions like parent, swap, and maxHeapify assist in maintaining heap properties during modifications. The contract employs an onlyAdmin modifier for access control, indicating centralized management

CultureIndex Architecture

The CultureIndex contract in the Revolution Protocol acts as a directory for art pieces, facilitating decentralized voting and ranking of artworks. It integrates a MaxHeap data structure to efficiently track and retrieve the highest voted art pieces. Key features include functions for adding art pieces, casting votes, and handling art piece drops. The contract manages art piece metadata and voting details, including vote weights, using mappings and arrays. It employs an onlyOwner modifier for critical administrative actions, indicating a level of centralized control. The contract integrates with ERC20 and ERC721 tokens for voting, highlighting its interoperability within the token ecosystem. CultureIndex also has parameters like minVoteWeight and quorumVotesBPS, which are essential for governance-related actions. This architecture supports a community-driven approach to curating art, leveraging blockchain's transparency and immutability. The contract must address challenges such as gas efficiency, data integrity, and fair voting mechanisms. Overall, CultureIndex stands as a pivotal component in the decentralized decision-making process of the Revolution Protocol

Revolution risk model

Admin abuse risks

 function insert(uint256 itemId, uint256 value) public onlyAdmin {
 function updateValue(uint256 itemId, uint256 newValue) public onlyAdmin {
 function extractMax() external onlyAdmin returns (uint256, uint256) {
 

onlyOwner (updateValue(),extractMax())

The contract includes an admin role with the capability to interact with the heap data structure. This role can insert and update values in the heap.

Risks:

Manipulation of Art Piece Rankings: The admin can potentially manipulate the order of art pieces by altering their values, affecting which art piece gets auctioned next.

Unauthorized Access: If access controls are not properly managed, there's a risk of unauthorized users gaining admin privileges.

function dropTopVotedPiece() public nonReentrant returns (ArtPiece memory) {
        require(msg.sender == dropperAdmin, "Only dropper can drop pieces");
       // Implementations
    }

dropperAdmin (dropTopVotedPiece())

The dropperAdmin role has the authority to drop (remove) the top-voted art piece from consideration.The role centralizes control over which art piece is auctioned, potentially undermining the decentralized voting mechanism.

Risks :

Censorship or Favoritism: The dropperAdmin can potentially exercise censorship or favoritism in the selection of art pieces for auction.

 function mint() public override onlyMinter nonReentrant returns (uint256) {
 
 function burn(uint256 verbId) public override onlyMinter nonReentrant {

onlyMinter ()

The minter is authorized to mint and burn tokens.

Risks:

Inflation: If the minter role is abused, there's a risk of inflating the token supply, leading to devaluation.

Unauthorized Minting and Burning : If the minter role's security is compromised, it can lead to unauthorized minting of tokens.

Mitigation Strategies

Transparent Governance: Implementing a transparent governance model where changes to critical parameters or actions require community consensus can mitigate risks.

Time-Locked Actions : Implementing time locks on critical actions (like changing key parameters) provides the community time to review and respond to changes.

Systemic risks

Economic Model Imbalance

If the economic incentives within the protocol are not well-designed, it could lead to unintended behaviors like gaming the system for profits. This might include manipulating voting or auction outcomes in the CultureIndex or AuctionHouse contracts.

Inflation Risk

If the creatorRateBps is set very high, a significant portion of auction proceeds will be used to purchase ERC20 tokens for creators. This could lead to a large influx of ERC20 tokens in the market. Similarly, if entropyRateBps is set low, more of the creators' earnings are in ERC20 tokens rather than Ether, further increasing the token supply.

Risk Scenario: Suppose an art piece is auctioned for 100 ETH. With a high creatorRateBps (say, 90%), 90 ETH worth of ERC20 tokens are purchased and distributed. If entropyRateBps is low (say, 10%), only 9 ETH is paid directly to creators, and the rest is used to buy ERC20 tokens. This increases the ERC20 token supply significantly, potentially leading to inflation if the demand does not keep up.

Deflation Risk

Conversely, setting a very low creatorRateBps reduces the ERC20 tokens purchased for creators. If the token emission is the primary way new tokens enter circulation, this could constrict the token supply. A high entropyRateBps means more auction proceeds are paid in Ether, reducing the demand for ERC20 tokens.

Risk Scenario: For an auction yielding 100 ETH, with a low creatorRateBps (say, 10%) and high entropyRateBps (say, 90%), only a small amount of ERC20 tokens are purchased (10 ETH worth), and most proceeds are paid in Ether (90 ETH). This could lead to a scarcity of ERC20 tokens, potentially causing deflation if the token utility or demand remains high.

Tokenomics and Governance Token Distribution

The distribution mechanism of governance tokens can create systemic risk. If too centralized, it could lead to decision-making being dominated by a few, undermining the protocol's decentralization

Unchecked Minting

• If the minter has unrestricted ability to mint new tokens at will, this could lead to an excessive increase in the token supply.

Token Devaluation Riks

• An oversupply of tokens, especially if it happens rapidly, can lead to devaluation. In economics, this is similar to inflation where the purchasing power of a currency drops as more of it becomes available.
• In a token ecosystem, this devaluation can diminish the value of tokens held by existing holders, potentially leading to loss of trust and disengagement from the community.

Dependence on External Contracts

Contracts relying on external data or other contracts ( external token contracts) are vulnerable to risks associated with these dependencies. The AuctionHouse contract's dependence on the correct functioning of the VerbsToken contract for auctions. If VerbsToken has a critical failure, it could disrupt the auction process.

Liquidity and Market Risks

Impact of Token Emission on Liquidity:

• If the ERC20TokenEmitter releases a large number of tokens into the market at once, it could temporarily increase liquidity. However, if this influx of tokens is not matched by demand, it could lead to a price drop.
• Conversely, if token emission is too slow or restricted, it could lead to liquidity shortages, making it difficult for users to execute transactions without impacting the price.

Price Volatility from Sudden Value Changes

• Rapid changes in the value of the ERC20 token can result from market speculation, changes in tokenomics, or broader market trends.
• Such volatility can lead to cascading effects, such as triggering liquidations in leveraged positions or causing panic selling.

Behavioral Impacts on Participants

• Abrupt changes in token value or liquidity can influence user behavior. For instance, a sudden price drop might lead to a sell-off, exacerbating the price decline.
• In a scenario where the ERC20 token is used for governance or staking, significant price changes can affect participation in these activities.

Technical risks

Smart Contract Vulnerabilities

• Smart contracts are prone to vulnerabilities due to coding errors or logical flaws. This can include issues like reentrancy attacks, overflow/underflow, improper access control, and more.
• If the ERC20TokenEmitter contract has a function that incorrectly calculates token emission rates or allows unauthorized access, it could lead to unintended token minting or manipulation of token distribution.

Economic Model Exploitation

• Flaws in the economic design of token models can be exploited. This includes token inflation/deflation, governance manipulation, or incentives that lead to undesirable behaviors.
• If the emission rate set by ERC20TokenEmitter is not balanced with the actual utility and demand of the token, it could lead to token devaluation or hyperinflation.

Upgradeability Risks

Upgradeable contracts can introduce risks related to the upgrade process, such as incorrect implementation in new versions or centralization risks due to the control of the upgrade process.

Front-Running and Transaction Ordering Dependency

• On public blockchains, transactions in the mempool can be seen by miners and other participants, who can choose to order transactions in a way that benefits them (front-running).
• In the AuctionHouse contract, users could observe pending bids and quickly place higher bids, exploiting the transaction ordering for profit.

Integration risks

Dependency on CultureIndex in VerbsToken

Malfunction in CultureIndex:

If the CultureIndex contract experiences a malfunction or bug (e.g., incorrect ranking of art pieces, failure in updating votes, or errors in returning art piece data), the VerbsToken contract might receive incorrect or invalid data for tokenization. This could result in minting tokens that represent the wrong art piece or duplicating tokens for the same art piece

Unexpected Changes in CultureIndex:

Suppose CultureIndex undergoes an unexpected upgrade or modification which alters the way art pieces are stored, ranked, or retrieved. In such a case, the VerbsToken contract might not be compatible with the new changes, leading to failures in the minting process or incorrect token attributions.

Data Consistency and Integrity:

Ensuring data integrity between CultureIndex and VerbsToken is critical. Any inconsistency in the art piece data (like metadata mismatches or identifier errors) can lead to issues in how the tokens represent the underlying art pieces

Interaction Between ERC20TokenEmitter and AuctionHouse

Changes in Emission Logic:

• If the emission logic within ERC20TokenEmitter is altered—due to either an upgrade to the contract or changes in parameters like emission rates—it might change the number or rate of tokens distributed to auction creators.
• This could occur, for example, if an upgrade introduces a new formula for calculating token distribution based on auction proceeds.

Impact on AuctionHouse Functionality: The AuctionHouse contract, expecting a certain behavior from ERC20TokenEmitter, might encounter issues if the actual token distribution post-auction does not align with the expected outcome. Such discrepancies could manifest as either an over-issuance or under-issuance of tokens to creators, potentially leading to disputes or dissatisfaction among participants.

Upgradability of Contracts and Consistency Maintenance

Implementation Changes Affecting Integrated Contracts : If ERC20TokenEmitter or CultureIndex is upgraded with changes in their functions or state variables, other contracts relying on them, like AuctionHouse or VerbsToken, might face integration issues. For instance, a changed function signature or new business logic can lead to failed calls or incorrect outcomes.

Data Structure and Storage Inconsistencies: Upgradable contracts must maintain consistent storage structures across versions. Any misalignment in storage structure can cause corrupted state or unpredictable behavior.

Version Dependency: Contracts like AuctionHouse might depend on specific versions of ERC20TokenEmitter or CultureIndex. Upgrades in these contracts without corresponding updates in dependent contracts can lead to compatibility issues.

Non-standard token risks

• If the ERC20TokenEmitter issues tokens that do not fully comply with the ERC-20 standard (for example, by modifying or omitting standard functions like transfer(), approve(), or balanceOf()), it could lead to integration issues with wallets, exchanges, and other DeFi protocols that expect standard ERC-20 compliance. Non-standard behaviors in token transfer, approval mechanisms, or event emissions could disrupt user interactions and lead to unexpected outcomes.

• if the VerbsToken incorporates non-standard features or deviates from the ERC-721 standard (like altering the standard transferFrom() behavior or metadata structure), it may face compatibility issues with NFT marketplaces, wallets, and display platforms. Custom functionalities, while potentially beneficial for unique use cases, might confuse users accustomed to standard ERC-721 behavior and properties.

Software engineering considerations

Security Considerations:

• Ensure that access control mechanisms are well-defined and that only authorized users or contracts can perform sensitive operations. You're using OpenZeppelin's Ownable pattern for some functions, but verify that it covers all necessary access control points.
• Verify the security of signature verification functions (_verifyVoteSignature) thoroughly since this is crucial for preventing unauthorized votes.
• Consider using the latest version of Solidity and OpenZeppelin libraries to benefit from security improvements.

Contract Upgrades:

• Be cautious with contract upgrades, as they can introduce risks if not done carefully. Ensure that the upgrade manager is secure and that the upgrade process has been thoroughly tested.
• Consider implementing a pause mechanism or timelock for upgrades to provide a safety net in case issues are discovered after an upgrade.

Gasless Voting:

• Gasless voting is a complex feature, and the contract should be thoroughly tested for potential edge cases, including scenarios where gasless votes expire.
• Make sure that the gasless voting mechanism provides adequate security to prevent abuse and unauthorized votes.

Testing:

Comprehensive unit tests and integration tests are essential to verify the correctness and security of the contract. Consider using tools like Truffle or Hardhat for testing.

Constants and Magic Numbers:

Avoid using magic numbers directly in the code. Instead, define them as constants with descriptive names to enhance code readability and maintainability.

Consider Refactoring:

If the contract becomes too complex, consider refactoring it into smaller, more modular contracts to improve code readability and maintainability.

Code Documentation:

• Consider adding detailed comments and explanations for complex functions and logic to make the code more understandable for developers who may work on it in the future.
• Ensure that function descriptions, parameter descriptions, and event descriptions are clear and informative.

Error Handling:

The code includes some require statements for input validation, which is good practice. However, it would be helpful to provide informative error messages when these conditions are not met to assist in debugging and troubleshooting.

Testing suite

- What is the overall line coverage percentage provided by your tests?: 88

• An 88% line coverage suggests that the testing suite is quite comprehensive. It indicates that a significant portion of the codebase has been tested, which is crucial for ensuring the reliability and security of the smart contracts in the Revolution Protocol.

• While 88% is a strong coverage metric, it also implies that there is still 12% of the code that isn't covered by tests. This remaining code could contain critical functionalities or edge cases that need to be addressed to ensure the overall robustness of the system

• The uncovered 12% presents an opportunity to identify specific areas or functions within the contracts that might require additional testing. This could include complex logic, edge cases, or newly added features.

Weakspots as per codebase

Complex Interactions in CultureIndex

function voteForMany(uint256[] calldata pieceIds) public nonReentrant {
    _voteForMany(pieceIds, msg.sender);
}

Weak Spot: This function implies complex interactions with multiple art pieces. Such complexity can lead to bugs, especially if interactions aren't carefully managed and tested for all edge cases.

Upgradability and Data Consistency

function _authorizeUpgrade(address _newImpl) internal view override onlyOwner {
    // Upgrade logic
}

Weak Spot: If not managed carefully, upgradable contracts can introduce inconsistencies in data and behavior, leading to potential vulnerabilities or contract failure post-upgrade.

Gas Efficiency in MaxHeap

function maxHeapify(uint256 pos) internal {
    // Heapify logic with multiple swaps
}

Weak Spot: Functions involving multiple array manipulations, like maxHeapify, can consume significant gas, especially with a large heap size. High gas costs can limit usability and scalability.

Absence of Circuit Breakers

// No function for pausing or stopping contract operations in emergencies

Weak Spot: Without mechanisms to pause or halt operations in emergencies, the contract is vulnerable to continued exploitation in the event of a discovered vulnerability.

Data Integrity in CultureIndex

mapping(uint256 => ArtPiece) public pieces;

Weak Spot: Complex data structures require rigorous validation to maintain integrity. Mistakes in data handling can lead to inconsistencies or incorrect application logic.

Time Spend

10 Hours

Time spent:

10 hours