Platform: Code4rena
Start Date: 10/06/2021
Pot Size: $45,000 USDC
Total HM: 21
Participants: 12
Period: 7 days
Judge: LSDan
Total Solo HM: 13
Id: 13
League: ETH
Rank: 6/12
Findings: 7
Award: $2,504.21
🌟 Selected for report: 3
🚀 Solo Findings: 0
pauliax
When transfering erc20 tokens, functions transfer and transferFrom are used. These functions return boolean to indicate if the action was successful, however, none of the usages check the returned value: erc20.transferFrom(msgSender(), address(this), _amount); erc20.transfer(_msgSender, _amount); This erc20 token may only be set by the admin upon initialization or function setTokenAddress, so we can assume it will be ensured that it behaves as expected, however, I wanted you to be aware of this potential issue and consider using SafeERC20: https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#SafeERC20
Check that these functions return true or implement SafeERC20 pattern.
#0 - Splidge
2021-06-15T08:51:43Z
Duplicate of #2
#1 - dmvt
2021-07-11T12:37:44Z
duplicate of #2
791.0609 USDC - $791.06
pauliax
This function sponsor should only be called by the factory, however, it does not have any auth checks, so that means anyone can call it with an arbitrary _sponsorAddress address and transfer tokens from them if the allowance is > 0: /// @notice ability to add liqudity to the pot without being able to win. /// @dev called by Factory during market creation /// @param _sponsorAddress the msgSender of createMarket in the Factory function sponsor(address _sponsorAddress, uint256 _amount) external override { _sponsor(_sponsorAddress, _amount); }
Check that the sender is a factory contract.
#0 - Splidge
2021-06-15T08:50:27Z
This is a good one!
#1 - mcplums
2021-06-18T13:29:36Z
Yeah this is massive one!! Thanks @pauliax :)
#2 - Splidge
2021-06-21T10:16:49Z
fixed here
pauliax
I expect every function in the Treasury that interacts with erc20 tokens to have this balancedBooks modifier to make sure that "funds haven't gone missing during this function call". To make sure that tokens were indeed transferred and marketBalance updated, this modifier can be used in function topupMarketBalance.
Add 'balancedBooks' to the declaration of function topupMarketBalance.
#0 - Splidge
2021-06-18T12:34:01Z
Duplicate of #23
#1 - dmvt
2021-07-11T10:56:20Z
duplicate of #112
🌟 Selected for report: gpersoon
Also found by: maplesyrup, pauliax
pauliax
Market initializes minRentalDayDivisor from the treasury but this variable can be later changed in treasury (function setMinRental). Here I am not sure if the market still needs to use old cached version of minRentalDayDivisor as all the interactions with the treasury will operate on the new value while market still uses the old: minRentalDayDivisor = treasury.minRentalDayDivisor(); An example where I see a potential problem: require( treasury.userDeposit(_user) >= _userTotalBidRate / minRentalDayDivisor, "Insufficient deposit" ); Here, the market fetches userDeposit from the treasury but uses cached minRentalDayDivisor to check the sufficient balance. If minRentalDayDivisor is updated in the treasury, this check may become ineffective.
Do not use a cached version of minRentalDayDivisor in the Market contract, always fetch the latest value from the Factory contract.
#0 - Splidge
2021-06-17T13:39:05Z
Duplicate of #31
#1 - dmvt
2021-07-11T10:57:57Z
duplicate of #31
pauliax
There is no check that _timestamps[1] (marketLockingTime) >= _timestamps[0] (marketOpeningTime). I think that should be enforced just in case to prevent market locking before opening.
Add in Factory createMarket: require( _timestamps[1] >= _timestamps[0], "Market locks too early" );
#0 - Splidge
2021-06-17T14:45:20Z
Duplicate of #61
#1 - dmvt
2021-07-10T13:43:09Z
duplicate of #61
79.1061 USDC - $79.11
pauliax
Here I think the check should be inclusive ('>=', not '>') to indicate the sufficient deposit balance: // this deposit could cancel the users foreclosure if ( (user[_user].deposit + _amount) > (user[_user].bidRate / minRentalDayDivisor) ) { isForeclosed[_user] = false; emit LogUserForeclosed(_user, false); }
if ( (user[_user].deposit + _amount) >= (user[_user].bidRate / minRentalDayDivisor) ) { ... }
#0 - Splidge
2021-06-16T12:47:47Z
Duplicate of #26
#1 - dmvt
2021-07-11T10:47:00Z
duplicate of #108
🌟 Selected for report: pauliax
434.0526 USDC - $434.05
pauliax
I can't find a reason why totalNftMintCount in Factory can't be replaced with ERC721 totalSupply() to make it less error-prone. As nfthub.mint issues a new token it should automatically increment totalSupply and this assignment won't be needed: totalNftMintCount = totalNftMintCount + _tokenURIs.length; Also in function setNftHubAddress you need to manually set _newNftMintCount if you want to change nfthub so an invalid value may crash the system. totalSupply() will eliminate totalNftMintCount and make the system more robust.
Replace totalNftMintCount with nfthub totalSupply() in Factory contract.
#0 - mcplums
2021-06-17T07:00:23Z
This is a good one, would indeed make it less error prone
#1 - Splidge
2021-06-21T14:42:56Z
Sometimes what looks like a small fix just takes you all the way down the rabbithole, this was one of them. totalSupply() isn't included by default so I had to import the Enumerable extension. fixed here
pauliax
_incrementState checks state boundaries before the increment, so in theory, it is possible to increment it to an invalid state (number 4). In my opinion, it should assert that after setting the new state. In practice, _incrementState is never called when the state is in WITHDRAW thus it cannot be exploited with the current code but it still needs to assume such a scenario in case you decide to change the code in the future.
Move to the bottom of the _incrementState: assert(uint256(state) < 4);
#0 - Splidge
2021-06-15T08:48:14Z
Duplicate of #9 I'm unsure of which severity they should be though. As this report states, currently this isn't an issue so would be severity 0, although it also states future code could make the problem worse which would elevate it to 1 and maybe higher depending on the changes. I'd be inclined to stick with severity 0 for now.
#1 - Splidge
2021-06-15T09:00:44Z
Duplicate of #9
#2 - dmvt
2021-07-09T15:44:48Z
I don't think we can use "the code might change in a way some time which causes this to be slightly severe" as a reasonable severity metric. There is definite legibility and gas savings available here, so I'm going to match the G severity of #9
#3 - dmvt
2021-07-09T15:45:37Z
duplicate of #9
pauliax
Could break inside if as there is no point in iterating further once cardAffiliateCut is set to 0: // check the validity of card affiliate array. // if not valid, reduce payout to zero if (_cardAffiliateAddresses.length == _numberOfCards) { for (uint256 i = 0; i < _numberOfCards; i++) { if (_cardAffiliateAddresses[i] == address(0)) { cardAffiliateCut = 0; } } } else { cardAffiliateCut = 0; }
add 'break' inside inner if statement.
#0 - Splidge
2021-06-15T08:54:16Z
Duplicate of #17
🌟 Selected for report: pauliax
0 USDC - $0.00
pauliax
_amount -= (_amount - marketBalance); is basically the same as: _amount = marketBalance;
#0 - Splidge
2021-06-21T14:41:09Z
fixed here