Obelisk Protocol
Smart Contract Security Assessment
Mar 05, 2024

SUMMARY
ABSTRACT
Dedaub was commissioned to perform a security audit of the Obelisk protocol.
BACKGROUND
The Obelisk protocol is a lottery protocol which allows the owner of the protocol to run a lottery across a number of rounds. During each round the owner chooses an NFT collection.
Participants are then able to join the draw associated with the round. A participant can submit a number of entries, by staking an NFT from the collection for each entry. The participant must also stake at least 1 ETH with the protocol for each entry he is submitting to the draw. These funds are transferred to YieldHub. While a round has not expired, participants can also increase the ETH associated with one of their entries.
Once a certain period of time has passed, the round is finalized, and a random number is generated using the randomizer.ai service. This number is used to choose up to five winners from among the participants.
A reward is calculated based on the yield which was generated from the staked ETH during the round. Once the round has been finalized, participants can exit the round, obtaining their principal back and NFT back. If a participant is a winner, he will also win 10% of the reward, while losers share 45% of the reward between them, based on the proportion of ETH which they staked, as opposed to the total ETH staked during the round by losing participants.
It is also possible for users known as donors to provide additional ETH, which increases the yield being generated and contributes to the rewards which can be disbursed by the protocol. Donors can withdraw their donation after 10 days, getting back the original amount but not the yield.
SETTING & CAVEATS
This audit report mainly covers the contracts of the at the time private Obelisk repository. According to the Obelisk team, the contracts under review originate from commit number 814eca50992057ed58053c7a3086b1c9c5701632 of this repository.
The code was provided to the auditing team as a zip file called obelisk-main.zip with an md5 hash of 8b84e56cbe9dd569e763b647260cac2e.
Two auditors worked on the codebase for 5 days on the following contracts:
- arbitrum
- RNGSender.sol
- Initializing.sol
- interfaces
- IAutoPxEth.sol
- IObelisk.sol
- IPirexEth.sol
- IRandomizerCallback.sol
- IRandomizer.sol
- IYieldHub.sol
- Obelisk.sol
- RNGReceiver.sol
- signals
- SObelisk.sol
- tokens
- Papyrus.sol
- YieldHubPirexEth.sol
Following the audit, fixes were implemented by the team. The auditors verified the fixes as of commit 7c2d4dc7a13fbb1ecfd37fb29e7d0d16e2f135a8.
The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than the regular use of the protocol. Functional correctness (i.e. issues in "regular use") is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e. full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.
VULNERABILITIES & FUNCTIONAL ISSUES
This section details issues affecting the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:
- User or system funds can be lost when third-party systems misbehave.
- DoS, under specific conditions.
- Part of the functionality becomes unusable due to a programming error.
- Breaking important system invariants but without apparent consequences.
- Buggy functionality for trusted users where a workaround exists.
- Security issues which may manifest when the system evolves.
Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.
CRITICAL SEVERITY
[No critical severity issues]
HIGH SEVERITY
H1
Obelisk contract’s _onReceiveRNG function reuses random seed which can lead to biased selection of winners
The _onReceiveRNG
function of the Obelisk
contract reuses the random number obtained from the randomizer (called seed
) when computing the maximum of 5 winners for the lottery.
This computation is carried out through a loop, which determines the winner using the formula winner = seed % availableParticipants
and which subtracts the number of availableParticipants
by one during each iteration.
Since the seed stays constant during this process, while the modulo changes sequentially, there could be a possibility where the seed is exactly divisible by more than one modulo, leading to the same slot of the round.participants
array being chosen more than once.
To see an extreme example of this in action, consider the case where the random number chosen is 20, and there are 6 participants in the draw. In this case, the winning slots are the following:
- 20 % 6 = 2
- 20 % 5 = 0
- 20 % 4 = 0
- 20 % 3 = 2
- 20 % 2 = 0
We recommend changing the seed (even pseudo-randomly) during each iteration, in order to avoid such a situation.
MEDIUM SEVERITY
In the _joinDraw
function of the Obelisk
contract, there is a reentrancy vulnerability because of the transfer of control to an arbitrary ERC721 contract during the performERC721Transfer
call. For instance, if a malicious contract renters _joinDraw
, it is possible to add two participants to the round with the same entryId
.
Obelisk::_joinDraw()
function _joinDraw(uint32[] memory _nftIds) private {
uint96 cachedRoundId = currentRoundId;
Round storage round = rounds[cachedRoundId];
uint256 totalNFT = _nftIds.length;
if (msg.value < MIN_ETH_JOIN_DRAW * totalNFT) revert LowerThanMinimum();
if (_isRoundOver(round)) revert RoundIsOver();
papyrus.safeTransferFrom(msg.sender, address(this), PAPYRUS_ID, totalNFT, "");
uint32 nftId;
uint128 depositETH = uint128(msg.value / totalNFT);
address collection = round.collection;
uint32 entryId = uint32(allEntries.length);
for (uint256 i; i < totalNFT; ++i) {
nftId = _nftIds[i];
// Transfer of control
// Reentering function has the same entryId as original call
_performERC721Transfer(collection, msg.sender, address(this), nftId);
round.participants.push(entryId);
// Reentering call gets to add its entry first
// The original call gets to push its entry after
// So the entryId will refer to the reentrant struct
allEntries.push(
DrawEntry({
owner: msg.sender,
roundId: cachedRoundId,
nftId: nftId,
eth: depositETH,
won: false,
withdrawn: false
})
);
// The entryId emitted by both calls is the index of the
// malicious DrawEntry
emit Staked(entryId, msg.sender, nftId, depositETH);
entryId++;
}
_depositETH(round);
}
This entryId
is emitted and read by users, who then use it to interact with their entries (for instance to call increaseEth
). Users trying to do so will find their calls reverting, as the entryId
they are using does not index their entry inside the allEntries
array, but an entry owned by the malicious user. Moreover, they will not be able to index the right entry unless they understand this vulnerability, because the event emitted by the system is giving them the wrong information.
M2
Accounting error arising from joining a draw with multiple NFTs results in incorrect redemptions when exiting a draw
Acknowledged
In the Obelisk
contract’s _joinDraw
function there can be a discrepancy between the amount deposited in yieldHub and the value accounted for in the DrawEntry
struct held inside the allEntries
array.
This is because the value held in the struct is the msg.value
divided by the number of _nftIds
being passed to the function, and this integer division can truncate any remainder of the operation.
Obelisk::_joinDraw()
function _joinDraw(uint32[] memory _nftIds) private {
uint96 cachedRoundId = currentRoundId;
Round storage round = rounds[cachedRoundId];
uint256 totalNFT = _nftIds.length;
if (msg.value < MIN_ETH_JOIN_DRAW * totalNFT) revert LowerThanMinimum();
if (_isRoundOver(round)) revert RoundIsOver();
papyrus.safeTransferFrom(msg.sender, address(this), PAPYRUS_ID, totalNFT, "");
uint32 nftId;
uint128 depositETH = uint128(msg.value / totalNFT);
address collection = round.collection;
uint32 entryId = uint32(allEntries.length);
for (uint256 i; i < totalNFT; ++i) {
nftId = _nftIds[i];
_performERC721Transfer(collection, msg.sender, address(this), nftId);
round.participants.push(entryId);
allEntries.push(
DrawEntry({
owner: msg.sender,
roundId: cachedRoundId,
nftId: nftId,
eth: depositETH,
won: false,
withdrawn: false
})
);
emit Staked(entryId, msg.sender, nftId, depositETH);
entryId++;
}
_depositETH(round);
}
On the other hand, the value deposited in yieldHub and the amount added to the round.stakedEth variable is the actual msg.value
.
Obelisk::_depositETH()
function _depositETH(Round storage _round) private {
if (block.timestamp >= _round.endStakingPeriod) revert StakingPhaseOver();
uint256 shares = yieldHub.deposit{ value: msg.value }();
_round.stakedEth += uint128(msg.value);
_round.claimableShares += uint128(shares);
}
This difference leads to the _onReceiveRNG
function overcounting the value of round.stakedEthLosers
. This occurs because while round.stakedEth
has the correct value, stakedEthWinners
does not - since it is a function of the ETH value held in the drawEntry
struct.
Obelisk::_onReceiveRNG()
function _onReceiveRNG(bytes32 _slug, bytes32 _value) internal override {
//some code omitted
uint256 stakedEthWinners;
//some code omitted
for (uint256 i = 0; i < MAX_WINNERS; ++i) {
if (availableParticipants == 0) {
break;
winner = seed % availableParticipants;
cachedPrevIndex = round.participants[totalParticipants - i];
cachedWinnerIndex = round.participants[winner];
winnerReceipt = allEntries[cachedWinnerIndex];
winnerReceipt.won = true;
stakedEthWinners += winnerReceipt.eth;
round.participants[totalParticipants - i] = cachedWinnerIndex;
round.participants[winner] = cachedPrevIndex;
availableParticipants--;
winnersCount++;
}
(uint128 claimableShares, uint128 principalShares) = (round.claimableShares, round.principalShares);
if (winnersCount < MAX_WINNERS && claimableShares > principalShares) {
// if there are less than 5 winners, to-be-undistributed reward is carried over
uint256 reward = uint256(claimableShares - principalShares);
sharesCarriedOver += reward.mulDiv(WINNER_REWARD, BPS) * (MAX_WINNERS - winnersCount)
+ reward.mulDiv(LOSERS_REWARD_TOTAL, BPS);
}
round.stakedEthLosers = uint128(round.stakedEth - stakedEthWinners);
}
These accounting anomalies are then realized when _exitDraw
is called. For instance, when calculating the unstakingShares
for the first time, the principal shares are multiplied by entryEth/stakedEth
, resulting in a smaller value of unstakingShares
than expected because entryEth
is a function of the eth value in the drawEntry
struct, which has thus been undercounted.
Similarly, when the caller is a loser and additional unstakingShares
are added by multiplying the roundReward by entryEth/refRound.stakedEthLosers
, the calculation is also incorrect because entryEth
is undercounted, while stakedEthLosers
has been overcounted.
Obelisk::_exitDraw()
function _exitDraw(uint32[] memory _ids) private {
DrawEntry storage entry;
uint256 unstakingNFT;
uint256 unstakingShares;
Round storage refRound;
uint128 entryEth;
uint128 stakedEth;
uint128 claimableShares;
uint128 principalShares;
uint256 roundReward;
for (uint256 i = 0; i < _ids.length; ++i) {
entry = allEntries[_ids[i]]; //@audit - not the right index??
refRound = rounds[entry.roundId];
if (entry.withdrawn) revert AlreadyExited()
if (entry.owner != msg.sender) revert NotDrawEntryOwner();
if (refRound.state != RoundState.FINALIZED) revert RoundNotFinalized();
if (!refRound.winnersInitialized) revert RoundWinnersNotInitialized();
_performERC721Transfer(refRound.collection, address(this), msg.sender, uint128(entry.nftId));
entry.withdrawn = true;
(entryEth, stakedEth, claimableShares, principalShares) =
(entry.eth, refRound.stakedEth, refRound.claimableShares, refRound.principalShares);
roundReward = claimableShares > principalShares ? claimableShares - principalShares : 0;
unstakingShares += uint256(principalShares).mulDiv(entryEth, stakedEth);
if (roundReward > 0) {
if (entry.won) {
// 10% for each winner
unstakingShares += roundReward.mulDiv(WINNER_REWARD, BPS);
} else {
// 45% is split to all losers
unstakingShares +=
roundReward.mulDiv(LOSERS_REWARD_TOTAL, BPS).mulDiv(entryEth, refRound.stakedEthLosers);
}
}
unstakingNFT++;
emit Unstaked(msg.sender, entry.nftId, entryEth, msg.sender);
}
papyrus.safeTransferFrom(address(this), msg.sender, PAPYRUS_ID, unstakingNFT, "");
yieldHub.redeem(unstakingShares, msg.sender);
}
The resulting redemption of the unstakingShares
from yieldHub will thus be incorrect as well.
M3
Participants can join the wrong draw if an enterDraw or enterDrawBatch transaction gets delayed.
In the Obelisk
contract, the owner will start a round by calling the startRound
function, and providing the collection of NFTs which the round will be based on. However, when a participant calls the enterDraw
or enterDrawBatch
functions, only the nftIds
are provided.
Thus if the enterDraw
or enterDrawBatch
transaction is delayed (e.g. due to a low gasPrice
), and in the meantime the round ends, the transaction could still be executed against a future round. This could happen either accidentally, or by a malicious user who stores the non-executed transaction, and sends it again to the network at a later time.
This can be a problem if the collection is the same but with a longer duration or staking period, the participant will end up locking their NFTs for longer than desired. Or if the collection has changed and the participant owns NFTs with the same ids in both collections. Indeed the participant could even end up staking the wrong NFTs from the wrong collection.
As a safety feature, the round which the participant wants to participate in should be specified as a parameter, i.e. the user should clearly state under which conditions they want to participate. If this does not agree with the current round the call should revert.
LOW SEVERITY
L1
Losers of a round can gain a greater reward than winners depending on the number of participants
Acknowledged
In the _exitDraw
function of the Obelisk
contract, there is the possibility of a perverse result in which the loser of a round receives a reward which is greater than the reward received by the winners of a round. For example, if there are 6 participants in a draw, and the maximum five of them are winners, the loser will receive a reward of roundReward x 45% x entryEth
/stakedEthLosers
. Since there is only one loser in this case, the last term is effectively 1. Therefore, the loser receives 45% of the roundReward, while each winner receives only 10%. This may not be the intended functioning of the lottery.
CENTRALIZATION ISSUES
It is often desirable for DeFi protocols to assume no trust in a central authority, including the protocol’s owner. Even if the owner is reputable, users are more likely to engage with a protocol that guarantees no catastrophic failure even in the case the owner gets hacked/compromised. We list issues of this kind below. (These issues should be considered in the context of usage/deployment, as they are not uncommon. Several high-profile, high-value protocols have significant centralization threats.)
The RNG is clearly a crucial component in a lottery protocol. Obelisk uses Randomizer, a decentralized RNG protocol that cannot be controlled by a single party. However, the integration of Randomizer still allows the protocol owner to completely control the outcome of the RNG, defying (at least partially) the purpose of using a decentralized RNG.
The first way this can be achieved is via the RNGSender::initialize
function which can be called multiple times by the contract’s owner. Amongst other things, the owner of the contract is able to change the randomizer
variable, which could allow him to call randomizerCallback
himself and arbitrarily select the winner of the lottery.
Note that, although from a security perspective this possibility essentially reduces the RNG to a centralized one, from an accountability perspective the use of the Randomizer is still useful, because such an attempt to bypass it would be observable.
Acknowledged
Even without modifying the randomizer
variable (see N1), the protocol owner can still affect the outcome of the RNG by controlling the cross-chain communication between Obelisk
and RNGSender
. Since the Randomizer protocol runs on Arbitrum, when a round is finalized Obelisk
sends a cross-chain request to RNGSender
to request a random value for that round. RNGSender
makes a corresponding request to Randomizer, and when the random value is received it is forwarded, again cross-chain, back to Obelisk
.
LayerZero is used for cross-chain messaging and both Obelisk
and RNGSender
inherit from LzApp
. As a consequence, both contracts allow their owner to control which sender on other chains can send messages to them by calling setTrustedRemote
, which is an onlyOwner
function. The RNG can be affected by controlling either side of the cross-chain communication.
On the side of Obelisk
, the owner can allow any contract to send messages to RNGReceiver::_nonblockingLzReceive
, which could cause the contract to receive an arbitrarily chosen random value for any round.
Similarly, on the side of RNGSender
, the owner can allow any contract to send messages to RNGSender:: _nonblockingLzReceive
and request for a random value. In this way the owner can request the random value before the round is finalized, so he can predict the RNG outcome and use the prediction to affect the winner.
Acknowledged
Even if a protocol owner is trusted not to act maliciously, there can be scenarios in which he simply stops responding (due to technical issues, loss of keys, etc). In a decentralized protocol, it is desirable for users to be guaranteed to at least be able to unblock their funds, without any action from the part of the owner.
In Obelisk
, when a round is finished, anyone can call finalizeRound
, to proceed with finalizing the round. However, users can exit the round only after the random value arrives and winnersInitialized
becomes true
. Although the cross-chain communication is automated, it might require the intervention of the owner in case of failure. If the owner is not responsive, the random value will never be received which will cause users’ funds to remain locked.
To prevent such scenarios, it might be desirable to add a procedure that unlocks funds after some expiration delay even if winnersInitialized == false.
OTHER / ADVISORY ISSUES
This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.
Acknowledged
The _onReceiveRNG
function of the Obelisk
contract calculates the winner using the formula seed % availableParticipants
, where seed
is the random number obtained remotely. This way of choosing winners suffers from a phenomenon called modulo bias, in that the values between 0
and availableParticipants-1
are not equally likely to be chosen (because they do not form a uniform distribution). This can be mitigated by requesting a large random number from the RNG, but the team should evaluate whether this approximate solution is appropriate for their application.
Dismissed
The Obelisk
contract treats ERC1155
and ERC721
tokens in an inconsistent manner. For instance, while it implements the IERC1155Receiver
interface, it does not implement the IERC721Receiver
interface. In addition transfers of ERC721
tokens pass through TokenTransferrer
, but those of ERC1155
tokens do not. We advise choosing one way of handling these tokens and applying it consistently.
A3
Obelisk::supportsInterface function should support token receiver interfaces for better integration with these tokens
Resolved
The supportsInterface
function of the Obelisk
contract should include support for IERC1155Receiver
and potentially for IERC721Receiver
so that collections interacting with the contract which contain checks for these interfaces can integrate fully with the protocol.
In the startRound
function of the Obelisk
contract, there could be an additional sanity check to the effect that the _durationInSeconds
of a round should be greater than the _stakingPeriodInSeconds
, as one cannot stake if a round has already expired. So it does not make sense to start a round in this case.
Dismissed
In the donate
function of the Obelisk contract
, the expiry
variable is of type uint88
. This contrasts with the types used for other expiry values such as the endTime
in the startRound
function, which is of type uint96
.
The initialize
function of the RNGSender
contract can be called more than once by the owner of the contract. If this is intended, it would be better to separate this into setter functions.
Dismissed
In the sendValue
function of the RNGSender
, the layerZeroFee
can be non-zero but still be too low for the message to be sent. Checking that the fee is at least equal to the value returned by the function estimateLayerZeroFee
would be useful in order to avoid downstream problems.
Dismissed
RNGSender::_nonblockingLzReceive
uses a try
block to handle cases when RNGSender::request
fails. However, NonblockingLzApp
already provides such a mechanism; the advantage of inheriting from NonblockingLzApp
(instead of LzApp)
is that failed messages can be handled by calling retryMessage
. It might be simpler/preferable to use the existing mechanism for handling failures instead of re-implementing it.
The code is compiled with Solidity 0.8.23
. At the time of writing version 0.8.23
does not have any known bugs but the team is encouraged to periodically monitor the list of known bugs since 0.8.23
is a relatively new version of Solidity.
DISCLAIMER
The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.
ABOUT DEDAUB
Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.