Reality Cards contest - shw's results

Handle

shw

Vulnerability details

Impact

In the contract RCTreasury, the return values of ERC20 transfer and transferFrom are not checked, which could be false if the transferred token is not ERC20-compliant. In that case, the transfer fails without being noticed by the calling contract.

Proof of Concept

Referenced code: RCTreasury.sol#L350 RCTreasury.sol#L298 RCTreasury.sol#L373 RCTreasury.sol#L478

Recommended Mitigation Steps

Use the SafeERC20 library implementation from Openzeppelin and call safeTransfer or safeTransferFrom when transferring ERC20 tokens.

Handle

shw

Vulnerability details

Impact

The function sponsor(address _sponsorAddress, uint256 _amount) in RCMarket is permissionless, which allows a malicious actor to sponsor any market in the name of some sponsor as long as enough tokens are approved. The malicious actor could then add sponsorship to markets where he will likely become the winner to gain more winning awards.

Proof of Concept

1. Suppose there are two opening markets, MarketA and MarketB. A sponsor, Alice, wants to sponsor MarketA with 100 amounts of DAI. So, after approving the treasury to send her tokens, she calls sponsor(100) on MarketA.
2. A malicious actor, Bob, listens to the mempool and notices Alice's transaction. He then front-runs the transaction and calls sponsor(Alice, 100) on MarketB.
3. Bob's transaction is executed, and the treasury transfers 100 DAI from Alice to MarketB, which is not Alice's intention.

Besides front-running, Bob can also check the past transactions to find anyone who approved the treasury before. If the current allowance is greater than zero, he can call sponsor directly with the victim's address as the parameter to sponsor any market he wants.

Referenced code: RCMarket.sol#L817-L822

Recommended Mitigation Steps

Should check if the caller of the function sponsor(address _sponsorAddress, uint256 _amount) is the factory.

Handle

shw

Vulnerability details

Impact

In the function createMarket of RCFactory, the market opening time is not checked before the market locking time, and thus markets can be created with invalid time configurations accidentally. Besides, if the advancedWarning is non-zero, there are two checks applied, and the first seems unnecessary (which is a requirement of the second one).

Proof of Concept

Referenced code: RCFactory.sol#L520-L527

Recommended Mitigation Steps

Check that the market opening time is before both the market locking time. In cases where advancedWarning is non-zero, if the purpose of the first check is to prevent subtraction overflow on the second check, consider removing the first one and re-writing the second one as _timestamps[0] > block.timestamp + advancedWarning.

Handle

shw

Vulnerability details

Impact

The function foreclosureTimeUser of RCTreasury underestimates the user's foreclosure time if the current time is not the user's last rent calculation time. The underestimation of the foreclosure time could cause wrong results when determining the new owner of the card.

Proof of Concept

The variable timeLeftOfDeposit at line 668 is calculated based on depositAbleToWithdraw(_user), the user's deposit minus the rent from the last rent calculation to the current time. Thus, the variable timeLeftOfDeposit indicates the time left of deposit, starting from now. However, at line 672, the foreclosureTimeWithoutNewCard is calculated by timeLeftOfDeposit plus the user's last rent calculation time instead of the current time. As a result, the user's foreclosure time is reduced. From another perspective, the rent between the last rent calculation time and the current time is counted twice.

Referenced code: RCTreasury.sol#L642-L653 RCTreasury.sol#L669 RCTreasury.sol#L672 RCTreasury.sol#L678 RCOrderbook.sol#L553-L557

Recommended Mitigation Steps

Change depositAbleToWithdraw(_user) at line 669 to user[_user].deposit. Or, change user[_user].lastRentCalc at both line 672 and 678 to block.timestamp.