Submission Details
Severity:
`TempleGoldStaking::withdraw` and `TempleGoldStaking::withdrawAll` Allowing Withdrawals During A Protocol Pause
## Relevant Github Links
https://github.com/Cyfrin/2024-07-templegold/blob/main/protocol/contracts/templegold/TempleGoldStaking.sol#L276
https://github.com/Cyfrin/2024-07-templegold/blob/main/protocol/contracts/templegold/TempleGoldStaking.sol#L285
## Summary
`TempleGoldStaking::withdraw` and `TempleGoldStaking::withdrawAll` allowing withdrawals during a protocol pause caused by unexpected vulnerabilities. This scenario opens the door for users to withdraw incorrectly calculated tokens due to errors in token calculations or front-end vulnerabilities, potentially resulting in financial losses for both the protocol and its users. Math-related vulnerabilities, such as overflow/underflow errors, also grow these risks by enabling incorrect token amounts to be processed during withdrawals.
## Vulnerability Details
`TempleGoldStaking.sol` will be paused if a vulnerability is found, disabling staking. Although the specific vulnerabilities are still unknown, they could potentially harm the protocol or its users. However, the current pause mechanism is inadequate for fully protecting users or the protocol from such vulnerabilities.
The pause mechanism is intended to mitigate unexpected vulnerabilities that may exist in the code once it has gone live. Therefore, it must be assumed that such vulnerabilities exist. While the protocol disables staking during a pause, it does not disable withdrawals. If these unexpected vulnerabilities threaten the funds of users or the protocol, an attacker could still exploit them to drain all available funds from the protocol or users.
The `TempleGoldStaking::withdraw` and `TempleGoldStaking::withdrawAll` function remains callable even when the protocol is paused.
```solidity
/**
* @notice Withdraw staked tokens
* @param amount Amount to withdraw
* @param claim Boolean if to claim rewards
*/
function withdraw(uint256 amount, uint256 index, bool claim) external override {
StakeInfo storage _stakeInfo = _stakeInfos[msg.sender][index];
_withdrawFor(_stakeInfo, msg.sender, msg.sender, index, amount, claim, msg.sender);
}
```
```solidity
/**
* @notice Withdraw all staked tokens
* @param claim Boolean if to claim rewards
*/
function withdrawAll(uint256 stakeIndex, bool claim) external override {
StakeInfo storage _stakeInfo = _stakeInfos[msg.sender][stakeIndex];
_withdrawFor(_stakeInfo, msg.sender, msg.sender, stakeIndex, _stakeInfo.amount, claim, msg.sender);
}
```
Example scenario:
* SushiSwap's token platform, MISO, experienced a significant hack resulting in the theft of 864.8 Ethereum, worth approximately $3 million. SushiSwap, a leading decentralized exchange (DEX) rivaling Uniswap, saw over $495 million in trading volume in the past 24 hours. MISO, which facilitates launching new projects on SushiSwap, was compromised through a supply chain attack.
An anonymous contractor, using the GitHub handle AristoK3, injected malicious code into the platform's front end, redirecting funds from the @JayPegsAutoMart NFT auction to their own wallet. The issue has since been patched. According to SushiSwap’s CTO, Joseph Delong, the attack occurred at 12:04 pm Eastern time on Thursday. By 9:45 am Eastern time on Friday, Delong confirmed the return of all stolen funds. ([Reference](https://decrypt.co/81120/sushiswaps-token-launchpad-hacked-over-3m-ethereum))
Key takeaway: Vulnerabilities can originate from various sources, including the platform's frontend. In cases where malicious code is injected and puts the protocol or users' funds at risk, the potential for funds to be redirected from intended recipients to attackers remains even after the vulnerabilities have been detected by the protocol. Once a codebase has been deployed live, the protocol may have limited recourse to prevent such exploits.
* If there is an error in the token calculation or user input, users can still withdraw tokens that have been incorrectly calculated. Math-related vulnerabilities, such as overflow/underflow errors and precision loss, are particularly relevant in this context. These issues can lead to incorrect token amounts being calculated during withdrawals. If the received tokens are more than they should be, the protocol stands to lose money. Conversely, if the received tokens are less than they should be, users will lose money. If the protocol stands to lose money, a malicious user could exploit this vulnerability to drain the protocol's funds.
## Impact
The `TempleGoldStaking::withdraw` and `TempleGoldStaking::withdrawAll` function remaining callable even during a protocol pause due to detected vulnerabilities presents a significant risk. If the error are related in token calculations or client input, users can withdraw incorrectly calculated tokens, potentially leading to financial losses for both the protocol and users. Math-related vulnerabilities like overflow/underflow errors further compound these risks, allowing for incorrect token amounts during withdrawals.
This situation leads the protocol and users to financial losses.
## Tools Used
Manual Review
## Recommendations
```diff
/**
* @notice Withdraw staked tokens
* @param amount Amount to withdraw
* @param claim Boolean if to claim rewards
*/
- function withdraw(uint256 amount, uint256 index, bool claim) external override {
+ function withdraw(uint256 amount, uint256 index, bool claim) external override whenNotPaused{
StakeInfo storage _stakeInfo = _stakeInfos[msg.sender][index];
_withdrawFor(_stakeInfo, msg.sender, msg.sender, index, amount, claim, msg.sender);
}
```
```diff
/**
* @notice Withdraw all staked tokens
* @param claim Boolean if to claim rewards
*/
- function withdrawAll(uint256 stakeIndex, bool claim) external override {
+ function withdrawAll(uint256 stakeIndex, bool claim) external override whenNotPaused {
StakeInfo storage _stakeInfo = _stakeInfos[msg.sender][stakeIndex];
_withdrawFor(_stakeInfo, msg.sender, msg.sender, stakeIndex, _stakeInfo.amount, claim, msg.sender);
}
```
Updates
Lead Judging Commences
inallhonesty about 1 year ago
Submission Judgement Published Assigned finding tags:
Only migrator should be able to perform actions when contract is paused.
Prize pool breakdown
Total prize
25,000 USDC
nSLOC
99825 USDC / LOC
20,000 USDC
2,500 USDC
2,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.