Reality Cards contest - shw's results

This is a duplicate of an issue found in the last contest.

It was not solved last time because we didn't want to risk introducing problems by completely rewriting our own version of the MetaTx contract (the version we are using is from a trusted source and working in many production environments that have been audited), so instead of writing our own version I attempted to use the OpenZeppelin version. We need to use the upgradable versions of the OpenZeppelin contracts because we clone RCMarket.sol, however there was an incompatibility between ERC2771Context.sol which uses an immutable variable in the constructor and AccessControl.sol which calls _msgSender() in the constructor. Details on this can be found in this thread..

As you can see in that thread the incompatibility has apparently been fixed, however this was done only 5 hours before the start of this contest (and after we had submitted the code for the contest), so it wasn't practical to implement this solution for this contest. Regardless as you say this attack is "not possible since each user's nonce is used only once".

I'll mark this as 'disputed' for now, if the judges decide that we should have re-written our own version (or discovered time-travel and implemented the OpenZeppelin version that's now availiable) then I'd be happy to change it to 'acknowledged' as we will not be making changes to such a sensitive area at this stage in the project.

Regardless of the solution and the mitigation steps the team is willing or not willing to take, I think highlighting the malleability issue is a valid 1 (Low Risk) issue that may not be worth fixing based on other risks as mentioned by @Splidge

I do not think using safeApprove would help here. Given the premise that the ERC20 in use is non-compliant and the approval fails, does it matter if the contracts are made aware of this or not? In either case transfers to the bridge contract would not be possible. The solution therefore isn't a different approval but using a different ERC20 which is compliant. Using a different ERC20 isn't necessary as the one we have chosen, USDC, is compliant.

safeIncreaseAllowance wouldn't make any difference either, we are approving the transfer of more tokens than exist, it'd be pretty impressive to see a multiple withdrawal attack manage to withdraw enough tokens to warrant using the additional approval. A rough calculation shows that if you moved the entire USDC supply on Polygon in a single transaction (assuming the lower end gas 50k) it'd still take 2.89*10^23 blocks filled with transfers before you get to the limit of a type(uin256).max approval, even if we sped Polygon up to 10 blocks/second that'd still take 67,000 x the age of the universe to process 🤯, but certainly after that we wouldn't want anybody using the extra approval.

Agree with warden that it would be better to use safeApprove and revert the setBridge call with a reasonable error message than fail silently. The safeIncreaseAllowance doesn't need to be implemented based on the use case. Using safeERC20 calls in a contract that are imports the library seems like a no brainer even if the expected use case doesn't require it. These contracts will potentially persist much longer past everyone remembering all the nuances of which ERC20 tokens work and which might not.

Understood, changing to Acknowledged as for this use case there is no impact and any further changes will delay other audits and our time to launch.

This transfer function is just for the market to move the NFTs while the market is operational, during this phase it doesn't matter if the NFT is given to a non-compliant contract as the market is in control of moving the NFT and will simply give it to the next user that makes a rental. Using _safeTransfer would be dangerous as it would give an attacker the opportunity to take part in a market via a contract that is not ERC721 compliant, when the market attempts to pass ownership to this contract the transfer would revert and the market would become locked.

claimCard in RCMarket.sol also calls the transfer method to claim the card, which could result in the situation outlined by the warden.

Ahh sorry, I missed the key part in the wardens impact "when transferring the card to the final winner".

I still think this isn't an issue as if the destination is a non-compliant contract, when using _safeTransfer the claimCard call would revert. The NFT is permanently stuck on either the market or on the non-compliant contract. At least the person who wrote the contract, funded it, used it to place bids, win the NFT and make the claimCard call would still get it transferred to their contract to use as a trophy cabinet of sorts. Maybe that person didn't implement onERC721Received? This check would be too little too late.

Maybe we could check when the first rental is made that the sender is eligible? Although that would deviate slightly from the standard as onERC721Received is supposed to be called after the transfer.