Platform: Code4rena
Start Date: 13/12/2023
Pot Size: $36,500 USDC
Total HM: 18
Participants: 110
Period: 8 days
Judge: 0xTheC0der
Id: 311
League: ETH
Rank: 39/110
Findings: 3
Award: $158.12
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: jnforja
Also found by: 0x175, 0xCiphky, 0xDING99YA, 0xmystery, ArmedGoose, Aymen0909, Franklin, KupiaSec, McToady, MrPotatoMagic, Ocean_Sky, PNS, Pechenite, TermoHash, Topmark, _eperezok, alexbabits, deth, hals, imare, jeff, ktg, leegh, mahdirostami, marqymarq10, mojito_auditor, neocrao, ptsanev, twcctop, zraxx
1.337 USDC - $1.34
https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L179-L183 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L190-L192
The ability for the owner to modify key auction parameters (timeBuffer
, reservePrice
, minBidIncrementPercentage
) during an ongoing auction introduces a level of unpredictability and potential unfairness in the auction process. This can lead to several issues:
Overall, while the flexibility of parameter adjustment offers operational advantages, its application during live auctions raises significant concerns regarding fairness and trustworthiness.
As a matter of fact, there simply isn't any scenario under which the trusted owner could perform the changes without bricking an ongoing auction because:
whenPaused
visibility.createBid()
would not be governed by whenNotPaused
.The current implementation of the auction contract allows the owner to alter key parameters during active auctions. This can be demonstrated by considering the following scenarios:
timeBuffer
can significantly prolong an auction, potentially frustrating bidders who were prepared for the auction to end sooner.timeBuffer
is initially set to 10 minutes. This means if a bid is placed within the last 10 minutes of the auction, the auction's end time is extended by 10 minutes from the time of the bid.// Extend the auction if the bid was received within `timeBuffer` of the auction end time bool extended = _auction.endTime - block.timestamp < timeBuffer; if (extended) auction.endTime = _auction.endTime = block.timestamp + timeBuffer;
reservePrice
during an auction can lead to a lower final sale price than what might have been achieved, impacting the seller's revenue.reservePrice
is initially set at 1 ETH.reservePrice
to 0.5 ETH after the auction has started but before any bids are placed.require(msg.value >= reservePrice, "Must send at least reservePrice");
minBidIncrementPercentage
in the middle of an auction could discourage further bidding due to the higher cost of each increment, potentially affecting the final auction price.minBidIncrementPercentage
is 5%. This means each new bid must be at least 5% higher than the current highest bid.require( msg.value >= _auction.amount + ((_auction.amount * minBidIncrementPercentage) / 100), "Must send more than last bid by minBidIncrementPercentage amount" );
In each scenario, the unpredictable alteration of auction rules creates an environment of uncertainty and potential unfairness.
Manual
Lock Parameters on Auction Start: Once an auction begins, lock the key parameters (timeBuffer
, reservePrice
, minBidIncrementPercentage
) so they cannot be changed until the auction concludes.
Timing
#0 - c4-pre-sort
2023-12-22T00:42:50Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2023-12-22T00:43:35Z
raymondfam marked the issue as duplicate of #55
#2 - c4-pre-sort
2023-12-24T14:17:54Z
raymondfam marked the issue as duplicate of #495
#3 - c4-judge
2024-01-06T18:14:50Z
MarioPoneder changed the severity to QA (Quality Assurance)
#4 - c4-judge
2024-01-07T16:03:01Z
MarioPoneder marked the issue as grade-c
#5 - c4-judge
2024-01-10T17:32:52Z
This previously downgraded issue has been upgraded by MarioPoneder
#6 - c4-judge
2024-01-10T17:33:13Z
MarioPoneder marked the issue as duplicate of #515
#7 - c4-judge
2024-01-10T17:35:11Z
MarioPoneder marked the issue as partial-50
#8 - c4-judge
2024-01-11T18:03:12Z
MarioPoneder changed the severity to 2 (Med Risk)
🌟 Selected for report: deepplus
Also found by: 0xDING99YA, 0xmystery, Aymen0909, DanielArmstrong, Inference, KupiaSec, SadeeqXmosh, SpicyMeatball, Tricko, adeolu, jnforja, passteque, rvierdiiev, wangxx2026, zhaojie
25.1638 USDC - $25.16
The lack of slippage and deadline protection mechanisms in the ERC20TokenEmitter.buyToken()
, operating under a Continuous Variable Rate Gradual Dutch Auction (VRGDAC) model, poses a moderate to high risk. This risk primarily stems from the potential financial loss or unexpected outcomes for users due to the volatile and dynamic pricing nature of the VRGDAC system. In a blockchain environment, where transaction times can be unpredictable and chain reorganizations (reorgs) are possible, the absence of these protections can lead to scenarios where users receive a significantly different amount of tokens than expected or their transactions are executed at an inopportune time. This could result in reduced trust and reliability in the system, potentially impacting its adoption and usage.
Volatility of Token Pricing: Given the VRGDAC model's time-sensitive pricing, the price at which a user initiates a transaction might significantly differ from the execution price. Specifically, if the emission is ahead of schedule, the price increases exponentially. This can be especially problematic in networks with high congestion or variable transaction times.
Chain Reorgs Impact: In the event of a chain reorg (Base and Optimism L2s), transactions might be rolled back or altered. Since the VRGDAC price depends on the specific block time and the history of sales up to that point, a reorg could lead to a transaction being executed with a less favorable token amount.
function buyToken( address[] calldata addresses, uint[] calldata basisPointSplits, ProtocolRewardAddresses calldata protocolRewardsRecipients ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
tokensSoldWad
.Here's a common instance entailed when AuctionHouse._settleAuction()
externally calls erc20TokenEmitter.buyToken()
:
//Buy token from ERC20TokenEmitter for all the creators if (creatorsShare > ethPaidToCreators) { creatorTokensEmitted = erc20TokenEmitter.buyToken{ value: creatorsShare - ethPaidToCreators }( vrgdaReceivers, vrgdaSplits, IERC20TokenEmitter.ProtocolRewardAddresses({ builder: address(0), purchaseReferral: address(0), deployer: deployer }) ); }
It's understandable zero slippage may be entailed here so that AuctionHouse._settleAuction()
will not revert due to minimum creatorTokensEmitted
not fulfilled. But the slippage option should be extended to all other users buying the tokens directly.
Manual
Implement Slippage Protection: Introduce a parameter in the buyToken function allowing users to specify a maximum acceptable slippage percentage. The contract should then ensure that the price at execution does not deviate beyond this limit from the price at initiation.
Introduce Deadline Protection: Add a deadline parameter to transactions, ensuring they are only valid until a specific block time or timestamp. If the transaction is not processed by this deadline, it fails, preventing execution at an unexpected or unfavorable time.
Timing
#0 - c4-pre-sort
2023-12-22T00:27:37Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2023-12-22T00:28:04Z
raymondfam marked the issue as duplicate of #26
#2 - c4-pre-sort
2023-12-24T06:00:41Z
raymondfam marked the issue as duplicate of #397
#3 - c4-judge
2024-01-06T16:29:09Z
MarioPoneder marked the issue as satisfactory
🌟 Selected for report: 0xmystery
Also found by: 00xSEV, 0xCiphky, 0xDING99YA, 0xhitman, ABAIKUNANBAEV, Aamir, BARW, IllIllI, King_, MrPotatoMagic, Pechenite, SovaSlava, SpicyMeatball, Topmark, Ward, ZanyBonzy, ast3ros, bart1e, cheatc0d3, deth, developerjordy, hals, imare, kaveyjoe, ktg, leegh, osmanozdemir1, peanuts, roland, rvierdiiev, shaka, sivanesh_808, spacelord47
131.6158 USDC - $131.62
protocolRewardsRecipients
to receive self-rebatesThe ERC20TokenEmitter.buyToken()
function is designed in such a way that the caller (or buyer) can fully control the protocolRewardsRecipients
parameter, which includes the builder
, purchaseReferral
, and deployer
addresses. This design allows the caller to potentially assign all these addresses to themselves.
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 );
Given this setup, if a caller assigns all three addresses (builder, purchaseReferral, deployer) to themselves, they could indeed benefit from the rewards percentages assigned to each of these roles. In the code, the basis points (BPS) for each reward are defined as constants:
When summed up, these total to 1.75% (175 BPS). So, if the buyer sets themselves as the recipient for all these rewards, they would effectively be receiving a 1.75% reward on their purchase value.
This could be a feature or a vulnerability, depending on the intended use and design of the contract:
To address this, if it's deemed a vulnerability, the smart contract could be updated to include checks that prevent the same address from being used for all these roles or to implement a more robust system for assigning these rewards. This requires careful consideration of the contract's intended economics and security implications.
minCreatorRateBps
has been set too highAuctionHouse.setMinCreatorRateBps()
require new min rate cannot be lower than previous min rate:
//ensure new min rate cannot be lower than previous min rate require( _minCreatorRateBps > minCreatorRateBps, "Min creator rate must be greater than previous minCreatorRateBps" );
If it's has been set too high, whether deliberately or accidentally, there's no way to drop the threshold. Hence, this could affect future setting of a new _creatorRateBps
:
require( _creatorRateBps >= minCreatorRateBps, "Creator rate must be greater than or equal to minCreatorRateBps" );
The auction contract, as currently designed, presents a a low to medium severity issue due to its allowance for bids of 0 ETH under specific conditions. When the _createAuction()
function initializes an auction, it sets auction.amount
to 0 ETH. Combined with a reserve price
also set at 0 ETH, this configuration allows the first bid to be 0 ETH, which satisfies the contract's require statements for both the reserve price and the minimum bid increment.
require(msg.value >= reservePrice, "Must send at least reservePrice"); require( msg.value >= _auction.amount + ((_auction.amount * minBidIncrementPercentage) / 100), "Must send more than last bid by minBidIncrementPercentage amount" );
Subsequent bids can also be 0 ETH, maintaining the same auction amount. This design flaw could lead to an auction proceeding and closing without any financial transaction, contradicting the fundamental purpose of an auction to facilitate competitive bidding and sell items for the highest possible price. Implementing safeguards such as a non-zero minimum reserve price, a required minimum first bid, or dynamic bid increments would be essential to ensure the auction's functionality and integrity.
The auction extension mechanism in Ethereum-based smart contracts, particularly when combined with the minBidIncrementPercentage
, presents a low to medium severity issue due to its interaction with the dynamics of Ethereum transactions, including frontrunning and mempool observation. Designed to prevent sniping, the extension mechanism ensures fairness by allowing bids within a timeBuffer
to prolong the auction.
bool extended = _auction.endTime - block.timestamp < timeBuffer; if (extended) auction.endTime = _auction.endTime = block.timestamp + timeBuffer;
However, this can lead to unpredictability in auction closure, potentially deterring bidders with strict budgets or timelines. Moreover, the visibility of transactions in the mempool before confirmation enables frontrunning, where opportunistic bidders can outbid others by observing their transactions. This scenario can result in auctions closing at lower bids than potentially achievable, as illustrated in the example where an auction expected to close at 11 ETH ends at 10.5 ETH due to frontrunning and extension.
For example, if the current _auction.amount
is 10 ETH, and minBidIncrementPercentage
is 5%. Alice in the last moment attempts to bid 11 ETH. Bob, seeing this in the mempool, frontruns with 10.5 ETH. Alice's call is denied, as 10.5 x 1.05 = 11.025 ETH. bool extended
is turned on and the auction is extended for a few minutes (timeBuffer
) but Alice is no longer interested since her budget is 11 ETH. An auction could have been sold for 11 ETH ends up with 10.5 ETH.
The strategic behavior of bidders may also evolve, leading to less transparent and more complex bidding processes. Mitigating these issues could involve implementing secret bids with a reveal phase, employing anti-frontrunning techniques, or adjusting the minBidIncrementPercentage
dynamically. While these measures aim to balance fairness, predictability, and transactional efficiency, they also introduce their own complexities and trade-offs, making this a nuanced issue requiring careful consideration in smart contract design.
In the createBid
function of the AuctionHouse
smart contract, the minBidIncrementPercentage
parameter, which sets the minimum bid increment in whole percentage points, could significantly impact the auction dynamics, especially in the context of a high Ether value.
require( msg.value >= _auction.amount + ((_auction.amount * minBidIncrementPercentage) / 100), "Must send more than last bid by minBidIncrementPercentage amount" );
Apparently, the denominator of 100
signifies the lowest minBidIncrementPercentage amount it could go is 1%.
As the worth of Ether increases, a minimum 1% bid increment translates into a larger absolute monetary value, potentially discouraging smaller bidders due to the higher financial commitment required for each subsequent bid. This reduced bid granularity could lead to fewer overall bids and might result in bidders overcommitting, as they are forced to raise their bids by at least this minimum percentage. The situation could alter the strategic approach to bidding, with participants possibly engaging in last-minute bidding wars or being deterred from participating if the increments exceed their budget constraints.
To ensure a balanced and accessible auction environment, considering a more flexible increment system, such as smaller percentage increments (via the adoption of BPS, e.g. 10_000 is equivalent to 100%) or a maximum increment cap in Ether, might be beneficial to accommodate varying Ether values and maintain efficient market pricing.
In the _settleAuction
function of the AuctionHouse
smart contract, setting the builder
and purchaseReferral
fields to address(0)
within the IERC20TokenEmitter.ProtocolRewardAddresses
struct raises important considerations.
IERC20TokenEmitter.ProtocolRewardAddresses({ builder: address(0), purchaseReferral: address(0), deployer: deployer })
This design choice might align with the contract's current requirements if no rewards are intended for builders or referrers. However, it potentially limits future flexibility and adaptability to new features. While it might offer gas savings, the implications on the contract's functionality and the ecosystem should be carefully evaluated. Moreover, such hardcoding should be transparently documented for clarity and understanding. The use of upgradeable contract patterns provides some leeway for future modifications, but it's crucial to balance immediate simplicity against long-term contract evolution and security.
As communities involved in cultural indexing and art piece voting grow, they face the challenge of declining active voter participation, which can hinder critical processes such as achieving quorum.
require(totalVoteWeights[piece.pieceId] >= piece.quorumVotes, "Does not meet quorum votes to be dropped.");
This trend, often due to factors like reduced interest or lack of awareness, necessitates strategic solutions. The protocol can boost engagement through improved communication, incentives for participation, and streamlined voting processes. Additionally, adapting quorum requirements, i.e. dynamic quorum adjustment
to reflect actual participation rates and carefully balancing tokenomics to avoid concentrated voting power are crucial. These measures, coupled with a focus on fostering long-term community involvement, are key to ensuring that every member feels their contribution is both meaningful and impactful in the evolving landscape of decentralized governance.
In the getArtPieceById
function within the VerbsToken
smart contract, the condition require(verbId <= _currentVerbId, "Invalid piece ID")
should ideally use <
instead of <=
. This is because _currentVerbId
is post-incremented in the _mintTo
function,
uint256 verbId = _currentVerbId++;
which means that at any given point, _currentVerbId
represents the next ID to be assigned, not an ID that has already been assigned to an existing NFT.
When the _mintTo
function is called, it increments _currentVerbId
after assigning the current ID to a new NFT. Therefore, the highest valid verbId
at any moment is _currentVerbId - 1
. If getArtPieceById
is called with verbId
equal to _currentVerbId
, it refers to an ID that has not yet been assigned to an NFT, leading to a potential reference to a non-existent art piece.
Consider making the following change to ensure that the function only processes requests for IDs that have already been assigned to minted NFTs, thus maintaining the integrity of the function and avoiding potential errors or unexpected behavior.
function getArtPieceById(uint256 verbId) public view returns (ICultureIndex.ArtPiece memory) { - require(verbId <= _currentVerbId, "Invalid piece ID"); + require(verbId < _currentVerbId, "Invalid piece ID"); return artPieces[verbId]; }
VerbsToken
Smart ContractThe VerbsToken
smart contract contains two critical functions, setContractURIHash and setDescriptor, that pose potential risks due to their ability to alter contract-level and individual NFT metadata, respectively.
The setContractURIHash
function, with a low to medium severity level, allows changing the collection-level metadata, which could impact the overall perception and value of the NFTs in the market. This might lead to confusion or mistrust among NFT owners and potential buyers if the collection's description or theme is altered significantly.
On the other hand, the setDescriptor
function poses a high-severity risk, as it directly affects the tokenURI
of each NFT. Changes made by this function can be substantial as the tokenURI
typically points to a JSON file that contains the NFTs' appearance and features, potentially compromising their originality and authenticity. This could have severe implications for the NFT's value and the owner's rights.
The presence of a lockDescriptor function, which irreversibly prevents further changes to the descriptor, shows an awareness of the potential risks associated with changing NFT metadata. However, the absence of a similar lock function for the contractURIHash
indicates a different level of consideration for the collection-level metadata compared to the individual NFT metadata.
To mitigate these risks, it is recommended to implement immutable metadata practices, enhance transparency and community involvement in any changes, provide clear documentation, and introduce a versioning system for metadata.
ECDSA.recover
over ecrecover
One of the most critical aspects to note about ecrecover
is its vulnerability to malleable signatures. This means that a valid signature can be transformed into a different valid signature without needing access to the private key. Where possible, adopt ECDSA.recover
as commented by the imported EIP712Upgradeable
in CultureIndex.sol.
/** * @dev Given an already https://eips.ethereum.org/EIPS/eip-712#definition-of-hashstruct[hashed struct], this * function returns the hash of the fully encoded EIP712 message for this domain. * * This hash can be used together with {ECDSA-recover} to obtain the signer of a message. For example: * * ```solidity * bytes32 digest = _hashTypedDataV4(keccak256(abi.encode( * keccak256("Mail(address to,string contents)"), * mailTo, * keccak256(bytes(mailContents)) * ))); * address signer = ECDSA.recover(digest, signature); * ``` */ function _hashTypedDataV4(bytes32 structHash) internal view virtual returns (bytes32) { return MessageHashUtils.toTypedDataHash(_domainSeparatorV4(), structHash); }
Here's a specific instance entailed:
address recoveredAddress = ecrecover(digest, v, r, s);
CultureIndex.hasVoted
:
/** * @notice Checks if a specific voter has already voted for a given art piece. * @param pieceId The ID of the art piece. * @param voter The address of the voter. * @return A boolean indicating if the voter has voted for the art piece. */ function hasVoted(uint256 pieceId, address voter) external view returns (bool) { return votes[pieceId][voter].voterAddress != address(0); }
could have been used in the following require statement:
- require(!(votes[pieceId][voter].voterAddress != address(0)), "Already voted"); + require(!(hasVoted(pieceId, voter)), "Already voted");
On https://www.desmos.com/calculator/im67z1tate, the integral of price is supposed to be p(x) = p0 * (1 - k)^(t - x/r)
. However, the exponent varies on the formula adopted by the protocol:
// given # of tokens sold, returns integral of price p(x) = p0 * (1 - k)^(x/r) ## [NC-01] Typo mistakes https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/MaxHeap.sol#L64 ```diff - /// @notice Struct to represent an item in the heap by it's ID + /// @notice Struct to represent an item in the heap by its ID
- /// @notice Struct to represent an item in the heap by it's ID mapping(uint256 => uint256) public heap; + /// @notice Mapping to represent an item in the heap by it's ID mapping(uint256 => uint256) public heap;
- // Ensure to address is not 0 if (from == address(0)) revert ADDRESS_ZERO(); + // Ensure from address is not 0 if (from == address(0)) revert ADDRESS_ZERO();
Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.
module.exports = { solidity: { version: "0.8.22", settings: { optimizer: { enabled: true, runs: 1000, }, }, }, };
Please visit the following site for further information:
https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler
Here's one example of instance on opcode comparison that delineates the gas saving mechanism:
for !=0 before optimization PUSH1 0x00 DUP2 EQ ISZERO PUSH1 [cont offset] JUMPI after optimization DUP1 PUSH1 [revert offset] JUMPI
Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.
#0 - raymondfam
2023-12-24T17:55:10Z
Possible upgrades:
L-01 --> #342
#1 - c4-pre-sort
2023-12-24T18:03:43Z
raymondfam marked the issue as high quality report
#2 - rocketman-21
2024-01-03T20:01:45Z
self rebates are an expected downside of the rewards system for now
#3 - rocketman-21
2024-01-03T20:02:00Z
solid minor cleanup report though
#4 - c4-sponsor
2024-01-03T20:02:05Z
rocketman-21 (sponsor) acknowledged
#5 - MarioPoneder
2024-01-07T19:03:20Z
Possible upgrades:
L-01 --> #342
#342 is now QA.
#6 - c4-judge
2024-01-07T20:02:17Z
MarioPoneder marked the issue as grade-a
#7 - MarioPoneder
2024-01-07T20:49:49Z
As always, the perfect QA report would be a combination of the best, therefore I also want to mention #549 and #541.
This one was selected for report not only because of the amount of Low
findings, but also because of the overall value provided by the in-depth elaborations.
Annotations:
L-01: Intended behaviour, but still valid to point out as NC
in a QA report
L-10: Is NC
because only 1 of 2 signatures could ever be used due to nonce
.
Otherwise I agree with the remaining findings and the assessed severities.
#8 - c4-judge
2024-01-07T20:49:55Z
MarioPoneder marked the issue as selected for report