Skip to main content
Dedaub

Obelisk Protocol

Smart Contract Security Assessment

Mar 05, 2024

LzAsia

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:

src
  • 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:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • 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.
LOW
Examples:
  • 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

H1HIGH

Obelisk contract’s _onReceiveRNG function reuses random seed which can lead to biased selection of winners
resolved

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

M1

DoS due to reentrancy in Obelisk::joinDraw function

M1MEDIUM

DoS due to reentrancy in Obelisk::joinDraw function
resolved

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

M2MEDIUM

Accounting error arising from joining a draw with multiple NFTs results in incorrect redemptions when exiting a draw
acknowledged

Acknowledged

The team has indicated that they will not be fixing this issue because the impact is not a significant one on the protocol.

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.

M3MEDIUM

Participants can join the wrong draw if an enterDraw or enterDrawBatch transaction gets delayed.
resolved

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

L1LOW

Losers of a round can gain a greater reward than winners depending on the number of participants
acknowledged

Acknowledged

The team has indicated that they will not be fixing this issue because it can only happen with very low probability.

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.)

N1

RNG can be controlled by the owner by modifying randomizer

N1CENTRALIZATION

RNG can be controlled by the owner by modifying randomizer
resolved

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.

N2

RNG controlled by the owner via cross-chain access control

N2CENTRALIZATION

RNG controlled by the owner via cross-chain access control
acknowledged

Acknowledged

The team indicated that they would be mitigating this issue through the use of a multisig for the owner account.

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.

N3

Funds can be blocked if the owner stops responding

N3CENTRALIZATION

Funds can be blocked if the owner stops responding
acknowledged

Acknowledged

The team indicated that they would be mitigating this issue through the use of a multisig for the owner account.

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.

A1

RNGSender::_onReceiveRNG function suffers from modulo bias when choosing winner

A1ADVISORY

RNGSender::_onReceiveRNG function suffers from modulo bias when choosing winner
acknowledged

Acknowledged

The team indicated that since a random uint256 was being requested, the bias would be negligible and that this was a suitable solution for their application.

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.

A2

Inconsistent treatment of ERC1155 and ERC721 tokens

A2ADVISORY

Inconsistent treatment of ERC1155 and ERC721 tokens
dismissed

Dismissed

The team indicated that this was the intended functioning of the protocol.

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

A3ADVISORY

Obelisk::supportsInterface function should support token receiver interfaces for better integration with these tokens
resolved

Resolved

The IERC721Receiver interface was not added to the supported interfaces. See resolution for A2.

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.

A4

Additional sanity check in Obelisk::startRound function

A4ADVISORY

Additional sanity check in Obelisk::startRound function
resolved

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.

A5

Inconsistent types for expiry variables in Obelisk contract

A5ADVISORY

Inconsistent types for expiry variables in Obelisk contract
dismissed

Dismissed

The team indicated that this was for storage packing purposes and to save gas.

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.

A6

RNGSender can be initialized multiple times

A6ADVISORY

RNGSender can be initialized multiple times
resolved

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.

A7

Additional sanity check in RNGSender::sendValue function

A7ADVISORY

Additional sanity check in RNGSender::sendValue function
dismissed

Dismissed

The team indicated that this was not necessary since the call will revert if the layerZeroFee is not sufficient.

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.

A8

Use NonblockingLzApp retry mechanism in RNGSender

A8ADVISORY

Use NonblockingLzApp retry mechanism in RNGSender
dismissed

Dismissed

The team indicated that the approach taken by them saves gas when compared to the retry mechanism.

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.

A9

Compiler bugs

A9ADVISORY

Compiler bugs
info

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.

Dedaub