Obelisk Protocol

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.