fibonacci - RioLRTIssuer::issueLRT reverts if deposit asset's approve method doesn't return a bool

[sherlock-admin]

fibonacci

medium

RioLRTIssuer::issueLRT reverts if deposit asset's approve method doesn't return a bool

Summary

Using ERC20::approve will not work with ERC20 tokens that do not return a bool.

Vulnerability Detail

The contest's README states that tokens that may not return a bool on ERC20 methods (e.g., USDT) are supposed to be used.

The RioLRTIssuer::issueLRT function makes a sacrificial deposit to prevent inflation attacks. To process the deposit, it calls the ERC20::approve method, which is expected to return a bool value.

Solidity has return data length checks, and if the token implementation does not return a bool value, the transaction will revert.

Impact

Issuing LRT tokens with an initial deposit in an asset that does not return a bool on an approve call will fail.

POC

Add this file to the test folder. Run test with forge test --mc POC --rpc-url=<mainnet-rpc-url> -vv.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.23;

import {Test, console2} from 'forge-std/Test.sol';
import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol';
import {SafeERC20} from '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';

contract POC is Test {
    address constant USDT = 0xdAC17F958D2ee523a2206206994597C13D831ec7;
    address immutable owner = makeAddr("owner");
    address immutable spender = makeAddr("spender");

    function setUp() external {
       deal(USDT, owner, 1e6);
    }

    function testApproveRevert() external {
        vm.prank(owner);
        IERC20(USDT).approve(spender, 1e6);
    }

    function testApproveSuccess() external {
        vm.prank(owner);
        SafeERC20.forceApprove(IERC20(USDT), spender, 1e6);

        uint256 allowance = IERC20(USDT).allowance(owner, spender);
        assertEq(allowance, 1e6);
    }
}

Code Snippet

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTIssuer.sol#L172

Tool used

Manual Review

Recommendation

Use forceApprove from OpenZeppelin's SafeERC20 library.

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits: https://github.com/rio-org/rio-sherlock-audit/pull/5

[mstpr]

Escalate I think this issue is low/informational for the following reasons:

1- There are no loss of funds here. If the USDT is picked then the underlying asset will not be possible to be added to LRT.

2- Current code is specifically for assets that has an EigenLayer strategy which there are no tokens like USDT.

3- Issuer contract is upgradable proxy, if the adding a new asset fails, then the new implementation can be used to add the asset to LRT.

[sherlock-admin2]

Escalate I think this issue is low/informational for the following reasons:

1- There are no loss of funds here. If the USDT is picked then the underlying asset will not be possible to be added to LRT.

2- Current code is specifically for assets that has an EigenLayer strategy which there are no tokens like USDT.

3- Issuer contract is upgradable proxy, if the adding a new asset fails, then the new implementation can be used to add the asset to LRT.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

Disagree with escalation, the argument is moot given it is explicitly stated in contest details that such tokens could be supported

Do you expect to use any of the following tokens with non-standard behaviour with the smart contracts?

• We plan to support tokens with no less than 6 decimals and no more than 18 decimals.
• Tokens may not return a bool on ERC20 methods (e.g. USDT)
• Tokens may have approval race protections (e.g. USDT)

[realfugazzi]

Escalate I think this issue is low/informational for the following reasons:

1- There are no loss of funds here. If the USDT is picked then the underlying asset will not be possible to be added to LRT.

2- Current code is specifically for assets that has an EigenLayer strategy which there are no tokens like USDT.

3- Issuer contract is upgradable proxy, if the adding a new asset fails, then the new implementation can be used to add the asset to LRT.

USDT is mentioned as a supported asset in the documentation, also double checked by the sponsor, see #232

The issue will prevent the initial deposit, enabling inflation attack vectors.

[solimander]

The issue will prevent the initial deposit, enabling inflation attack vectors.

It will not allow USDT to be added, rather than enable inflation attack vectors.

[Czar102]

I disagree with the escalation, the codebase will not be able to support planned functionality without loss of funds, so Medium is appropriate.

[mstpr]

I disagree with the escalation, the codebase will not be able to support planned functionality without loss of funds, so Medium is appropriate.

there won't be any loss of funds tho. It is just simply USDT will not be added, tx will fail.

[solimander]

In which case, the issuer would be upgraded to support USDT.

[realfugazzi]

I disagree with the escalation, the codebase will not be able to support planned functionality without loss of funds, so Medium is appropriate.

there won't be any loss of funds tho. It is just simply USDT will not be added, tx will fail.

right even setting 0 will break the tx, but it falls in the med severity according to the docs as core func is broken

[Czar102]

Result: Medium Has duplicates

As noted above, breaks core functionality, hence Medium is appropriate.

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• mstpr: rejected

[10xhash]

The protocol team fixed this issue in PR/commit rio-org/rio-sherlock-audit#5.

Fixed forceApprove is now used

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.