Reality Cards contest - cmichel's results

Handle

cmichel

Vulnerability details

Vulnerability Details

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success. According to the standard the return value must be checked for true, otherwise the transfer will have failed.

The treasury contract calls ERC20.transferFrom/transfer four times without checking for the success return value.

Impact

ERC20-compliant tokens that don't actually perform the transfer and return false are still counted as a correct transfer. Users can "deposit" and increase their balance this way without actually transfering tokens, and then withdraw the treasury.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handles the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

Vulnerability Details

When a user deposits to the treasury they first approve the contract and then call its deposit action which performs an ERC20.transferFrom. It's possible for an attacker to frontrun the final deposit transaction after the user approval and turn the deposit into a sponsor this way, which is basically a free donation to a market outcome (card). The attacker can create their own market where they are the only participant betting on a card that is guaranteed to win.

Attack:

1. Attacker creates custom market M with a guaranteed winning card. (They can time the market close in a way such that no other user can rent the card longer then them.)
2. user A sends a transaction which approve the treasury for a deposit amount.
3. user A sends a Treasury.deposit(amount) action instead.
4. An attacker observes the deposit from 2 and frontruns it with a sponsor action from their custom market by sending a M.sponsor(A, amount) transaction.
5. This increases the payout of the card in the attacker market but takes the funds from the user in the treasury.sponsor(_sponsorAddress=A, _amount) treasury call: erc20.transferFrom(_sponsor=A, address(this), _amount);
6. The attacker wins, and can withdraw their profit at some point, essentially stealing the user's deposit

Impact

User deposits can be stolen.

Recommended Mitigation Steps

The sponsor function should not allow specifying a custom sponsorAddress. The market should pull in the amounts from msg.sender using ERC20.transferFrom(msg.sender, amount) and then forward this amount to the treasury using a simple ERC20.transfer(treasury, amount) followed by the treasury.sponsor call that does the remaining book-keeping.

Handle

cmichel

Vulnerability details

Vulnerability Details

The RCNftHubL2.safeTransferFrom function does not correctly implement the ERC721 spec:

When using safeTransferFrom, the token contract checks to see that the receiver is an IERC721Receiver, which implies that it knows how to handle ERC721 tokens. ERC721

This check is not implemented, it just drops the _data argument.

Impact

Contracts that don't know how to handle ERC721 tokens (are not an IERC721Receiver) can accept them but they should not when using safeTransferFrom according to spec.

Recommended Mitigation Steps

Implement the IERC721Receiver check in safeTransferFrom.

Handle

cmichel

Vulnerability details

Vulnerability Details

The treasury only checks its globalPause field but does not check its market-specific marketPaused field for Treasury.sponsor. A paused market contract can therefore still deposit as a sponsor using Market.sponsor

Impact

The market-specific pause does not work correctly.

Recommended Mitigation Steps

Add checks for marketPaused in the Treasury for sponsor.

Handle

cmichel

Vulnerability details

Vulnerability Details

There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().

Impact

The deposit() function will introduce unexpected balance inconsistencies when comparing internal asset records with external ERC20 token contracts.

Recommended Mitigation Steps

One possible mitigation is to measure the asset change right before and after the asset-transferring routines

Handle

cmichel

Vulnerability details

Vulnerability Details

The treasury implements a max contract balance check in the deposit function:

require(
    (erc20.balanceOf(address(this)) + _amount) <= maxContractBalance,
    "Limit hit"
);

A whale can stop anyone from depositing by front-running a user's deposit with a deposit that pushes the contract balance to the maxContractBalance limit first. The user's deposit will then fail in the check. Afterwards, the whale can withdraw again.

This is not only restricted to whales, miners/users can do the same using same-block cross-transaction flashloans and submitting a (attacker deposit, user deposit, attacker withdraw) flashbundle to a miner. Possibilities like this will only become more prevalent in the future.

Impact

Any users can be blocked from depositing which prevents them from renting cards. This allows an attacker to manipulate the outcome of a market in their favor by strategically preventing other competitors to bid on their cards (causing forfeiture due to a low deposit balance).

Recommended Mitigation Steps

Remove the contract limit or at least set the limit very high if it keeps happening.