Summary
When someone wants to stake the amount is deposited to StakingPool via deposit function which is called by PriorityPool.
Then the _mint will be called which eventually calls _mintShares
In order to prevent inflation attacks protocol deduct DEAD_SHARES from the deposited amount if pool is empty and no shares are minted yet, as we can see here in _mintShares function which is called by _mint function:
* @notice Mints new shares to an account
* @param _recipient account to mint shares for
* @param _amount shares amount
*/
function _mintShares(address _recipient, uint256 _amount) internal {
require(_recipient != address(0), "Mint to the zero address");
if (totalShares == 0) {
shares[address(0)] = DEAD_SHARES;
totalShares = DEAD_SHARES;
_amount -= DEAD_SHARES;
}
totalShares += _amount;
shares[_recipient] += _amount;
}
Vulnerability Details
The issue here is that the amount accounted in StakingPool::deposit in totalStaked is the full amount not the deducted amount as we can see here:
if (_amount > 0) {
token.safeTransferFrom(msg.sender, address(this), _amount);
_depositLiquidity(_data);
_mint(_account, _amount);
totalStaked += _amount;
But the amount of shares minted is with the deducted amount (_amount - DEAD_SHARES) as we can see in _mintShares function above.
So the users who deposit first will incur loss of funds and also possibly users who deposits after.
PoC:
For the sake of simplicity I will use following values:
deposited amount of assets will be 1 (_amount = 1)
DEAD_SHARES = 0.3
So user want to deposit 1 to the StakingPool. The _mint function is called with amount value of 1.
So when minting shares since there are no deposits before the totalShares minted will be 0 and it will trigger the following actions:
if (totalShares == 0) {
shares[address(0)] = DEAD_SHARES;
totalShares = DEAD_SHARES;
_amount -= DEAD_SHARES;
}
So the actual amount minted will be 0.7 (1 - 0.3 = 0.7) to the user and also totalShares will be 0.7 now.
But in StakingPool the full amount of 1 will be accounted. So in the end protocol will account that user staked 1 token but in reality user got 0.7 amount of shares not full amount which will make the remaining amount unredeemable for the user and stuck forever.
Impact
The impact of this is that user will incur loss of funds, funds will be stuck and the protocol will have accounted wrong amounts and it will have wrong data which could impact further functionality and thrust of the protocol.
Tools Used
Manual Reviev
Recommendations
In order to users does not incure losses and protocol have correct accounting and data make the following changes:
Add following changes to StakingPool and modify deposit function like this:
.
.
.
// list of all strategies controlled by pool
address[] private strategies;
// total number of tokens staked in the pool
uint256 public totalStaked;
// max number of tokens that can sit in the pool outside of a strategy
uint256 public unusedDepositLimit;
// used to prevent vault inflation attack
+ uint256 private constant DEAD_SHARES = 10 ** 3;
.
.
.
function deposit(
address _account,
uint256 _amount,
bytes[] calldata _data
) external onlyPriorityPool {
require(strategies.length > 0, "Must be > 0 strategies to stake");
uint256 startingBalance = token.balanceOf(address(this));
if (_amount > 0) {
token.safeTransferFrom(msg.sender, address(this), _amount);
_depositLiquidity(_data);
_mint(_account, _amount);
+ if (totalStaked == 0) {
+ totalStaked = _amount - DEAD_SHARES;
+ } else {
+ totalStaked += _amount;
+ }
} else {
_depositLiquidity(_data);
}
uint256 endingBalance = token.balanceOf(address(this));
if (endingBalance > startingBalance && endingBalance > unusedDepositLimit)
revert InvalidDeposit();
}
.
.
.
+ function getTotalStake() public view returns (uint256){
+ return _totalStaked();
+ }
/**
* @notice Returns the total amount of asset tokens staked in the pool
* @return the total staked amount
*/
function _totalStaked() internal view override returns (uint256) {
return totalStaked;
}
Add the following in IStakingPool.sol:
.
.
.
.
function getUnusedDeposits() external view returns (uint256);
function burn(uint256 _amount) external;
+ function getTotalStake() external view returns (uint256);
Also make following changes in PriorityPool.sol:
.
.
.
// total number of tokens queued for deposit into the staking pool
uint256 public totalQueued;
// total number of tokens deposited into the staking pool since the last distribution
uint256 public depositsSinceLastUpdate;
// total number of shares received for tokens deposited into the staking pool since the last distribution
uint256 private sharesSinceLastUpdate;
// used to prevent vault inflation attack
+ uint256 private constant DEAD_SHARES = 10 ** 3;
.
.
.
function onTokenTransfer(address _sender, uint256 _value, bytes calldata _calldata) external {
if (_value == 0) revert InvalidValue();
(bool shouldQueue, bytes[] memory data) = abi.decode(_calldata, (bool, bytes[]));
if (msg.sender == address(token)) {
+ if (stakingPool.getTotalStake() == 0) {
+ _value -= DEAD_SHARES;
+ }
_deposit(_sender, _value, shouldQueue, data);
} else if (msg.sender == address(stakingPool)) {
uint256 amountQueued = _withdraw(_sender, _value, shouldQueue);
token.safeTransfer(_sender, _value - amountQueued);
} else {
revert UnauthorizedToken();
}
}
.
.
.
function deposit(uint256 _amount, bool _shouldQueue, bytes[] calldata _data) external {
if (_amount == 0) revert InvalidAmount();
+ if (stakingPool.getTotalStake() == 0) {
+ _amount -= DEAD_SHARES;
+ }
token.safeTransferFrom(msg.sender, address(this), _amount);
_deposit(msg.sender, _amount, _shouldQueue, _data);
}
