deposit() function is open to reentrancy attacks

[code423n4]

Handle

jayjonah8

Vulnerability details

Impact

In Vault.sol the deposit() function is left wide open to reentrancy attacks. The function eventually calls _createDeposit() => _createClaim() which calls depositors.mint() which will then mint an NFT. When the NFT is minted the sender will receive a callback which can then be used to call the deposit() function again before execution is finished. An attacker can do this minting multiple NFT's for themselves. claimers.mint() is also called in the same function which can also be used to call back into the deposit function before execution is complete. Since there are several state updates before and after NFT's are minted this can be used to further manipulate the protocol like with newShares which is called before minting. This is not counting what an attacker can do with cross function reentrancy entering into several other protocol functions (like withdraw) before code execution is complete further manipulating the system.

Proof of Concept

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L160

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L470

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L476

Tools Used

Manual code review

Recommended Mitigation Steps

Reentrancy guard modifiers should be placed on the deposit(), withdraw() and all other important protocol functions to prevent devastating attacks.

[naps62]

Fixed in https://github.com/sandclock-org/solidity-contracts/pull/75