| Audit Date | 11/16/2023 - 11/23/2023 |
| Auditor | HollaDieWaldfee (@HollaWaldfee100) |
| Version 1 | 11/23/2023 Initial Report |
| Version 2 | 12/11/2023 Mitigation Review |
- Audit Report - Naffles
- Contents
- Disclaimer
- About HollaDieWaldfee
- Scope
- Centralization Risks
- Severity Classification
- Summary
- Findings
- High Risk Findings (5)
- 1. Anyone can call
L2NaffleBase.setWinner()and steal assets - 2. Naffle participation is possible after random number has been selected, allowing to manipulate Naffle outcome and steal assets
- 3. Re-used Open Entry Tickets can be reset, causing inconsistent state and clearing Naffle participation / allowing to use an open entry ticket for unlimited number of Naffles
- 4. L2 -> L1 message can be executed with wrong function, resulting in loss of assets
- 5. Naffle owners can drain funds from L2 Naffle contract by front-running
VRFManager
- 1. Anyone can call
- Medium Risk Findings (2)
- Low Risk Findings (3)
- Improvement Findings (7)
- 11. Add
removeAdmin()function - 12. Add additional
viewfunctions for storage variables - 13. Use OZ library to check ECDSA signature to prevent signature malleability
- 14. Remove unused storage variables
- 15. Remove redundant
L2OpenEntryTicketView.getTotalSupply()function - 16. Remove
minL2ForwardedGasForCreateNafflerequirement - 17. Allow Naffle creators to prove failed L1->L2 transaction
- 11. Add
- High Risk Findings (5)
The following smart contract audit report is based on the information and code provided by the client, and any findings or recommendations are made solely on the basis of this information. While the Auditor has exercised due care and skill in conducting the audit, it cannot be guaranteed that all issues have been identified and that there are no undiscovered errors or vulnerabilities in the code.
Furthermore, this report is not an endorsement or certification of the smart contracts, and the Auditor does not assume any responsibility for any losses or damages that may result from the use of the smart contracts, either in their current form or in any modified version thereof.
HollaDieWaldfee is a top ranked Smart Contract Auditor performing audits on code4rena and Sherlock. He has placed 1st in multiple audit contests.
On Sherlock he uses the handle "roguereddwarf" to compete in audit contests.
He can also be booked for conducting Private Audits.
Contact:
Twitter/X: @HollaWaldfee100
The audit has been conducted on the code in the client's GitHub repository (https://github.com/Naffles/naffles-smart-contracts) at commit 96961659e9d160a93712028297a044f5486c3df8.
The following source files are in scope:
- L1NaffleAdmin.sol
- L1NaffleBase.sol
- L1NaffleBaseInternal.sol
- L1NaffleBaseStorage.sol
- L1NaffleDiamond.sol
- L1NaffleView.sol
- L2NaffleAdmin.sol
- L2NaffleBase.sol
- L2NaffleBaseInternal.sol
- L2NaffleBaseStorage.sol
- L2NaffleDiamond.sol
- L2NaffleView.sol
- L2OpenEntryTicketAdmin.sol
- L2OpenEntryTicketBase.sol
- L2OpenEntryTicketBaseInternal.sol
- L2OpenEntryTicketDiamond.sol
- L2OpenEntryTicketStorage.sol
- L2OpenEntryTicketView.sol
- L2PaidTicketAdmin.sol
- L2PaidTicketBase.sol
- L2PaidTicketBaseInternal.sol
- L2PaidTicketDiamond.sol
- L2PaidTicketStorage.sol
- L2PaidTicketView.sol
This is not an audit of the entire codebase.
The purpose of this audit is to check the most important contracts with a focus on their business logic and Ethereum <-> zkSync Era cross-chain communication.
The mitigation review has been conducted at commit bb4c45873e233087ba9549dcded09c7081ea350a.
All the in scope contracts are deployed using the Diamond Proxy pattern.
With regards to Centralization Risks this means that there exists an owner of the Diamond Proxy that can change its logic at any time.
By being able to change the contract logic, the owner has full control over the assets that are owned by the contracts.
Beyond the owner that has full control over all the contracts, there also exists a admin role in all of the contracts. The admin is able to perform privileged actions such as setting parameters. If the admin abuses his privileges he can achieve similar outcomes as the owner, even though less directly so.
E.g., the admin can mint as many tickets as he likes or assign a VRFManager and set himself as the winner of Naffles, allowing him to steal the Naffle assets.
Lastly, there exists the VRFManager role that is supposed to operate the NaffleVRF contract that is deployed on Polygon and commit the random numbers and winners to zkSync Era by calling L2NaffleBase.setWinner(). By providing wrong winners, the VRFManager is able to steal Naffle assets as well.
In summary, all three privileged roles (owner, admin, VRFManager) must be fully trusted.
In contrast to the above three roles, Naffle owners should not have to be fully trusted. They should not be able to steal funds from users or impair the functionality of the protocol otherwise. Due to some of the vulnerabilities described here, Naffle owners are not properly restricted.
By implementing the mitigations, Naffle owners are no longer able to cheat on users and they can only impact the Naffles that they themselves manage (either cancelling a Naffle or drawing a winner early).
| Severity | Impact: High | Impact: Medium | Impact: Low |
|---|---|---|---|
| Likelihood: High |  |
|
 |
| Likelihood: Medium | |
|
 |
| Likelihood: Low | |
|
|
Impact - the technical, economic and reputation damage of a successful attack
Likelihood - the chance that a particular vulnerability is discovered and exploited
| Severity | Total | Fixed | Partially Fixed | Acknowledged | Disputed | Reported |
|---|---|---|---|---|---|---|
|
5 | 5 | 0 | 0 | 0 | 0 |
|
2 | 2 | 0 | 0 | 0 | 0 |
|
3 | 1 | 0 | 2 | 0 | 0 |
|
7 | 7 | 0 | 0 | 0 | 0 |
Description:
The L2NaffleBase.setWinner() function does not have any access controls.
It should only be callable by the VRFManager role.
Impact:
The L2NaffleBase.setWinner() function can be called for any Naffle. Since the function is supposed to be called by the VRFManager, there are no sanity checks and the winner is directly bridged to Ethereum where the token in the Naffle is sent to the winner via the L1NaffleBaseInternal._setWinnerAndTransferPrize() function.
As soon as a Naffle is created and the Naffle prize is transferred into the L1 Naffle contract, it can be stolen by anyone.
Recommendation:
The fix is to restrict the L2NaffleBase.setWinner() function to the VRFManager role:
diff --git a/contracts/naffle/zksync/L2NaffleBase.sol b/contracts/naffle/zksync/L2NaffleBase.sol
index 1eed0054..e18f527f 100644
--- a/contracts/naffle/zksync/L2NaffleBase.sol
+++ b/contracts/naffle/zksync/L2NaffleBase.sol
@@ -62,7 +62,7 @@ contract L2NaffleBase is IL2NaffleBase, L2NaffleBaseInternal, AccessControl {
uint256 _randomNumber,
address _winner,
uint256 _platformDiscountInPercent
- ) external returns (bytes32) {
+ ) external onlyRole(VRF_ROLE) returns (bytes32) {
return _setWinner(
_naffleId,
_randomNumber,Mitigation Review:
The recommendation has been implemented.
2. Naffle participation is possible after random number has been selected, allowing to manipulate Naffle outcome and steal assets 
Description:
The winner of a Naffle is randomly selected. A random number is requested from Chainlink VRF on Polygon and needs to be provided by the VRFManager when calling L2NaffleBaseInternal._setWinner().
The issue is that while the random number is requested, users can still enter the Naffle.
So they can wait for the random number to be chosen and front-run the VRFManager by calling L2NaffleBaseInternal._buyTickets() or L2NaffleBaseInternal._useOpenEntryTickets().
Assume we have an UNLIMITED Naffle with currently 19 tickets. Assume further that the random number is committed on the Polygon chain but the VRFManager has not yet called L2NaffleBaseInternal._setWinner().
An attacker can precalculate based on the formula that selects the winning ticket id, whether it's possible to exploit the knowledge of the random number:
uint256 winningTicketId = _randomNumber % (naffle.numberOfPaidTickets + naffle.numberOfOpenEntries) + 1;Assume the random number is 400. The attacker can calculate that when he buys 8 more tickets, the total number of tickets will be 27. In this case the attacker will win since:
uint256 winningTicketId = 400 % 27 + 1 = 23
Impact:
Naffles can be entered while already knowing the random number. Thereby one can precalculate who the winner will be and make oneself the winner by increasing the ticket count.
This is not always possible (e.g., when the maximum number of paid and open entry tickets is already reached in a STANDARD Naffle) but it's still a profitable attack in some scenarios.
Recommendation:
It must not be possible to enter a Naffle once the random number has been requested.
Also, it must not be possible for the Naffle owner to cancel the Naffle while a winner is selected to prevent the following strategy:
- if owner ticket wins, continue Naffle
- if owner ticket loses, cancel Naffle
In both cases, the owner gets all funds (his own, and the funds from unsuspecting users) AND the token.
Accordingly, the fix is to restrict access to the L2NaffleBaseInternal._buyTickets(), L2NaffleBaseInternal._useOpenEntryTickets() and L2NaffleBaseInternal._ownerCancelNaffle() functions:
diff --git a/contracts/naffle/zksync/L2NaffleBaseInternal.sol b/contracts/naffle/zksync/L2NaffleBaseInternal.sol
index 803497ab..0b494dc9 100644
--- a/contracts/naffle/zksync/L2NaffleBaseInternal.sol
+++ b/contracts/naffle/zksync/L2NaffleBaseInternal.sol
@@ -75,6 +75,10 @@ abstract contract L2NaffleBaseInternal is IL2NaffleBaseInternal, AccessControlIn
L2NaffleBaseStorage.Layout storage layout = L2NaffleBaseStorage.layout();
NaffleTypes.L2Naffle storage naffle = layout.naffles[_naffleId];
+ if (layout.naffleRandomNumberRequested[_naffleId] == true) {
+ revert SelectingWinner(_naffleId);
+ }
+
if (naffle.naffleTokenInformation.tokenAddress == address(0)) {
revert InvalidNaffleId(_naffleId);
}
@@ -168,6 +172,10 @@ abstract contract L2NaffleBaseInternal is IL2NaffleBaseInternal, AccessControlIn
L2NaffleBaseStorage.Layout storage layout = L2NaffleBaseStorage.layout();
NaffleTypes.L2Naffle storage naffle = layout.naffles[_naffleId];
+ if (layout.naffleRandomNumberRequested[_naffleId] == true) {
+ revert SelectingWinner(_naffleId);
+ }
+
if (naffle.naffleTokenInformation.tokenAddress == address(0)) {
revert InvalidNaffleId(_naffleId);
}
@@ -236,6 +244,10 @@ abstract contract L2NaffleBaseInternal is IL2NaffleBaseInternal, AccessControlIn
L2NaffleBaseStorage.Layout storage layout = L2NaffleBaseStorage.layout();
NaffleTypes.L2Naffle storage naffle = layout.naffles[_naffleId];
+ if (layout.naffleRandomNumberRequested[_naffleId] == true) {
+ revert SelectingWinner(_naffleId);
+ }
+
if (naffle.owner != msg.sender) {
revert NotAllowed();
}Mitigation Review:
When a random number has been requested, naffle.status is set to NaffleTypes.NaffleStatus.SELECTING_WINNER.
The L2NaffleBaseInternal._buyTickets() and L2NaffleBaseInternal._useOpenEntryTickets() functions can only be called in the ACTIVE or POSTPONED state and so they can't be called when the status is SELECTING_WINNER.
The L2NaffleBaseInternal._ownerCancelNaffle() function now checks for the SELECTING_WINNER status and reverts in this case.
3. Re-used Open Entry Tickets can be reset, causing inconsistent state and clearing Naffle participation / allowing to use an open entry ticket for unlimited number of Naffles
Description:
Tickets for cancelled Naffles can be refunded using the L2NaffleBaseInternal._refundTicketsForNaffle() function.
The issue exists in the L2OpenEntryTicketBaseInternal._detachFromNaffle() function that is called downstream.
The entry ticket is not removed from the naffleIdTicketIdOnNaffleTicketId mapping which means the refund can be executed multiple times, resetting the ticket every time:
ticket.naffleId = 0;
ticket.ticketIdOnNaffle = 0;This can be done even when the open entry ticket has already been re-attached to another Naffle. As a result, the same open entry ticket can be used as an entry in the naffleIdTicketIdOnNaffleTicketId mapping for multiple ongoing Naffles.
Impact:
There are three potential ways to exploit this:
- detach an open entry ticket that is owned by someone else and has been re-attached to a new Naffle
- use one's own open entry ticket to participate in multiple Naffles at the same time
- reuse an open entry from a losing Naffle
Which impact can be realized depends on how the VRFManager queries the data of the Naffles and their attached tickets.
Presumably he simply uses the L2OpenEntryTicketView.getOwnerOfNaffleTicketId() function.
In this case, impact 2) and 3) can be realized while 1) can't.
If instead the content of the ticket itself is checked with the L2OpenEntryTicketView.getOpenEntryTicketById() function, impact 1) and 3) can be realized, while 2) can't.
Recommendation:
When an open entry ticket is detached from a Naffle, the entry in the naffleIdTicketIdOnNaffleTicketId mapping must be removed. Thereby it's ensured that the ticket can't be detached from the same Naffle again, triggering a revert instead:
diff --git a/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketBaseInternal.sol b/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketBaseInternal.sol
index 5b6912a0..e2189976 100644
--- a/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketBaseInternal.sol
+++ b/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketBaseInternal.sol
@@ -67,6 +67,7 @@ abstract contract L2OpenEntryTicketBaseInternal is IL2OpenEntryTicketBaseInterna
for (uint i = 0; i < length; ++i) {
uint256 ticketId = l.naffleIdTicketIdOnNaffleTicketId[_naffleId][_ticketIdsOnNaffle[i]];
+ l.naffleIdTicketIdOnNaffleTicketId[_naffleId][_ticketIdsOnNaffle[i]] = 0;
NaffleTypes.OpenEntryTicket storage ticket = l.openEntryTickets[ticketId];
if (ticketId == 0) {Mitigation Review:
The recommendation has been implemented.
Description:
The L2NaffleDiamond sends two types of messages to L1.
One is a message to cancel a Naffle.
The other is a message to set the winner of a Naffle.
The problem is that both types of messages can be executed with either of the functions on L1.
The L1NaffleBase.consumeSetWinnerMessage() and L1NaffleBase.consumeCancelMessage() functions rely on abi.decode to revert when the amount of encoded arguments doesn't match the amount of arguments that is decoded.
However, abi.decode does not revert in these cases.
Impact:
A message to set a winner can be executed with the L1NaffleBase.consumeCancelMessage() function to cancel the Naffle.
And a message to cancel a Naffle can be executed with the L1NaffleBase.consumeSetWinnerMessage() function to set the winner for the Naffle.
In the first case, there is less damage because the token in the Naffle is simply returned to the owner (even though users couldn't get a refund on L2).
In the second case, the token in the Naffle is sent to a random address, i.e., the token is lost.
Recommendation:
Check the first argument of the decoded message and ensure it matches either cancel or setWinner..
diff --git a/contracts/naffle/ethereum/L1NaffleBase.sol b/contracts/naffle/ethereum/L1NaffleBase.sol
index 7500964f..6864afe0 100644
--- a/contracts/naffle/ethereum/L1NaffleBase.sol
+++ b/contracts/naffle/ethereum/L1NaffleBase.sol
@@ -52,7 +52,8 @@ contract L1NaffleBase is IL1NaffleBase, L1NaffleBaseInternal, AccessControl, IER
_message,
_proof
);
- (, uint256 naffleId, address winner) = abi.decode(_message, (string, uint256, address));
+ (string memory action, uint256 naffleId, address winner) = abi.decode(_message, (string, uint256, address));
+ require(keccak256(abi.encodePacked(action)) == keccak256(abi.encodePacked("setWinner")));
_setWinnerAndTransferPrize(naffleId, winner);
}
@@ -75,7 +76,8 @@ contract L1NaffleBase is IL1NaffleBase, L1NaffleBaseInternal, AccessControl, IER
_message,
_proof
);
- (, uint256 naffleId) = abi.decode(_message, (string, uint256));
+ (string memory action, uint256 naffleId) = abi.decode(_message, (string, uint256));
+ require(keccak256(abi.encodePacked(action)) == keccak256(abi.encodePacked("cancel")));
_cancelNaffle(naffleId);
}An even more efficient solution would be to encode the message type as an enum instead of a string.
enum ActionType {
CANCEL,
SET_WINNER
}Mitigation Review:
The recommendation has been implemented.
Message types are now encoded via the ActionType enum.
A further optimization is to remove abi encoding and hashing the action value as that was only necessary for strings.
diff --git a/contracts/naffle/ethereum/L1NaffleBase.sol b/contracts/naffle/ethereum/L1NaffleBase.sol
index 34b21d22..5b458d41 100644
--- a/contracts/naffle/ethereum/L1NaffleBase.sol
+++ b/contracts/naffle/ethereum/L1NaffleBase.sol
@@ -69,7 +69,7 @@ contract L1NaffleBase is IL1NaffleBase, L1NaffleBaseInternal, AccessControl, IER
_proof
);
(NaffleTypes.ActionType action, uint256 naffleId, address winner) = abi.decode(_message, (NaffleTypes.ActionType, uint256, address));
- require(keccak256(abi.encodePacked(action)) == keccak256(abi.encodePacked(NaffleTypes.ActionType.SET_WINNER)));
+ require(action == NaffleTypes.ActionType.SET_WINNER);
_setWinnerAndTransferPrize(naffleId, winner);
}
@@ -93,7 +93,7 @@ contract L1NaffleBase is IL1NaffleBase, L1NaffleBaseInternal, AccessControl, IER
_proof
);
(NaffleTypes.ActionType action, uint256 naffleId) = abi.decode(_message, (NaffleTypes.ActionType , uint256));
- require(keccak256(abi.encodePacked(action)) == keccak256(abi.encodePacked(NaffleTypes.ActionType.CANCEL)));
+ require(action == NaffleTypes.ActionType.CANCEL);
_cancelNaffle(naffleId);
}
/**This has been implemented in 5bf887822bac8df771ef8aaf674617f06bea9f0d.
Description:
It is possible for the L2NaffleBaseInternal._setWinner() function to be called even if the Naffle has been previously cancelled.
The attack is performed by executing the following steps:
- The attacker creates a
STANDARDNaffle and buys some (but not all, to be able to cancel the Naffle) of the paid tickets - The attacker waits for the
endTimeto be reached - The attacker calls
L2NaffleBaseInternal._drawWinner()to request a random number that theVRFManagerneeds to commit to the zkSync Era chain - The attacker front-runs the
VRFManagerby doing:- call
L2NaffleBaseInternal._ownerCancelNaffle() - call
L2NaffleBaseInternal._refundTicketsForNaffle()to get a refund for his paid tickets
- call
VRFManagernow callsL2NaffleBaseInternal._setWinner(), sending the earnings for the paid tickets to the attacker
Now the attacker owns his Naffle token and all his funds for the paid tickets times two.
Also note that it doesn't matter whether the cancel or setWinner message is consumed on L1 first since in either case the attacker ends up receiving the Naffle token.
Impact:
A malicious Naffle creater can drain the funds from the L2 Naffle contract, causing a loss to the protocol and other users.
Recommendation:
This issue is fixed as part of the fix for issue #2.
An additional layer of security is to add a check in the L2NaffleBaseInternal._setWinner() function that the Naffle status is ACTIVE or POSTPONED.
diff --git a/contracts/naffle/zksync/L2NaffleBaseInternal.sol b/contracts/naffle/zksync/L2NaffleBaseInternal.sol
index 803497ab..8bbb27f8 100644
--- a/contracts/naffle/zksync/L2NaffleBaseInternal.sol
+++ b/contracts/naffle/zksync/L2NaffleBaseInternal.sol
@@ -356,6 +356,10 @@ abstract contract L2NaffleBaseInternal is IL2NaffleBaseInternal, AccessControlIn
L2NaffleBaseStorage.Layout storage layout = L2NaffleBaseStorage.layout();
NaffleTypes.L2Naffle storage naffle = layout.naffles[_naffleId];
+ if (naffle.status != NaffleTypes.NaffleStatus.ACTIVE && naffle.status != NaffleTypes.NaffleStatus.POSTPONED) {
+ revert InvalidNaffleStatus(naffle.status);
+ }
+
uint256 winningTicketId = _randomNumber % (naffle.numberOfPaidTickets + naffle.numberOfOpenEntries) + 1;
naffle.winningTicketId = winningTicketId;This ensures that a refund has not already been made.
Mitigation Review:
The L2NaffleBaseInternal._setWinner() function can only be called in the SELECTING_WINNER state which in turn ensures that the Naffle has not been cancelled.
Description:
There exist two types of Naffles. STANDARD and UNLIMITED Naffles.
For UNLIMITED Naffles, a winner should only be drawn (i.e., a random number requested) when the endTime is reached.
By calling the L2NaffleBaseInternal._buyTickets() function with amount=0 parameter for an UNLIMITED Naffle that doesn't have any paid tickets yet, the random number can be requested immediately, i.e., the Naffle is effectively ended.
The random number is requested since L2NaffleBaseInternal._buyTickets() doesn't not revert when a zero amount is provided and the downstream check in L2NaffleBaseInternal._checkIfNaffleIsFinished() passes:
function _checkIfNaffleIsFinished(
NaffleTypes.L2Naffle storage _naffle
) internal {
if (_naffle.numberOfOpenEntries == _naffle.openEntryTicketSpots && _naffle.numberOfPaidTickets == _naffle.paidTicketSpots) {
_requestRandomNumber(_naffle.naffleId);
}
}In the outlined scenario, all numbers in the above check are equal to zero and so the check passes and the random number is requested.
Note that the same issue exists in the L2NaffleBaseInternal._useOpenEntryTickets() function when an empty _ticketIds array is provided.
Impact:
The Impact of this finding alone is limited to a griefing attack (making new UNLIMITED Naffles unusable).
This leaves funds unaffected as the admin of the protocol can just cancel the Naffle (as long as the VRFManager does not mistakenly set a winner), sending the token(s) back to the Naffle owner.
Recommendation:
Add a check to the affected L2NaffleBaseInternal._buyTickets() and L2NaffleBaseInternal._useOpenEntryTickets() functions that amount / _ticketIds.length must be greater zero.
diff --git a/contracts/naffle/zksync/L2NaffleBaseInternal.sol b/contracts/naffle/zksync/L2NaffleBaseInternal.sol
index 803497ab..f32bccae 100644
--- a/contracts/naffle/zksync/L2NaffleBaseInternal.sol
+++ b/contracts/naffle/zksync/L2NaffleBaseInternal.sol
@@ -78,6 +78,9 @@ abstract contract L2NaffleBaseInternal is IL2NaffleBaseInternal, AccessControlIn
if (naffle.naffleTokenInformation.tokenAddress == address(0)) {
revert InvalidNaffleId(_naffleId);
}
+ if (_amount == 0) {
+ revert InvalidAmount();
+ }
if (naffle.status != NaffleTypes.NaffleStatus.ACTIVE && naffle.status != NaffleTypes.NaffleStatus.POSTPONED) {
revert InvalidNaffleStatus(naffle.status);
}
@@ -171,6 +174,9 @@ abstract contract L2NaffleBaseInternal is IL2NaffleBaseInternal, AccessControlIn
if (naffle.naffleTokenInformation.tokenAddress == address(0)) {
revert InvalidNaffleId(_naffleId);
}
+ if (_ticketIds.length == 0) {
+ revert InvalidAmount();
+ }
if (naffle.status != NaffleTypes.NaffleStatus.ACTIVE && naffle.status != NaffleTypes.NaffleStatus.POSTPONED) {
revert InvalidNaffleStatus(naffle.status);
}Mitigation Review:
The recommendation has been implemented.
Description:
The L2NaffleBaseInternal._postponeNaffle() and L2NaffleBaseInternal._ownerCancelNaffle() functions can only be called by the owner of a Naffle.
In both cases it is required that naffle.endTime <= block.timestamp, i.e., the endTime is reached.
However, when the endTime is reached users can also call L2NaffleBaseInternal._drawWinner().
So ther exists a race condition. owners want to postpone the Naffle or cancel the Naffle due to insufficient participation and participants want to draw a winner since they have a high chance of winning the Naffle.
The owner then won't be able to cancel the Naffle or postpone it even though it is a feature of the protocol that he should be able to do it.
Impact:
Naffles are completed even when the owner may want to cancel it or postpone it due to insufficient participation.
Recommendation:
Remove the endTime check from L2NaffleBaseInternal._postponeNaffle() and L2NaffleBaseInternal._ownerCancelNaffle().
Naffle owners should have the option to call both functions before a winner can be drawn.
diff --git a/contracts/naffle/zksync/L2NaffleBaseInternal.sol b/contracts/naffle/zksync/L2NaffleBaseInternal.sol
index 803497ab..39a93c6e 100644
--- a/contracts/naffle/zksync/L2NaffleBaseInternal.sol
+++ b/contracts/naffle/zksync/L2NaffleBaseInternal.sol
@@ -240,10 +240,6 @@ abstract contract L2NaffleBaseInternal is IL2NaffleBaseInternal, AccessControlIn
revert NotAllowed();
}
- if (naffle.endTime > block.timestamp) {
- revert NaffleNotEndedYet(naffle.endTime);
- }
-
if (naffle.naffleType == NaffleTypes.NaffleType.UNLIMITED) {
revert InvalidNaffleType(naffle.naffleType);
}
@@ -513,9 +509,6 @@ abstract contract L2NaffleBaseInternal is IL2NaffleBaseInternal, AccessControlIn
if (naffle.status != NaffleTypes.NaffleStatus.ACTIVE) {
revert InvalidNaffleStatus(naffle.status);
}
- if (naffle.endTime > block.timestamp) {
- revert NaffleNotFinished(naffle.endTime);
- }
if (naffle.owner != msg.sender) {
revert NotNaffleOwner(naffle.owner);
}Mitigation Review:
The recommendation has been implemented.
Testing of the codebase is performed with the Brownie framework while deployment is performed with Hardhat.
The dependencies for Brownie are specified as follows:
dependencies:
- chiru-labs/ERC721A@4.2.3
- OpenZeppelin/openzeppelin-contracts@4.8.0
- OpenZeppelin/openzeppelin-contracts-upgradeable@4.8.1
- solidstate-network/solidstate-solidity@0.0.51
- c0dejax/v2-testnet-contracts@.0.2.0
- smartcontractkit/chainlink@2.3.0And dependencies for Hardhat are specified here:
"devDependencies": {
"@ethersproject/hash": "^5.7.0",
"@ethersproject/web": "^5.7.1",
"@matterlabs/hardhat-zksync-deploy": "^0.6.3",
"@matterlabs/hardhat-zksync-solc": "^0.3.16",
"@matterlabs/hardhat-zksync-verify": "^0.2.0",
"@matterlabs/zksync-contracts": "^0.6.1",
"@nomicfoundation/hardhat-verify": "^1.1.0",
"@nomiclabs/hardhat-ethers": "^2.2.1",
"@openzeppelin/contracts": "^4.8.2",
"@openzeppelin/contracts-upgradeable": "^4.8.2",
"@solidstate/contracts": "^0.0.54",
"@typechain/hardhat": "^6.1.5",
"@typechain/truffle-v5": "^8.0.2",
"@types/chai": "^4.3.4",
"@types/mocha": "^10.0.1",
"@types/node": "^18.15.11",
"chai": "^4.3.7",
"dotenv": "^16.0.3",
"ethers": "^5.7.2",
"hardhat": "^2.13.0",
"hardhat-dependency-compiler": "^1.1.3",
"mocha": "^10.2.0",
"ts-node": "^10.9.1",
"typechain": "^8.1.1",
"typescript": "^5.0.3",
"zksync-web3": "^0.14.3"
},The dependency versions do not match.
E.g. tests use Solidstate contracts version 0.0.51 while deployment uses 0.0.54.
Based on the solidstate commit history there do not seems to be any breaking changes in between 0.0.51 and 0.0.54. Also, tests are supposed to be migrated to Hardhat anyways. But still, it's dangerous to use different versions for testing and deployment and the short term recommendation is to use the same version in both the Brownie and the Hardhat project, while the long-term recommendation is to migrate tests to Hardhat.
Mitigation Review:
The versions for solidstate are now identical. Still, the versions for openzeppelin are different. The only Openzeppelin import, the ECDSA library, is identical in both commits.
The long-term recommendation remains to move tests to hardhat.
The L1NaffleBaseInternal and L2NaffleBaseInternal contracts are able to consume signatures that are signed by the signatureSigner address.
In both cases the domainSeparator is calculated like so:
bytes32 domainSeparator = keccak256(
abi.encode(
_domainSignature,
keccak256(abi.encodePacked(_domainName))
)
);The domainSeparator therefore only consists of the _domainName which, according to the client, is set to Naffles in production, Naffles development in development and Naffles staging in staging.
The problem is that in case there are multiple production deployments, on the same chain or on different chains, signatures can be used across deployments.
It is therefore recommended to expand the domainSeparator by a chainId and verifyingContract field to make each deployment unique.
The domainSignature then needs to be set like this:
_setDomainSignature(keccak256(abi.encodePacked("EIP712Domain(string name,uint256 chainId,address verifyingContract)")));And the domainSeparator is then computed like this:
bytes32 domainSeparator = keccak256(
abi.encode(
_domainSignature,
keccak256(abi.encodePacked(_domainName)),
block.chainid,
address(this)
)
);Mitigation Review:
The client did not implement any changes.
There won't be a problem as long as domainName is unique for each live deployment.
Description:
The VRFManager needs to provide the winner address as an argument to the L2NaffleBase.setWinner() function.
The winning ticket is calculated based on the random number and the winner is the address that owns the ticket at that time.
Impact:
A User A that owns a ticket and wants to transfer it to User B at a time when the winner is currently being selected cannot be sure which owner is used to determine the winner.
This can lead to unintended outcomes. User A does not know whether he will be considered as the ticket owner or User B will.
Recommendation:
The VRFManager should adopt a safe policy for when to checkpoint the ticket owners. A natural checkpoint is say 10 minutes after the time at which the random number is being requested.
It is important to have a 5-10 minute buffer such that ticket transfers cannot be front-run by drawing a winner (and thereby requesting the random number).
Instead, when the random number has been requested, users can abstain from transfers to avoid any race conditions.
Mitigation Review:
This finding does not require any changes in the smart contracts.
Also, there does not exist any code in the repository that implements the VRFManager.
The finding should be considered for the future implementation of the VRFManager.
The four admin facet contracts have a setAdmin() function that is callable by the owner.
However there is no removeAdmin() function. The owner could still give himself the admin role and then use AccessControl.revokeRole() to remove existing admins.
But having a dedicated removeAdmin() function would be more convenient:
diff --git a/contracts/naffle/ethereum/L1NaffleAdmin.sol b/contracts/naffle/ethereum/L1NaffleAdmin.sol
index e69de0b6..645a30e7 100644
--- a/contracts/naffle/ethereum/L1NaffleAdmin.sol
+++ b/contracts/naffle/ethereum/L1NaffleAdmin.sol
@@ -63,6 +63,10 @@ contract L1NaffleAdmin is IL1NaffleAdmin, L1NaffleBaseInternal, AccessControl, S
_grantRole(_getAdminRole(), _admin);
}
+ function removeAdmin(address _admin) external onlyOwner {
+ _revokeRole(_getAdminRole(), _admin);
+ }
+
/**
* @inheritdoc IL1NaffleAdmin
*/
diff --git a/contracts/naffle/zksync/L2NaffleAdmin.sol b/contracts/naffle/zksync/L2NaffleAdmin.sol
index 83f05164..346679ca 100644
--- a/contracts/naffle/zksync/L2NaffleAdmin.sol
+++ b/contracts/naffle/zksync/L2NaffleAdmin.sol
@@ -15,6 +15,10 @@ contract L2NaffleAdmin is IL2NaffleAdmin, L2NaffleBaseInternal, AccessControl, S
_grantRole(_getAdminRole(), _admin);
}
+ function removeAdmin(address _admin) external onlyOwner {
+ _revokeRole(_getAdminRole(), _admin);
+ }
+
/**
* @inheritdoc IL2NaffleAdmin
*/
diff --git a/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketAdmin.sol b/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketAdmin.sol
index 80407866..fd16bf31 100644
--- a/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketAdmin.sol
+++ b/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketAdmin.sol
@@ -14,6 +14,10 @@ contract L2OpenEntryTicketAdmin is IL2OpenEntryTicketAdmin, L2OpenEntryTicketBas
_grantRole(_getAdminRole(), _admin);
}
+ function removeAdmin(address _admin) external onlyOwner {
+ _revokeRole(_getAdminRole(), _admin);
+ }
+
/**
* @inheritdoc IL2OpenEntryTicketAdmin
*/
diff --git a/contracts/tokens/zksync/ticket/paid/L2PaidTicketAdmin.sol b/contracts/tokens/zksync/ticket/paid/L2PaidTicketAdmin.sol
index 65a61969..227567e4 100644
--- a/contracts/tokens/zksync/ticket/paid/L2PaidTicketAdmin.sol
+++ b/contracts/tokens/zksync/ticket/paid/L2PaidTicketAdmin.sol
@@ -14,7 +14,11 @@ contract L2PaidTicketAdmin is IL2PaidTicketAdmin, L2PaidTicketBaseInternal, Acce
function setAdmin(address _admin) external onlyOwner {
_grantRole(_getAdminRole(), _admin);
}
+ function removeAdmin(address _admin) external onlyOwner {
+ _revokeRole(_getAdminRole(), _admin);
+ }
+
/**
* @inheritdoc IL2PaidTicketAdmin
*/Mitigation Review:
The recommendation has been implemented.
Some storage variables do not have view function associated with them which means their values cannot be queried via another smart contract nor can they be easily checked in a block explorer.
Check all storage variables and see whether they need a view function.
In particular it is recommended to have a view function for the platformFeesAccumulated variable in L2NaffleBaseStorage to be able to query the current amount of accumulated fees since this is required as an input to L2NaffleAdmin.withdrawPlatformFees().
Mitigation Review:
There now exists a view function for the platformFeesAccumulated variable.
Some storage variables still don't have a view function associated with them, but it's at the discretion of the client whether they will be needed.
Also, facets can be upgraded and so adding view functions later on is possible.
Signatures are checked via the Signature.getSigner() library function.
The ecrecover() function accepts signatures that are malleable (i.e. can be changed and remain valid). There's currently no notion of signatures being supposed to be used only once so it's not an issue.
However it is something to be aware of and it would be best to use OZ's ECDSA library (specifically the recover() function) instead which reverts if a malleable signature is passed.
Check here for a background on signature malleability.
Mitigation Review:
The recommendation has been implemented and the ECDSA library is now used.
The Storage contracts declare some variables that are never used.
Remove them to make the code cleaner and more understandable.
Note: Also remove the setter function for L1_MESSENGER_ADDRESS.
https://github.com/Naffles/naffles-smart-contracts/blob/96961659e9d160a93712028297a044f5486c3df8/contracts/naffle/ethereum/L1NaffleBaseStorage.sol#L15
Note: Also remove getter and setter functions for minimumPaidTicketPriceInWei.
https://github.com/Naffles/naffles-smart-contracts/blob/96961659e9d160a93712028297a044f5486c3df8/contracts/naffle/ethereum/L1NaffleBaseStorage.sol#L28
Note: Also remove the setter function for minL2GasLimitForCreateNaffle.
https://github.com/Naffles/naffles-smart-contracts/blob/96961659e9d160a93712028297a044f5486c3df8/contracts/naffle/ethereum/L1NaffleBaseStorage.sol#L30
Mitigation Review:
All recommended changes have been implemented, except for removing minimumTicketPrinceInWei
The recommendation is to remove it.
It has been removed in 5bf887822bac8df771ef8aaf674617f06bea9f0d.
The L2OpenEntryTicketView.getTotalSupply()function is redundant since the ERC721 standard already includes a public totalSupply() function.
The totalSupply() function is part of ERC721Enumerable which L2OpenEntryTicketBase downstream inherits from.
diff --git a/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketBaseInternal.sol b/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketBaseInternal.sol
index 5b6912a0..d8b1d648 100644
--- a/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketBaseInternal.sol
+++ b/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketBaseInternal.sol
@@ -118,14 +118,6 @@ abstract contract L2OpenEntryTicketBaseInternal is IL2OpenEntryTicketBaseInterna
L2OpenEntryTicketStorage.layout().l2NaffleContractAddress = _l2NaffleContractAddress;
}
- /**
- * @notice gets the total supply.
- * @return totalSupply the total supply.
- */
- function _getTotalSupply() internal view returns (uint256 totalSupply) {
- totalSupply = _totalSupply();
- }
-
/**
* @notice sets the base URI
* @param _baseURI the base URI.
diff --git a/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketView.sol b/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketView.sol
index 03b5d2c9..b71bd1b3 100644
--- a/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketView.sol
+++ b/contracts/tokens/zksync/ticket/open_entry/L2OpenEntryTicketView.sol
@@ -20,13 +20,6 @@ contract L2OpenEntryTicketView is IL2OpenEntryTicketView, L2OpenEntryTicketBaseI
return _getAdminRole();
}
- /**
- * @inheritdoc IL2OpenEntryTicketView
- */
- function getTotalSupply() external view returns (uint256) {
- return _getTotalSupply();
- }
-Mitigation Review:
The recommendation has been implemented.
The minL2ForwardedGasForCreateNaffle variable is set once upon the construction of the L1NaffleDiamond and is used to check that msg.value is sufficient to successfully execute the L1->L2 transaction.
Based on the zksync Era source code, the L2 gas price can vary which means there is no good way to set a minimum for msg.value.
Since the caller of createNaffle already needs to provide _l2MessageParams which contains l2GasLimit and l2GasPerPubdataByteLimit we can assume that he can also calculate the current L2 gas price (in reality this can be handled by the application frontend).
The recommendation is to not rely on the L1 contracts to enforce gas requirements for L2, but instead to rely on the caller to make such calculations.
diff --git a/contracts/naffle/ethereum/L1NaffleBaseInternal.sol b/contracts/naffle/ethereum/L1NaffleBaseInternal.sol
index 1cde54b0..0fe1c494 100644
--- a/contracts/naffle/ethereum/L1NaffleBaseInternal.sol
+++ b/contracts/naffle/ethereum/L1NaffleBaseInternal.sol
@@ -118,10 +118,6 @@ abstract contract L1NaffleBaseInternal is IL1NaffleBaseInternal {
})
);
- if(msg.value < layout.minL2ForwardedGasForCreateNaffle) {
- revert InsufficientL2GasForwardedForCreateNaffle();
- }
-
txHash = zksync.requestL2Transaction{value: msg.value}(
layout.zkSyncNaffleContractAddress,
0,
@@ -424,23 +420,6 @@ abstract contract L1NaffleBaseInternal is IL1NaffleBaseInternal {
naffle = L1NaffleBaseStorage.layout().naffles[_naffleId];
}
- /**
- * @notice sets minimum gas to be forwarded for L2 transactions in _createNaffle
- * @param _minL2ForwardedGasForCreateNaffle the minimum gas limit to be forwarded for L2 transactions in _createNaffle
- * @dev set the minimum amount of wei this transaction should foward to L2. Should be much higher than the actual cost, since refunds are given
- */
- function _setMinL2ForwardedGas(uint256 _minL2ForwardedGasForCreateNaffle) internal {
- L1NaffleBaseStorage.layout().minL2ForwardedGasForCreateNaffle = _minL2ForwardedGasForCreateNaffle;
- }
-
- /**
- * @notice sets minimum gas limit foir the L2 transaction in _createNaffle
- * @param _minL2GasLimit the minimum gas limit for the L2 transaction in _createNaffle
- */
- function _setMinL2GasLimit(uint256 _minL2GasLimit) internal {
- L1NaffleBaseStorage.layout().minL2GasLimitForCreateNaffle = _minL2GasLimit;
- }
-
/**
* @notice sets the collection signature
* @param _collectionSignature the new collection signature.
diff --git a/contracts/naffle/ethereum/L1NaffleBaseStorage.sol b/contracts/naffle/ethereum/L1NaffleBaseStorage.sol
index 890c46d0..34f1a17d 100644
--- a/contracts/naffle/ethereum/L1NaffleBaseStorage.sol
+++ b/contracts/naffle/ethereum/L1NaffleBaseStorage.sol
@@ -26,8 +26,6 @@ library L1NaffleBaseStorage {
uint256 minimumNaffleDuration;
uint256 minimumPaidTicketSpots;
uint256 minimumPaidTicketPriceInWei;
- uint256 minL2ForwardedGasForCreateNaffle;
- uint256 minL2GasLimitForCreateNaffle;
mapping(uint256 => NaffleTypes.L1Naffle) naffles;
mapping(uint256 => address) naffleWinner;
mapping(uint256 => mapping(uint256 => bool)) isL2ToL1MessageProcessed;
diff --git a/contracts/naffle/ethereum/L1NaffleDiamond.sol b/contracts/naffle/ethereum/L1NaffleDiamond.sol
index c516dd87..e81ae8b6 100644
--- a/contracts/naffle/ethereum/L1NaffleDiamond.sol
+++ b/contracts/naffle/ethereum/L1NaffleDiamond.sol
@@ -29,10 +29,6 @@ contract L1NaffleDiamond is SolidStateDiamond, AccessControl, L1NaffleBaseIntern
_setFoundersKeyAddress(_foundersKeyContractAddress);
_setFoundersKeyPlaceholderAddress(_foundersKeyPlaceholderAddress);
_setZkSyncAddress(_zksyncContractAddress);
-
- // pre calculated minimum gas required
- _setMinL2ForwardedGas(1163284000000000);
- _setMinL2GasLimit(2326568);
_setSignatureSignerAddress(msg.sender);
_setCollectionWhitelistSignature(keccak256(abi.encodePacked("CollectionWhitelist(address tokenAddress,uint256 expiresAt)")));
_setDomainSignature(keccak256(abi.encodePacked("EIP712Domain(string name)")));Mitigation Review:
The recommendation has been implemented.
Currently, when a L1->L2 transaction to create a Naffle fails, the only way to get the token back is for the admin to call L1NaffleAdmin.adminCancelNaffle()
Instead it's possible to make use of the Mailbox.proveL1ToL2TransactionStatus() function such that the Naffle creator can prove that the transaction has failed.
diff --git a/contracts/libraries/NaffleTypes.sol b/contracts/libraries/NaffleTypes.sol
index 764d997f..743ea811 100644
--- a/contracts/libraries/NaffleTypes.sol
+++ b/contracts/libraries/NaffleTypes.sol
@@ -69,6 +69,7 @@ library NaffleTypes {
address owner;
address winner;
bool cancelled;
+ bytes32 txHash;
}
struct OpenEntryTicket {
@@ -91,5 +92,10 @@ library NaffleTypes {
NaffleStatus status;
NaffleType naffleType;
}
+
+ enum TxStatus {
+ Failure,
+ Success
+ }
}
diff --git a/contracts/naffle/ethereum/L1NaffleBase.sol b/contracts/naffle/ethereum/L1NaffleBase.sol
index 7500964f..8ba40a4a 100644
--- a/contracts/naffle/ethereum/L1NaffleBase.sol
+++ b/contracts/naffle/ethereum/L1NaffleBase.sol
@@ -8,6 +8,8 @@ import "@solidstate/contracts/interfaces/IERC721Receiver.sol";
import "@solidstate/contracts/interfaces/IERC1155Receiver.sol";
import "../../../interfaces/naffle/ethereum/IL1NaffleBase.sol";
+import "../../libraries/NaffleTypes.sol";
+
contract L1NaffleBase is IL1NaffleBase, L1NaffleBaseInternal, AccessControl, IERC721Receiver, IERC1155Receiver {
/**
@@ -33,6 +35,32 @@ contract L1NaffleBase is IL1NaffleBase, L1NaffleBaseInternal, AccessControl, IER
);
}
+ function cancelFailedNaffle(
+ uint256 _naffleId,
+ uint256 _l2BlockNumber,
+ uint256 _l2MessageIndex,
+ uint16 _l2TxNumberInBlock,
+ bytes32[] calldata _merkleProof
+ ) external {
+ L1NaffleBaseStorage.Layout storage layout = L1NaffleBaseStorage.layout();
+ NaffleTypes.L1Naffle storage naffle = layout.naffles[_naffleId];
+
+ require(msg.sender == naffle.owner);
+
+ bool success = zksync.proveL1ToL2TransactionStatus(
+ naffle.txHash,
+ _l2BlockNumber,
+ _l2MessageIndex,
+ _l2TxNumberInBlock,
+ _merkleProof,
+ TxStatus.Failure
+ );
+
+ require(success);
+
+ _cancelNaffle(_naffleId);
+ }
+
/**
* @inheritdoc IL1NaffleBase
*/
diff --git a/contracts/naffle/ethereum/L1NaffleBaseInternal.sol b/contracts/naffle/ethereum/L1NaffleBaseInternal.sol
index 1cde54b0..3cc4ed71 100644
--- a/contracts/naffle/ethereum/L1NaffleBaseInternal.sol
+++ b/contracts/naffle/ethereum/L1NaffleBaseInternal.sol
@@ -96,14 +96,6 @@ abstract contract L1NaffleBaseInternal is IL1NaffleBaseInternal {
IERC20(_naffleTokenInformation.tokenAddress).safeTransferFrom(msg.sender, address(this), _naffleTokenInformation.amount);
}
- layout.naffles[naffleId] = NaffleTypes.L1Naffle({
- naffleTokenInformation: _naffleTokenInformation,
- naffleId: naffleId,
- owner: msg.sender,
- winner: address(0),
- cancelled: false
- });
-
IZkSync zksync = IZkSync(layout.zkSyncAddress);
bytes memory data = abi.encodeWithSignature(
"createNaffle(((address,uint256,uint256,uint8),address,uint256,uint256,uint256,uint256,uint8))",
@@ -132,6 +124,15 @@ abstract contract L1NaffleBaseInternal is IL1NaffleBaseInternal {
msg.sender
);
+ layout.naffles[naffleId] = NaffleTypes.L1Naffle({
+ naffleTokenInformation: _naffleTokenInformation,
+ naffleId: naffleId,
+ owner: msg.sender,
+ winner: address(0),
+ cancelled: false,
+ txHash: txHash
+ });
+
emit L1NaffleCreated(_naffleTokenInformation, naffleId, msg.sender, _paidTicketSpots, _ticketPriceInWei, _endTime, _naffleType);
}
@@ -264,6 +265,7 @@ abstract contract L1NaffleBaseInternal is IL1NaffleBaseInternal {
L1NaffleBaseStorage.Layout storage layout = L1NaffleBaseStorage.layout();
NaffleTypes.L1Naffle storage naffle = layout.naffles[_naffleId];
+ require(naffle.cancelled == false);
naffle.cancelled = true;
NaffleTypes.NaffleTokenInformation memory tokenInfo = naffle.naffleTokenInformation;Note that this function requires further testing and just serves as a suggestion for how to implement the functionality.
Also, from a security perspective it doesn't make a difference whether the L1NaffleAdmin.adminCancelNaffle() function is removed or not since the admin is fully trusted anyways.
Mitigation Review:
The recommendation has been implemented.