search

code-423n4 / 2023-06-reserve-findings

1 stars 0 forks source link

RToken.redeem should claim rewards before sending tokens to user #32

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RToken.sol#L156-L158

Vulnerability details

Impact

RToken.redeem should claim rewards before sending tokens to user. Because after that he will not receive them anymore.

Proof of Concept

When user mints RToken, then he sends some tokens as collateral and they are stored by BackingManager. Some of collateral tokens can earn rewards and that's why BackingManager is Trading and has functions to claim that rewards.

When user redeems their RTokens, that means that BackingManager will retun part of collateral that it's currently storing. Once it will do that, then BackingManager will not be able to claim rewards for that part of tokens anymore. And these rewards will be lost. And they can be even not small.

Tools Used

VsCode

Recommended Mitigation Steps

I believe that rewards should be claimed for all tokens, that are going to be sent to redeemers. These rewards should be used in order to back RToken.

Assessed type

Error

c4-sponsor commented 1 year ago

tbrent marked the issue as sponsor disputed

tbrent commented 1 year ago

BackingManager will not be able to claim rewards for that part of tokens anymore. And these rewards will be lost. And they can be even not small.

Not true. As long as the ERC20 remains registered in the AssetRegistry, then BackingManager.claimRewards() would claim all rewards, even if the ERC20 balances that earned those rewards are now gone.

rvierdiiev commented 1 year ago

I meant that he will not be able to claim rewards based on part of tokens that user has redeemed. Of course you still can claim for another tokens that BackingManager controls.

tbrent commented 1 year ago

The protocol does not aim to treat holders on an individual-basis. Outcomes are at the group level across the board. You can examine the StRSR contract and how it handles the RSR drip; or the Furnace contract and how it handles melting. This is simply not a goal of the protocol.

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Invalid