sherlock-audit / 2022-11-telcoin-judging

0 stars 0 forks source link

WATCHPUG - Flashloan `TEL` tokens to stake and exit in the same block can fake a huge amount of stake with minimal material cost #83

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

WATCHPUG

high

Flashloan TEL tokens to stake and exit in the same block can fake a huge amount of stake with minimal material cost

Summary

Checkpoints#getAtBlock() can be faked with falshloan as it may return the value of the first checkpoint in the same block.

Vulnerability Detail

Checkpoints#getAtBlock() will return the value on check point #0 when there are two check points in the same block (#0 and #1).

Therefore, one can take a falshloan of TEL tokens to stake and exit in the same block, which will create two checkpoints.

Impact

Malicious user can fake their stake to gain a high percentage rewards with falshloan and avoid slashing.

Code Snippet

https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/StakingModule.sol#L147-L149

https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/StakingModule.sol#L403-L406

Tool used

Manual Review

Recommendation

Consider requiring the exit to be at least 1 block later than the blocknumber of the original stake.

amshirif commented 1 year ago

https://github.com/telcoin/telcoin-staking/pull/9

dmitriia commented 1 year ago

Escalate for 30 USDC I apologize if missed some point, but how this can be used to drain funds from the protocol?

stakedByAt is used only at balanceOfAt(): https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/StakingModule.sol#L98-L104

Which is stand-alone view function: https://github.com/sherlock-audit/2022-11-telcoin/search?q=balanceOfAt

Slashing example is clear, but it uses latest(), not stakedByAt(): https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/StakingModule.sol#L403-L406 https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/StakingModule.sol#L356-L360

For the latest() Checkpoints will return the latest entry, after the flash loan: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/utils/CheckpointsUpgradeable.sol#L94-L100

I.e. if an attacker front-run the slashing, his flashloan will be finished when slashing run and the latest, small, entry will be used in claiming.

Am I missing something here?

If not the severity should be Med as view function is impacted, which can backfire downstream, but that's an assumption typically meaning downgrading the severity.

sherlock-admin commented 1 year ago

Escalate for 30 USDC I apologize if missed some point, but how this can be used to drain funds from the protocol?

stakedByAt is used only at balanceOfAt(): https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/StakingModule.sol#L98-L104

Which is stand-alone view function: https://github.com/sherlock-audit/2022-11-telcoin/search?q=balanceOfAt

Slashing example is clear, but it uses latest(), not stakedByAt(): https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/StakingModule.sol#L403-L406 https://github.com/sherlock-audit/2022-11-telcoin/blob/main/contracts/StakingModule.sol#L356-L360

For the latest() Checkpoints will return the latest entry, after the flash loan: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/utils/CheckpointsUpgradeable.sol#L94-L100

I.e. if an attacker front-run the slashing, his flashloan will be finished when slashing run and the latest, small, entry will be used in claiming.

Am I missing something here?

If not the severity should be Med as view function is impacted, which can backfire downstream, but that's an assumption typically meaning downgrading the severity.

You've created a valid escalation for 30 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

hrishibhat commented 1 year ago

Escalation accepted

After consulting with the sponsors, judges took a deep dive on the issue and decided to make this a medium severity.

sherlock-admin commented 1 year ago

Escalation accepted

After consulting with the sponsors, judges took a deep dive on the issue and decided to make this a medium severity.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

jack-the-pug commented 1 year ago

Fix confirmed