Vulnerability Details
To account the fees mintFee() function mints vault tokens to the treasury. Since the amount of pendingFee is directly proportional to the totalSupply of the vault tokens, this will have a compounding effect.
function mintFee() public {
_mint(_store.treasury, GMXReader.pendingFee(_store));
_store.lastFeeCollected = block.timestamp;
}
https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXVault.sol#L334-L337
function pendingFee(GMXTypes.Store storage self) public view returns (uint256) {
uint256 totalSupply_ = IERC20(address(self.vault)).totalSupply();
uint256 _secondsFromLastCollection = block.timestamp - self.lastFeeCollected;
return (totalSupply_ * self.feePerSecond * _secondsFromLastCollection) / SAFE_MULTIPLIER;
}
https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXReader.sol#L38-L42
POC Test
Apply the following diff and run forge test --mt test_compoundedFee
@@ -346,5 +346,48 @@ contract GMXDepositTest is GMXMockVaultSetup, GMXTestHelper, TestUtils {
require(IERC20(vault).balanceOf(address(treasury)) > treasuryBal, "treasury balance should increase");
}
+ function test_compoundedFee() external {
+ // higher the deposit, higher the vault tokens minted for the deposit and hence higher the minted vault tokens for the treasury making the compounding effect more significant
+ deal(address(WETH), user1, 100 ether);
+ uint256 snapshot = vm.snapshot();
+ vm.startPrank(user1);
+ uint256 treasuryBal = IERC20(vault).balanceOf(address(treasury));
+ // setup deposit
+ _createDeposit(address(WETH), 20e18, 0, SLIPPAGE, EXECUTION_FEE);
+ mockExchangeRouter.executeDeposit(
+ address(WETH),
+ address(USDC),
+ address(vault),
+ address(callback)
+ );
+
+ skip(30 days);
+ vault.mintFee();
+
+ uint nonCompoundedFeesCollected = IERC20(vault).balanceOf(address(treasury)) - treasuryBal;
+
+
+ vm.revertTo(snapshot);
+ require(IERC20(vault).balanceOf(address(treasury)) == treasuryBal);
+
+ // setup deposit
+ _createDeposit(address(WETH), 20e18, 0, SLIPPAGE, EXECUTION_FEE);
+ mockExchangeRouter.executeDeposit(
+ address(WETH),
+ address(USDC),
+ address(vault),
+ address(callback)
+ );
+
+ skip(15 days);
+ vault.mintFee();
+ skip(15 days);
+ vault.mintFee();
+
+ uint compoundedFeesCollected = IERC20(vault).balanceOf(address(treasury)) - treasuryBal;
+
+ require(compoundedFeesCollected > nonCompoundedFeesCollected);
+
+ }
}
Impact
User's can end up paying more fees than calculated
Recommendations
Eliminate the mintedFeeTokens from being involved in the pendingFee calculation.
One way to achieve this is by keeping track of activeFeeToken amount in a separate variable and applying the following formula for pendingFee:
function pendingFee(GMXTypes.Store storage self) public view returns (uint256) {
uint256 totalSupply_ = IERC20(address(self.vault)).totalSupply();
uint256 _secondsFromLastCollection = block.timestamp - self.lastFeeCollected;
-- return (totalSupply_ * self.feePerSecond * _secondsFromLastCollection) / SAFE_MULTIPLIER;
++ return ((totalSupply_ - activeFeeTokenAmount) * self.feePerSecond * _secondsFromLastCollection) / SAFE_MULTIPLIER;
}
