Platform: Code4rena
Start Date: 06/01/2022
Pot Size: $60,000 USDC
Total HM: 20
Participants: 33
Period: 7 days
Judge: LSDan
Total Solo HM: 9
Id: 67
League: ETH
Rank: 3/33
Findings: 4
Award: $3,260.92
🌟 Selected for report: 4
🚀 Solo Findings: 0
90.0579 USDC - $90.06
jayjonah8
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.
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
Manual code review
Reentrancy guard modifiers should be placed on the deposit(), withdraw() and all other important protocol functions to prevent devastating attacks.
#0 - naps62
2022-02-21T13:25:40Z
90.0579 USDC - $90.06
jayjonah8
In Depositors.sol the mint() function eventually calls _safeMint() which has a callback that can be used to reenter the function. Since state updates are made after the call to _safeMint this is dangerous because the function can be reentered multiple times while the deposits are only updated once.
Manual code review
Add a reentrancy guard modifier to the mint() function in Depositors.sol
#0 - r2moon
2022-01-11T16:35:31Z
2657.2374 USDC - $2,657.24
jayjonah8
In Vault.sol the sponsor() function does not have a reentrancy guard allowing an attacker to reenter the function because the depositors.mint() function has as callback to the msg.sender. Since there are state updates after the call to depositors.mint() function this is especially dangerous. An attacker can make it so the totalSponsored amount is only updated once after calling mint() several times since the update takes place after the callback. The same will be true for the Sponsored event that is emitted.
https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L244
Manual code review
A reentrancy guard modifier should be added to the sponsor() function in Vault.sol
#0 - naps62
2022-01-13T11:28:29Z
#1 - MrToph
2022-03-02T16:38:24Z
An attacker can make it so the totalSponsored amount is only updated once after calling mint() several times since the update takes place after the callback.
given the high severity, I would have liked to see how this can be used to steal funds or break functionality. attack path is not clear to me
🌟 Selected for report: jayjonah8
Also found by: bugwriter001, camden, palina, sirhashalot
232.4551 USDC - $232.46
jayjonah8
In Vault.sol the deposit() function eventually calls claimers.mint() and depositers.mint(). Calling mint this way does not ensure that the receiver of the NFT is able to accept them. _safeMint() should be used with reentrancy guards as a guard to protect the user as it checks to see if a user can properly accept an NFT and reverts otherwise.
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#L256
Manual code review
Use _safeMint() instead of mint()
#0 - r2moon
2022-01-07T16:13:22Z
I think _safeMint check if the recipient contract is able to accept NFT, it does not involves any issues. However we will use _safeMint.
#1 - gabrielpoca
2022-01-12T10:32:51Z
@ryuheimat this is a non-issue. The mint functions called in the Vault's deposit function are implemented by us, they just happen to be called mint.
#2 - dmvt
2022-01-28T13:21:25Z
The Depositors contract does use _safeMint, but the Claimers contract does not.
See: https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/vault/Claimers.sol#L63 https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/vault/Depositors.sol#L53
The deposit function on Vault also appears to lack reentrancy guards. The issue is valid and should be addressed, despite the fact that the warden clearly did not look at the Depositors contract to see that it already used _safeMint.
#3 - naps62
2022-02-21T13:30:29Z
This is already fixed
15.442 USDC - $15.44
jayjonah8
In Claimers.sol there's an open TODO. These can point to potential problems that can be exploited and should be resolved and removed from the code.
https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/vault/Claimers.sol#L32
Manual code review
Open Todos should be resolved and removed
#0 - r2moon
2022-01-07T16:11:51Z
We will remove them later, but they are out of scope. They won't lead any risks
#1 - dmvt
2022-01-28T13:37:27Z
Claimers is in scope and TODOs are code smells.
1 — Low: Low: Assets are not at risk. State handling, function incorrect as to spec, issues with comments.
#2 - dmvt
2022-01-28T20:21:20Z
duplicate of #96
265.7237 USDC - $265.72
jayjonah8
In Vault.sol as well as other files, floating pragmas are used. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
https://swcregistry.io/docs/SWC-103
https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L2
Manual code review
change pragma statements from: "pragma solidity ^0.8.10;" to "pragma solidity 0.8.10;"
#0 - naps62
2022-02-16T15:51:25Z
already fixed