Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/contracts/p1/BackingManager.sol#L69-L75 https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/contracts/p1/AssetRegistry.sol#L207

Vulnerability details

Proof of Concept

First note this from the readme https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/README.md#L142-L145

### ERC20 token behaviors in scope

| Question | Answer |
| -------- | ------ |

| [Revert on large approvals and/or transfers](https://github.com/d-xo/weird-erc20?tab=readme-ov-file#revert-on-large-approvals--transfers) | In scope |

Now take a look at https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/contracts/libraries/Allowance.sol#L17-L46

function safeApproveFallbackToMax(
  address tokenAddress,
  address spender,
  uint256 value
) internal {
  IERC20ApproveOnly token = IERC20ApproveOnly(tokenAddress);

  // 1. Set initial allowance to 0
  token.approve(spender, 0);
  // untestable:
  //    allowance should always be 0 if token behaves correctly
  require(token.allowance(address(this), spender) == 0, 'allowance not 0');

  if (value == 0) return;

  // 2. Try to set the provided allowance
  bool success; // bool success = false;
  try token.approve(spender, value) {
    success = token.allowance(address(this), spender) == value;
    // solhint-disable-next-line no-empty-blocks
  } catch {}

  // 3. Fall-back to setting a maximum allowance
  if (!success) {
    token.approve(spender, type(uint256).max);
    // untestable:
    //    allowance should always be max value if token behaves correctly
    require(token.allowance(address(this), spender) >= value, 'allowance missing');
  }
}

This is a helper function that's used accross the proocol and it's used to cover all the edge cases for the types of tokens the protocol supports, issue however is that the fallback logic would not work for some of the supported tokens, which is because it attempts approve type(uint256).max) on these tokens, but per the readMe, we can see that some of the tokens that are to be supported would revert on these approvals, which would make this attempt incompatible with them.

In the same light, take a look at https://github.com/code-423n4/2024-07-reserve/blob/825e5a98a8c94a7b4458b3b359a507edb9e662ba/contracts/p1/BackingManager.sol#L69-L75

function grantRTokenAllowance(IERC20 erc20) external notFrozen {
  require(assetRegistry.isRegistered(erc20), 'erc20 unregistered');
  // == Interaction ==
  IERC20(address(erc20)).safeApprove(address(rToken), 0);
  IERC20(address(erc20)).safeApprove(address(rToken), type(uint256).max);
}

The above would also revert on these set of tokens.

Impact

A functionality of the protocol and it's availbility would be impacted, cause as hinted this fallback attempt to approving the token would always revert and to exarcebate the issue the BackingManager#grantRTokenAllowance() would also now fail on these tokens, which would mean that registering these tokens would now fail cause of the revert that occurs here:

    function _registerIgnoringCollisions(IAsset asset) private returns (bool swapped) {
..snip

        if (!main.frozen()) {
            backingManager.grantRTokenAllowance(erc20);
        }
..snip
    }

Which is directly queried during registrations here.

Recommended Mitigation Steps

Consider not supporting these tokens at all.

Assessed type

ERC20

code-423n4 / 2024-07-reserve-validation