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: 4/33
Findings: 3
Award: $3,057.99
🌟 Selected for report: 3
🚀 Solo Findings: 1
pauliax
exchange_underlying in functions _swapUnderlyingToUst and _swapUstToUnderlying lack slippage control, it uses a default value of 0 minimum received.
A common attack in DeFi is the sandwich attack. Upon observing a trade of asset X for asset Y, an attacker frontruns the victim trade by also buying asset Y, lets the victim execute the trade, and then backruns (executes after) the victim by trading back the amount gained in the first trade. Intuitively, one uses the knowledge that someone’s going to buy an asset, and that this trade will increase its price, to make a profit. The attacker’s plan is to buy this asset cheap, let the victim buy at an increased price, and then sell the received amount again at a higher price afterwards.
Consider making this slippage parameter configurable, so you can set it if it will be exploited by the mempool beasts.
#0 - naps62
2022-01-13T14:15:48Z
duplicate of #7
🌟 Selected for report: pauliax
1771.4916 USDC - $1,771.49
pauliax
functions claimYield, _withdraw, and _unsponsor should validate that _to is not an empty 0x0 address to prevent accidental burns.
Consider implementing the proposed validation: require _to != address(0)
#0 - dmvt
2022-01-28T20:51:32Z
In this case assets are at risk due to external factors. A zero address check makes sense.
#1 - naps62
2022-02-15T17:52:49Z
pauliax
There are TODOs left in the code. While this does not cause any direct issue, it indicates a bad smell and uncertainty and makes it harder for an auditor to make assumptions.
// TODO: emit the groupId // TODO no invested amount yet // TODO exclude sponsored assets // TODO Make names dynamic
Consider fixing TODOs or removing distracting comments.
#0 - naps62
2022-01-13T19:52:27Z
duplicate of #171 (and probably others)
#1 - dmvt
2022-01-28T15:24:30Z
duplicate of #96
🌟 Selected for report: pauliax
590.4972 USDC - $590.50
pauliax
function sponsor accepts a parameter of timestamp _lockedUntil, but I do not it is very convenient because of the deterministic nature of blockchain. When you send the tx you cannot be sure when it will be mined, so it may result in reverted txs in case _lockedUntil becomes below MIN_SPONSOR_LOCK_DURATION while waiting to be mined in the mempool. I think it would be better to accept the lock period, and then add it to the block.timestamp in the function.
Another problem is that _lockedUntil does not have an upper boundary, so a fat finger error can cost a deposit locked forever.
Consider replacing _lockedUntil with lock period and introducing a reasonable upper limit, e.g. MAX_SPONSOR_LOCK_DURATION.
🌟 Selected for report: pauliax
590.4972 USDC - $590.50
pauliax
If the deposit contains multiple claims, it may introduce a small precision loss. In _createClaim when calculating the percentage it calculates the specific amount for that claim:
uint256 amount = _amount.percOf(_claim.pct);
Due to multiplication / division imperfections, the total amount assigned to all the claims could be slightly smaller than the total amount that is later charged from the user.
The imprecision should be pretty small but if you want to fix this, _createClaim could return the actual amount used, then _createDeposit could sum it up and deposit could transfer this final sum which should be <= _params.amount.
#0 - naps62
2022-02-16T14:13:22Z
This was fixed in https://github.com/sandclock-org/solidity_contracts/pull/11/files