sherlock-audit / 2024-02-rubicon-finance-judging

5 stars 3 forks source link

MatricksDeCoder - solmate SafeTransferLib does not check token contract existence #45

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

MatricksDeCoder

medium

solmate SafeTransferLib does not check token contract existence

Summary

Use of Solmate SafeTransferLib may not be safe for token transfers as there may be token addresses input as expected to have code or contract but with no contract leading to either party losing tokens or in transaction

Vulnerability Detail

SafeTransferLib does not check contract existence. It is not uncommon for contracts to use the same address across different chains and users of this system may expect this. However this may not be the case as token could be deployed on chainA, but not on chainB. Parties having interacted with chainA and tokenA address may assume chainB tokenA address has tokenA contract when its empty

SafeTransferLib silently fails leading to no tokens moving from swapper or filler depending.

Impact

A swapper may specify an input token that does not exist but fillers or opposite parties may expect it exists. Swapper will get output tokens without any real transfer of input tokens going to fillers or other side parties . Unfairly gains tokens

A swapper may specify an output token that does not exist resulting in them losing as no real transfer of output token occurs from the fillers or other side of the parties. So user inputs tokens and losses by not getting expected output token whilst other side gains inputs without having to really transfer any output token

2024-02-rubicon-finance-MatricksDeCoder/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol

Code Snippet

function transferInputTokens(
        ResolvedOrder memory order,
        address to
    ) internal virtual;
 /*
 https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L233
 Input token transfers Above will use signature transfers from permit which use Solmate safeTransferFrom which uses
e.g ERC20(permitted.token).safeTransferFrom(owner, transferDetails[i].to, requestedAmount) that does not check if token contract exists
*/

 for (uint256 j = 0; j < outputsLength; j++) {
                    OutputToken memory output = resolvedOrder.outputs[j];
                    output.token.transferFill(output.recipient, output.amount);
                }
/*
https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/lib/CurrencyLibrary.sol#L49
uses -> ERC20(currency).safeTransferFrom(msg.sender, recipient, amount);  

Output token transfers above transferFill in CurrencyLibrary uses Solmate for transfer
e.g ERC20(currency).safeTransferFrom(msg.sender, recipient, amount); that does not check that the output token contract exists so nothing really sent as empty contracts silently fail
*/

Tool used

Manual Review

Recommendation

Consider using OpenZeppelin's Safe

sherlock-admin commented 7 months ago

3 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

Low

PNS commented:

User input validation: User input validation to prevent user mistakes is not considered a valid issue.

0xAadi commented:

Invalid: OOS, custom implementation tokens are OOS