Skip to content

Commit

Permalink
test(RewardsStreamerMP): add test to ensure funds are saffe when stac…
Browse files Browse the repository at this point in the history
…k overflow

If there's a malicious upgrade which causes a stack overflow error when
`leave()` is called, the user of the vault should still be able to get
their funds out.

This commit adds a test that proofs this is happening.
  • Loading branch information
0x-r4bbit committed Nov 29, 2024
1 parent ec2dc08 commit dfe8929
Show file tree
Hide file tree
Showing 4 changed files with 146 additions and 26 deletions.
61 changes: 35 additions & 26 deletions .gas-report
Original file line number Diff line number Diff line change
Expand Up @@ -22,26 +22,26 @@
| MIN_LOCKUP_PERIOD | 274 | 274 | 274 | 274 | 11 |
| MP_RATE_PER_YEAR | 230 | 230 | 230 | 230 | 3 |
| SCALE_FACTOR | 294 | 294 | 294 | 294 | 41 |
| STAKING_TOKEN | 2381 | 2381 | 2381 | 2381 | 192 |
| accountedRewards | 373 | 951 | 373 | 2373 | 76 |
| STAKING_TOKEN | 2381 | 2381 | 2381 | 2381 | 196 |
| accountedRewards | 373 | 970 | 373 | 2373 | 77 |
| emergencyModeEnabled | 2398 | 2398 | 2398 | 2398 | 7 |
| enableEmergencyMode | 2460 | 19367 | 24652 | 24652 | 8 |
| getAccount | 1621 | 1621 | 1621 | 1621 | 72 |
| initialize | 137773 | 137773 | 137773 | 137773 | 48 |
| isTrustedCodehash | 563 | 1063 | 563 | 2563 | 192 |
| initialize | 137773 | 137773 | 137773 | 137773 | 49 |
| isTrustedCodehash | 563 | 1063 | 563 | 2563 | 196 |
| leave | 62061 | 62061 | 62061 | 62061 | 1 |
| lock | 9839 | 33584 | 14168 | 76747 | 3 |
| proxiableUUID | 363 | 363 | 363 | 363 | 3 |
| rewardIndex | 394 | 420 | 394 | 2394 | 76 |
| setTrustedCodehash | 26226 | 26226 | 26226 | 26226 | 48 |
| stake | 134677 | 171246 | 176279 | 196758 | 59 |
| totalMP | 329 | 329 | 329 | 329 | 79 |
| totalMaxMP | 373 | 373 | 373 | 373 | 79 |
| totalStaked | 329 | 329 | 329 | 329 | 79 |
| rewardIndex | 394 | 419 | 394 | 2394 | 77 |
| setTrustedCodehash | 26226 | 26226 | 26226 | 26226 | 49 |
| stake | 134677 | 171329 | 176279 | 196758 | 60 |
| totalMP | 329 | 329 | 329 | 329 | 80 |
| totalMaxMP | 373 | 373 | 373 | 373 | 80 |
| totalStaked | 329 | 329 | 329 | 329 | 80 |
| unstake | 63997 | 84793 | 63997 | 120464 | 13 |
| updateAccountMP | 15353 | 17591 | 17855 | 17855 | 19 |
| updateGlobalState | 11066 | 41310 | 30530 | 63481 | 28 |
| upgradeToAndCall | 3124 | 8887 | 10809 | 10809 | 4 |
| upgradeToAndCall | 3124 | 9263 | 10809 | 10809 | 5 |


| src/StakeManagerProxy.sol:StakeManagerProxy contract | | | | | |
Expand All @@ -54,21 +54,21 @@
| MIN_LOCKUP_PERIOD | 701 | 3973 | 5201 | 5201 | 11 |
| MP_RATE_PER_YEAR | 657 | 657 | 657 | 657 | 3 |
| SCALE_FACTOR | 721 | 721 | 721 | 721 | 41 |
| STAKING_TOKEN | 7308 | 7308 | 7308 | 7308 | 192 |
| accountedRewards | 800 | 1378 | 800 | 2800 | 76 |
| STAKING_TOKEN | 7308 | 7308 | 7308 | 7308 | 196 |
| accountedRewards | 800 | 1397 | 800 | 2800 | 77 |
| emergencyModeEnabled | 7325 | 7325 | 7325 | 7325 | 7 |
| enableEmergencyMode | 28455 | 45356 | 50640 | 50640 | 8 |
| getAccount | 2075 | 2075 | 2075 | 2075 | 72 |
| implementation | 343 | 934 | 343 | 2343 | 274 |
| isTrustedCodehash | 993 | 1493 | 993 | 2993 | 192 |
| rewardIndex | 821 | 847 | 821 | 2821 | 76 |
| setTrustedCodehash | 52872 | 52872 | 52872 | 52872 | 48 |
| totalMP | 756 | 756 | 756 | 756 | 79 |
| totalMaxMP | 800 | 800 | 800 | 800 | 79 |
| totalStaked | 756 | 756 | 756 | 756 | 79 |
| implementation | 343 | 935 | 343 | 2343 | 280 |
| isTrustedCodehash | 993 | 1493 | 993 | 2993 | 196 |
| rewardIndex | 821 | 846 | 821 | 2821 | 77 |
| setTrustedCodehash | 52872 | 52872 | 52872 | 52872 | 49 |
| totalMP | 756 | 756 | 756 | 756 | 80 |
| totalMaxMP | 800 | 800 | 800 | 800 | 80 |
| totalStaked | 756 | 756 | 756 | 756 | 80 |
| updateAccountMP | 41712 | 43950 | 44214 | 44214 | 19 |
| updateGlobalState | 37054 | 67298 | 56518 | 89469 | 28 |
| upgradeToAndCall | 29767 | 35525 | 37445 | 37445 | 4 |
| upgradeToAndCall | 29767 | 35900 | 37445 | 37445 | 5 |


| src/StakeVault.sol:StakeVault contract | | | | | |
Expand All @@ -77,9 +77,9 @@
| 1374543 | 6483 | | | | |
| Function Name | min | avg | median | max | # calls |
| emergencyExit | 36353 | 48857 | 48091 | 65191 | 7 |
| leave | 33507 | 53144 | 33507 | 92418 | 3 |
| leave | 33507 | 132602 | 62962 | 370978 | 4 |
| lock | 33245 | 60236 | 48577 | 110544 | 4 |
| stake | 33454 | 242265 | 250826 | 271353 | 60 |
| stake | 33454 | 242405 | 250826 | 271353 | 61 |
| trustStakeManager | 28997 | 28997 | 28997 | 28997 | 1 |
| unstake | 33260 | 117303 | 113502 | 156376 | 14 |

Expand Down Expand Up @@ -157,12 +157,21 @@
| Deployment Cost | Deployment Size | | | | |
| 625454 | 3260 | | | | |
| Function Name | min | avg | median | max | # calls |
| approve | 46330 | 46339 | 46342 | 46342 | 245 |
| balanceOf | 558 | 1386 | 558 | 2558 | 345 |
| mint | 51279 | 58746 | 51279 | 68379 | 261 |
| approve | 46330 | 46339 | 46342 | 46342 | 250 |
| balanceOf | 558 | 1394 | 558 | 2558 | 349 |
| mint | 51279 | 58734 | 51279 | 68379 | 266 |
| transfer | 34384 | 48853 | 51484 | 51484 | 13 |


| test/mocks/StackOverflowStakeManager.sol:StackOverflowStakeManager contract | | | | | |
|-----------------------------------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost | Deployment Size | | | | |
| 1033443 | 4615 | | | | |
| Function Name | min | avg | median | max | # calls |
| leave | 391 | 161316 | 161316 | 322322 | 334 |
| proxiableUUID | 319 | 319 | 319 | 319 | 1 |


| test/mocks/XPProviderMock.sol:XPProviderMock contract | | | | | |
|-------------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost | Deployment Size | | | | |
Expand Down
1 change: 1 addition & 0 deletions .gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ LeaveTest:test_TrustNewStakeManager() (gas: 2642695)
LockTest:test_LockFailsWithInvalidPeriod() (gas: 306175)
LockTest:test_LockFailsWithNoStake() (gas: 61440)
LockTest:test_LockWithoutPriorLock() (gas: 403273)
MaliciousUpgradeTest:test_UpgradeStackOverflowStakeManager() (gas: 1761895)
NFTMetadataGeneratorSVGTest:testGenerateMetadata() (gas: 85934)
NFTMetadataGeneratorSVGTest:testSetImageStrings() (gas: 58332)
NFTMetadataGeneratorSVGTest:testSetImageStringsRevert() (gas: 35804)
Expand Down
41 changes: 41 additions & 0 deletions test/RewardsStreamerMP.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { StakeVault } from "../src/StakeVault.sol";
import { IStakeManagerProxy } from "../src/interfaces/IStakeManagerProxy.sol";
import { StakeManagerProxy } from "../src/StakeManagerProxy.sol";
import { MockToken } from "./mocks/MockToken.sol";
import { StackOverflowStakeManager } from "./mocks/StackOverflowStakeManager.sol";

contract RewardsStreamerMPTest is Test {
MockToken rewardToken;
Expand Down Expand Up @@ -1948,3 +1949,43 @@ contract LeaveTest is RewardsStreamerMPTest {
_leave(alice);
}
}

contract MaliciousUpgradeTest is RewardsStreamerMPTest {
function _upgradeStakeManager() internal {
address newImpl = address(new RewardsStreamerMP());
bytes memory initializeData;
UUPSUpgradeable(streamer).upgradeToAndCall(newImpl, initializeData);
}

function setUp() public override {
super.setUp();
}

function test_UpgradeStackOverflowStakeManager() public {
uint256 aliceInitialBalance = stakingToken.balanceOf(alice);

// first change the existing manager's state
_stake(alice, 100e18, 0);
checkStreamer(
CheckStreamerParams({
totalStaked: 100e18,
totalMP: 100e18,
totalMaxMP: 500e18,
stakingBalance: 100e18,
rewardBalance: 0,
rewardIndex: 0,
accountedRewards: 0
})
);

// upgrade the manager to a malicious one
address newImpl = address(new StackOverflowStakeManager());
bytes memory initializeData;
UUPSUpgradeable(streamer).upgradeToAndCall(newImpl, initializeData);

// alice leaves system and is able to get funds out, despite malicious manager
_leave(alice);

assertEq(stakingToken.balanceOf(alice), aliceInitialBalance, "Alice should get her tokens back");
}
}
69 changes: 69 additions & 0 deletions test/mocks/StackOverflowStakeManager.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.26;

import { IStakeManager } from "./../../src/interfaces/IStakeManager.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { TrustedCodehashAccess } from "./../../src/TrustedCodehashAccess.sol";
import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol";

contract StackOverflowStakeManager is
UUPSUpgradeable,
IStakeManager,
TrustedCodehashAccess,
ReentrancyGuardUpgradeable
{
IERC20 public STAKING_TOKEN;

Check warning on line 16 in test/mocks/StackOverflowStakeManager.sol

View workflow job for this annotation

GitHub Actions / lint

Variable name must be in mixedCase
IERC20 public REWARD_TOKEN;

Check warning on line 17 in test/mocks/StackOverflowStakeManager.sol

View workflow job for this annotation

GitHub Actions / lint

Variable name must be in mixedCase

uint256 public constant SCALE_FACTOR = 1e18;
uint256 public constant MP_RATE_PER_YEAR = 1e18;

uint256 public constant MIN_LOCKUP_PERIOD = 90 days;
uint256 public constant MAX_LOCKUP_PERIOD = 4 * 365 days;
uint256 public constant MAX_MULTIPLIER = 4;

uint256 public totalStaked;
uint256 public totalMP;
uint256 public totalMaxMP;
uint256 public rewardIndex;
uint256 public accountedRewards;
uint256 public lastMPUpdatedTime;
bool public emergencyModeEnabled;

struct Account {
uint256 stakedBalance;
uint256 accountRewardIndex;
uint256 accountMP;
uint256 maxMP;
uint256 lastMPUpdateTime;
uint256 lockUntil;
}

mapping(address account => Account data) public accounts;

function getStakedBalance(address _vault) external view override returns (uint256 _balance) {

Check warning on line 45 in test/mocks/StackOverflowStakeManager.sol

View workflow job for this annotation

GitHub Actions / lint

Code contains empty blocks
// implementation
}
function lock(uint256 _seconds) external override {

Check warning on line 48 in test/mocks/StackOverflowStakeManager.sol

View workflow job for this annotation

GitHub Actions / lint

Code contains empty blocks
// implementation
}
function stake(uint256 _amount, uint256 _seconds) external override {

Check warning on line 51 in test/mocks/StackOverflowStakeManager.sol

View workflow job for this annotation

GitHub Actions / lint

Code contains empty blocks
// implementation
}
function unstake(uint256 _amount) external override {

Check warning on line 54 in test/mocks/StackOverflowStakeManager.sol

View workflow job for this annotation

GitHub Actions / lint

Code contains empty blocks
// implementation
}

function leave() external override {
this.leave();
}

function _authorizeUpgrade(address) internal view override {
_checkOwner();
}

function getAccount(address _account) external view returns (Account memory) {
return accounts[_account];
}
}

0 comments on commit dfe8929

Please sign in to comment.