New user cannot stake a transferred fighter which had been staked & fully unstaked in the current round by the sender

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L248

Vulnerability details

According to docs: "Players are only able to earn $NRN in our game by staking their tokens and winning."

Users can stake their token as long as they haven't unstaked even once in a round. If a user unstakes, then he can no longer stake in that round. Users can unstake their full staked amount.

However if a user has fully unstaked their tokens they cannot stake again. But if he now transfers it to another player, the new player also cannot stake to earn rewards.

Proof of Concept

Whenever a user unstakes hasUnstaked is set to true.

File: RankedBattle.sol

     function unstakeNRN(uint256 amount, uint256 tokenId) external {
     ...
        hasUnstaked[tokenId][roundId] = true;
     ...

In stakeNRN() the last require checks if the tokenId has not been unstaked in the current round.

File: RankedBattle.sol

      function stakeNRN(uint256 amount, uint256 tokenId) external {
        require(amount > 0, "Amount cannot be 0");
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
        require(_neuronInstance.balanceOf(msg.sender) >= amount, "Stake amount exceeds balance");
        require(hasUnstaked[tokenId][roundId] == false, "Cannot add stake after unstaking this round");
        ...

If the user fully unstakes all the stake, the fighter can be transferred to another user which can be seen in the below function in FighterFarm contract.

File: FighterFarm.sol

       function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
        return (
          _isApprovedOrOwner(msg.sender, tokenId) &&
          balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
          !fighterStaked[tokenId]
        );
    }

Scenario

• Bob has staked his fighter in the current round. (Now Bob cannot transfer the fighter)
• After sometime, Bob fully unstakes the fighter.
• hasUnstaked is set to true & the fighter can now be transferred because updateFighterStaking sets fighterStaked to false.

File: RankedBattle.sol

    hasUnstaked[tokenId][roundId] = true;
        bool success = _neuronInstance.transfer(msg.sender, amount);                    
        if (success) {
            if (amountStaked[tokenId] == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, false);      
            }

• Bob now transfers the fighter to Alice while the current round is active.
• Alice tries to stake the fighter to earn tokens by calling stakeNRN() for the current round. The function reverts because of the require statement which checks if the tokenId hasn't already been unstaked in the current round.

Impact

The recipient of the fighter won't be able to stake in order to earn $NRN tokens & has to wait till a new round is set to be able to stake again. This vulnerability deprives the user from being able to earn tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Allow new owners of the fighter to be able to stake for the current round if the previous owner has fully unstaked their stake.

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #1641

[raymondfam]

Will have to wait till the next round.

[c4-judge]

HickupHH3 changed the severity to 3 (High Risk)

[c4-judge]

HickupHH3 changed the severity to 2 (Med Risk)

[c4-judge]

HickupHH3 marked the issue as not a duplicate

[c4-judge]

HickupHH3 marked the issue as duplicate of #1289

[c4-judge]

HickupHH3 marked the issue as partial-75

[HickupHH3]

has some inkling of DoS, but phrased as an unintended side effect.

actually intended behaviour.

[c4-judge]

HickupHH3 marked the issue as partial-25

[c4-judge]

HickupHH3 changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by HickupHH3

[c4-judge]

HickupHH3 marked the issue as not a duplicate

[c4-judge]

HickupHH3 marked the issue as duplicate of #1289

[c4-judge]

HickupHH3 marked the issue as partial-25

[c4-judge]

HickupHH3 marked the issue as not a duplicate

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Invalid

[Shubh0412]

Hello @HickupHH3 The impact described here & in #1289 is the same.

Whats the point for an attacker if he unstakes with 0 or any amount for the matter of fact? Ultimately the point is that the new user is being barred from staking in the current round which lasts for approx 2 weeks. Its not like the attacker carries out the attack, prevents the new owner from staking & then after the current round is over, gets the fighter back. The ownership is actually transferred to the new user.

The new owner would want the fighter based on its attributes & not how much has been currently staked on that fighter because only the new owner can unstake, not the previous owner.

Plus this does not come under "User's mistake" because its not like the user enters wrong value or this scenario happens because of lack of knowledge on the user's part.

I cannot see the point that it heavily benefits the attacker or anything because the bug impacts the new owner of the fighter which has been shown above.

Again I would like to say that it does not matter if the unstake amount is 0 or full, the new owner will not be able to stake in the current round in both these scenarios & will be able to stake again after the current round has ended in both of these scenarios.

DoS happens because of the vulnerability present in the code & not because of an attacker's mindset or a user's mistake.

Both the root cause & the maximum impact has been described in the report above which satisfies for a Medium severity vulnerability.

[HickupHH3]

Good points raised. As long as the fighter has been unstaked, whether 0 or non-zero amount, the new owner should be unable to stake it in the current round.

Even if unstake() were to require some minimum amount of NRN to be unstaked, the core issue is being able to unstake in the current round it's staked, then not being able to re-stake in the current round, so the attacker can:

• stake the minimum amount
• unstake the minimum amount
• sell the fighter on 3rd party exchanges
• new owner unable to stake in the current round

It seems like it's intended behaviour, so I'd like to @SonnyCastro @brandinho to confirm before invalidating #1289 and its group.

[Shubh0412]

@HickupHH3 Thank you for taking my opinions into consideration.

Regarding the first two points you mentioned above

• stake the minimum amount
• unstake the minimum amount

The attacker has no power in this scenario. After transferring the fighter, the ownership belongs to the new user. So the previous owner (attacker) cannot stake/unstake because of this check in both functions.

 function stakeNRN(uint256 amount, uint256 tokenId) external {
        require(amount > 0, "Amount cannot be 0");
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
 ......................................................................................................

function unstakeNRN(uint256 amount, uint256 tokenId) external {
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");

The only scenario being the user selling or transferring it to someone else as mentioned by you in the third point which impacts the new user as mentioned by you in the fourth point.

If possible, I'd still like to discuss that this cannot be an intended behaviour. Had it been, it would be mentioned in the docs or would have come up in the discussions during the contest.

From the point of view of the new owner of the fighter, it wouldn't make sense to him. Why would he have something which he cannot use? Seeing he cannot use it himself, he might then sells to another who suffers from the same problem.

Overall, the issue does come under Medium severity because of partial DoS to the new owner & the fact that the intended timeline is not a couple of hours or a single day, but 15 days (2 weeks) before the problem is solved.

[HickupHH3]

The attacker has no power in this scenario. After transferring the fighter, the ownership belongs to the new user. So the previous owner (attacker) cannot stake/unstake because of this check in both functions.

This is before the attacker (current owner) lists his fighter for sale, so that the unstaked mapping is set to true.

If possible, I'd still like to discuss that this cannot be an intended behaviour. Had it been, it would be mentioned in the docs or would have come up in the discussions during the contest

We can argue about this all day, but it's up to the sponsors @SonnyCastro @brandinho to say what is the expected behaviour.

From the point of view of the new owner of the fighter, it wouldn't make sense to him. Why would he have something which he cannot use? Seeing he cannot use it himself, he might then sells to another who suffers from the same problem.

Various reasons (speculative, FOMO, etc.). Also, 15 days is the worst case scenario, when the action sequence is done at the beginning of the round.

[Shubh0412]

My apologies. Didn't mean to turn this discussion into an argument.

Will wait for the sponsor's opinions on this issue.

[mcgrathcoutinho]

Hi @HickupHH3, the core issue is not the same.

As long as the fighter has been unstaked, whether 0 or non-zero amount, the new owner should be unable to stake it in the current round. - As I mentioned on the primary issue, a user staking/unstaking with a non-zero amount is intended behaviour. This is because the system tries to avoid the user from continuously staking and unstaking. In this case, the staking and unstaking could also occur from multiple accounts by the same user, which is why they have restricted the fighter NFT to be staked only once per round.

Regarding the primary, it does not state the same issue for two reasons:

1. A fighter NFT that was never staked for the current round is bound from staking.
2. The affected user is the buyer who should validly be able to stake the NFT if it was not staked for the current round. We can also see that the stakeNRN() function disallows anyone from supplying 0 as a parameter. I believe the same should apply to unstakeNRN().

This report here is just a report of how the system works. The primary uses a valid attack path that a malicious user can use to cause harm to other users.

[HickupHH3]

https://github.com/code-423n4/2024-02-ai-arena-findings/issues/1453#issuecomment-2011158581

To re-iterate, even with a non-zero amount blocker, the same harm can be achieved by a malicious user with the following steps listed above, although able to do it with 0 amount lowers the cost by 1 tx since it skips the staking step.

[mcgrathcoutinho]

Hi @HickupHH3

If a fighter NFT is staked using stakeNRN(), you can only unstake it once fully using unstakeNRN(), following which the fighter NFT cannot be staked for again using either the same account or another account since the protocol cannot differentiate between whether both addresses are by the same user or different users. It is true that in case the users are different, the other user would be affected but once again, the protocol cannot differentiate it. If the protocol tries to solve this, it would break their design of preventing multiple staking/unstaking in the same round since users could just use different accounts to stake and unstake more than once.

In the primary, the attack path is different because you unstake directly (i.e. the NFT was not validly staked for the current round through stakeNRN()). This can be leveraged by a malicious user to DOS the buyer from staking the NFT though it was never staked using stakeNRN(). In this case the notion of the same user is not considered since that would be considered self-rugging but when it comes to different users, the malicious user directly unstakes (which is not the intended path, which makes it an attack path) to intentionally cause harm to buyers. The mitigation of the primary issue is different and does not break the intended design compared to how solving the other issue would.

The entry points are what distinguishes the primary from the issue here. As long as there is a non-zero stake occurring, it would fall into intended behaviour by the team as I mentioned above.

[Shubh0412]

Hi @mcgrathcoutinho

Would probably like to clear some misconceptions here. Please do correct me if I am wrong.

According to docs on the official website - Link

"Also, a player may stake as many NRNs on their Challenger NFT at the beginning of a Round. They can also increase their stake during a Round. However, if they choose to reduce their stake during a Round they are no longer able to increase stake until the following Round. This is to prevent people from gaming the staking system by selectively reducing stake before ranked matches."

The users are able to unstake as many times in a round till they have reached the original staked amount. But if they have unstaked even once, they cannot stake again in the current round.

A user might stake in round 1, do nothing in round 2 & 3, and then unstake in round 4. Then sell to someone else. The new owner would not be able to stake for the current round. Plus there seems to no point if the previous owner just leaves its staked token with the fighter because only the new owner can unstake.

Although the attack path in both our issues is different, ultimately it does have the same impact.

In yours, fighter is not staked -> unstaked is called with 0 -> sold to new user -> user unable to stake -> DoS

In mine, fighter already staked -> unstaked fully -> sold to new user -> user unable to stake -> DoS

[mcgrathcoutinho]

Hi @Shubh0412,

There are no misconceptions. I was talking about staking X amount and unstaking X amount fully that would set hasUnstaked to true.

Regarding the remaining information, please completely read this comment of mine. I did not disagree about the impact.

[brandinho]

This is the intended behavior. As mentioned, there is no way for us to know whether the new wallet is the same user or not. This is ultimately to prevent smurfing (i.e. unstaking, then intentionally losing and transfer to a new wallet to stake again). We have been testing this for about 6 months now and I can assure you that many people have tried this strategy. They realize after they transfer their fighter that they cannot do it.

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Invalid