Revolution Protocol - rvierdiiev's results

Lines of code

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

Vulnerability details

Proof of Concept

When piece is created then creationBlock is set to block.number. So all votes will be fetched for that block only during the piece voting. Also quorumVotes is calculated for the piece as percentage from current total supply of voting tokens which are erc20 token and erc721 verbs token. To pass voting, piece should have more than quorumVotes.

AuctionHouse is selling verbs tokens one by one. For each new auction verb token is minted, which increases total supply of erc721 voting token. In case if no one is interested in purchasing, then the token will be burnt.

So as i said, new auction mints new token and increases total voting power. In case if new piece will be created during the auction, then this increased total voting power will be used to calculate quorum. And in case if then this token will be burnt(for example in same block), then actual total supply will be smaller than was calculated for the quorum.

Another thing that i want to say is that in case if piece was created at the moment, when auction was in progress, then even when someone will purchase that token, purchaser will never be able to participate in voting for that token(only if he will settle auction is same block when piece was created), because of creationBlock. This means that usually, ongoing auction's nft will never participate in the piece voting, however it's weight is used in the total supply of erc721.

Exactly same problem exists for the DAO voting. The minted verb token increases totalSupply and thus increases proposalThreshold. But again purchaser will not be able to use that nft for voting on the proposals that were created during the auction.

Impact

The quorum calculation is not correct.

Tools Used

VsCode

Recommended Mitigation Steps

So first of all creationBlock should be block.number - 1(then even if settling was in same block with piece creating, then this nft doesn't count). And i think, that createPiece should not take into account auctioned nft and reduce newPiece.totalVotesSupply with voting power for that nft. The best option will be to not increase totalSupply, when verb is minted for the auction, but only when it's sold.

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

When new piece is created, then it is inserted in the heap with 0 votes. Also creationBlock is set to block.number for that piece. It means that maximum amount that this piece can get is limited to the total supply of voting tokens in the creationBlock.

When someone cast votes for the piece, then votes from creationBlock are fetched as the amount. Then this amount is used to update piece in the heap.

So here is the problem. Suppose that 1 piece was created when total voting supply was 100 and another one is created when total voting supply was 1000. Second piece needs just 10% of total supply to beat first piece. Let's imagine that in some moment first piece gets all 100%, but second piece has 110 votes(just 11%). As result second piece will be at the top of the heap. Now let's also imagine that quorum is 60%, which means that first piece passes it, however second one don't and as result token minting is failing.

Impact

Voting system is not good enough.

Tools Used

VsCode

Recommended Mitigation Steps

I think that instead of storing total votes in the heap, you need to store percentage of total supply at the creation time. So then in mine example, first piece will have 100, while second will have 11 and as result first piece will be selected.

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

When new piece is created, then block.numer is stored as newPiece.creationBlock. This means that during the voting all votes amounts will be fetched for the creationBlock and maximum voting power should be limited to the supply at the moment of creation.

We can also see, that newPiece.quorumVotes amount is also calculated for the piece and is based on the total supply of voting tokens at the current moment. newPiece.quorumVotes will be used to determine if piece can be minted.

The problem is that when createPiece is called, then block is not finished yet and total supply for voting tokens is not finalized for that block as well. This allows user to manipulate quorumVotes amount. They can create piece and increase total supply of voting tokens to be able to pass piece voting even with smaller percentage than quorum.

Impact

Users can make it easier to mint piece and get fees

Tools Used

VsCode

Recommended Mitigation Steps

Use previous block for the voting.

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

Using ERC20TokenEmitter.buyToken function anyone can buy erc20 voting tokens. buyToken function uses vrgda approach to calculate token's price. The goal of vrgda is to sell fixed amount of tokens per unit of time. Depending on the current sold amount in this unit vrgda can decrease or increase price to attract buyers or vice versa.

Because of the described behavior of vrgda it means that price of erc20 token is not stable and changes after each call and time. Thus the function needs slippage protection, so buyer can get at least minimum amount that he is agreed and revert otherwise and not cause loses for the caller.

Impact

No slippage control for the caller.

Tools Used

VsCode

Recommended Mitigation Steps

Add additional param, like minErc20Amount, as slippage protection. Also you can add expire check as well.

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

Using ERC20TokenEmitter.buyToken function anyone can buy erc20 voting tokens. buyToken function uses vrgda approach to calculate tokens price. The goal of vrgda is to sell fixed amount of tokens per unit of time. Depending on the current sold amount in this the unit vrgda can decrease or increase price to attract buyers or vice versa.

buyToken function is payable and msg.value is the amount of eth that buyer is willing to spend for erc20 tokens. Part of this amount is considered as creator payment and entropyRateBps variable determines how much of the creator's payment should be provided in the form of erc20.

Thus totalTokensForCreators param is amount of erc20 tokens that creator will receive. To calculate that amount function calls getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment), which is vrgda call.

Only after creator's tokens were calculated using vrgda, then purchasers tokens amount is calculated. And only after 2 calculations emittedTokenWad is increased.

Note, that emittedTokenWad is the total sold amount that is used by vrgda algorithm. Thus after totalTokensForCreators is calculated, then emittedTokenWad is not increased, which means that vrgda calculation of totalTokensForBuyers will be incorrect.

Impact

erc20 voting tokens are calculated and distributed incorrectly

Tools Used

VsCode

Recommended Mitigation Steps

I think that you need to call getTokenQuoteForEther for the whole amount: treasury funds + entropy of creator and then distribute them.

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

When auction is settled, then creators of piece receive rewards. Part of this rewards are sent in eth and other part of eth is used to buy voting tokens for creators. Amount that should be sent directly depends on entropyRateBps. In case if entropyRateBps is 0, then all rewards should be sent in form of erc20 voting token.

The problem that in this case vrgdaSplits and vrgdaReceivers arrays will not be initialized and as result erc20TokenEmitter.buyToken function will revert, because of this check. As result settling will stuck, until entropyRateBps will be set to bigger value.

Impact

Settling will stuck.

Tools Used

VsCode

Recommended Mitigation Steps

Initialize vrgdaSplits and vrgdaReceivers arrays before if block.

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

CultureIndex contract allows users to sign cast votes messages, so then can be executed by third party using voteForManyWithSig or batchVoteForManyWithSig functions.

Validation of the signed message is done in the _verifyVoteSignature function. This is how message is encoded. voteHash = keccak256(abi.encode(VOTE_TYPEHASH, from, pieceIds, nonces[from]++, deadline));

pieceIds is an array and according to the eip712: https://eips.ethereum.org/EIPS/eip-712#definition-of-encodedata

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). The struct values are encoded recursively as hashStruct(value). This is undefined for cyclical data.

This means that arrays should be additionally encoded which is not done in the contract. As result contract is not eip712 compatible, which means that it will fail to integrate with third party signing solutions.

Impact

Contract is not eip712 compatible.

Tools Used

VsCode

Recommended Mitigation Steps

Pls, check this answer it has links to the correct implementation https://ethereum.stackexchange.com/questions/125105/signing-an-array-whit-eth-signtypeddata-v4

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

AuctionHouse is configured with reservePrice and minBidIncrementPercentage params adn both of them can be 0. While reservePrice is unlikely will be 0, minBidIncrementPercentage can be for sure, as minBidIncrementPercentage == 0 means that auction owner doesn't care what is the min price increment, he only wants each bid to be bigger than previous.

createBid function is used to provide your bid. Several criterias should be met. Bid should be not smaller than reservePrice and bid should be not less than increment previous bid.

In case if minBidIncrementPercentage is set to 0, then it will be enough for next bidder to just send same amount of funds that was sent by previous bidder and he will get the nft.

Impact

Bidder can provide same amount and get the nft.

Tools Used

VsCode

Recommended Mitigation Steps

Make sure that current bid is bigger than previous one.

Assessed type

Error