First Flight #9: Soulmate

Summary

In Staking.sol, the functions deposit, claimRewards and withdraw all do not follow CEI. The emits of the events are after external transfers have been done.

Vulnerability Details

The events in the Staking.sol contract are emitted after external transfers have been executed in the functions deposit, withdraw, and claimRewards. This violates the Checks-Effects-Interactions pattern, a best practice in Solidity smart contract development. According to this pattern, external calls (interactions) should be made after state changes (effects) to prevent reentrancy attacks and ensure that contract state is consistent before interacting with other contracts or external entities.

Impact

Without a dApp, this protocol is no more than a couple of contracts. So inevitably there will have to be an off-chain (website) that will be listening and acting to events being emitted by the smart-contracts.

Proof of Concept

consider a scenario where a malicious contract exploits the lack of proper ordering in the deposit function. By calling the deposit function from a contract that performs a reentrancy attack, it could potentially manipulate the contract state or funds before the Deposited event is emitted, leading to unintended consequences or loss of funds for legitimate users.
Patrick Collins himself validates this finding not one week ago:
https://www.linkedin.com/posts/patrickalphac_a-lot-of-people-think-its-fine-to-emit-events-activity-7160679494452715524-Ou3M?utm_source=share&utm_medium=member_desktop

Tools Used

manual review

Recommendations

Emit events before executing external transfers or interactions.