Upon calling the unstake function, it ensures that the unstaking service collects any pending rewards:
if (reward > 0) {
_withdraw(multisig, reward);
}
The withdraw implementation is different, depending on wether the Staking contract distributes tokens or native currency as rewards, but it essentially boils down to subtracting rewards from the current balance and transferring them to the service multisig - balance -= amount;.
The mandatory collection of reward can cause the service to be staked for much longer than it's owner intended to. Consider a situation where the Staking contract has no funds to distribute and all calls to unstake revert due to an underflow in the _withdraw function: balance -= amount;.
Root Cause
Lack of force unstake mechanism that will forfeit rewards in favour of the service owner being able to stake elsewhere.
Impact
Having a Staking contract without funds to distribute can be especially damaging in cases where there are multiple services staked with pending rewards. In such situations owners will either have to wait for new rewards without any guarantees of new funds arriving or they would have to deposit themselves so they can unstake. The second option puts their funds at risk, as another service could use their deposit to unstake before them.
The lack of force unstake creates a peculiar situation where service owners could incur opportunity loss or have to potentially pay to unstake (with the risk of losing funds).
PoC
Lets suppose the following turn of events:
Staking contract stops receiving regular funding
Services have rewards pending that are more than the current balance in the contract.
Service owners can't unstake and move to a more profitable Staking contract.
If a service owner deposits in the Staking contract themselves, they could get frontrun by another owner and lose their funds.
Suggested Mitigation
Add a forceUnstake flag that will allow unstake without receiving the pending rewards. It could look something like this:
if (forceUnstake) {
sInfo.reward = 0;
reward = 0;
} else if (reward > 0) {
_withdraw(multisig, reward);
}
Lines of code
https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingBase.sol#L867-L869 https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingToken.sol#L104-L111 https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingNativeToken.sol#L28-L37
Vulnerability details
Relevant code: StakingBase::unstake, StakingToken::_withdraw, StakingNativeToken::_withdraw
Description
Upon calling the
unstake
function, it ensures that the unstaking service collects any pending rewards:The withdraw implementation is different, depending on wether the
Staking
contract distributes tokens or native currency as rewards, but it essentially boils down to subtracting rewards from the current balance and transferring them to the service multisig -balance -= amount;
.The mandatory collection of reward can cause the service to be staked for much longer than it's owner intended to. Consider a situation where the
Staking
contract has no funds to distribute and all calls tounstake
revert due to an underflow in the_withdraw
function:balance -= amount;
.Root Cause
Lack of force unstake mechanism that will forfeit rewards in favour of the service owner being able to stake elsewhere.
Impact
Having a
Staking
contract without funds to distribute can be especially damaging in cases where there are multiple services staked with pending rewards. In such situations owners will either have to wait for new rewards without any guarantees of new funds arriving or they would have to deposit themselves so they can unstake. The second option puts their funds at risk, as another service could use their deposit to unstake before them.The lack of force unstake creates a peculiar situation where service owners could incur opportunity loss or have to potentially pay to unstake (with the risk of losing funds).
PoC
Lets suppose the following turn of events:
Staking
contract stops receiving regular fundingbalance
in the contract.Staking
contract.Staking
contract themselves, they could get frontrun by another owner and lose their funds.Suggested Mitigation
Add a
forceUnstake
flag that will allowunstake
without receiving the pending rewards. It could look something like this:Assessed type
DoS