StRSR.leakyRefresh implementation is wrong

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/StRSR.sol#L665-L683

Vulnerability details

Impact

StRSR.leakyRefresh implementation is wrong. Asset registry refresh will not work as planned.

Proof of Concept

StRSR.leakyRefresh function should check how much percents of totalRSR was withdrawn, since last assetsRegistry.refresh call. This is needed to save gas for users.

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/StRSR.sol#L665-L683

    function leakyRefresh(uint256 rsrWithdrawal) private {
        uint48 lastRefresh = assetRegistry.lastRefresh(); // {s}

        // Assumption: rsrWithdrawal has already been taken out of draftRSR
        uint256 totalRSR = stakeRSR + draftRSR + rsrWithdrawal; // {qRSR}
        uint192 withdrawal = _safeWrap((rsrWithdrawal * FIX_ONE + totalRSR - 1) / totalRSR); // {1}

        // == Effects ==
        leaked = lastWithdrawRefresh != lastRefresh ? withdrawal : leaked + withdrawal;
        lastWithdrawRefresh = lastRefresh;

        if (leaked > withdrawalLeak) {
            leaked = 0;
            lastWithdrawRefresh = uint48(block.timestamp);

            /// == Refresh ==
            assetRegistry.refresh();
        }
    }

In case if lastWithdrawRefresh was done in same time as assetRegistry.lastRefresh, then percentage is calculated and added to the counter. In case, if this counter is more than withdrawalLeak, which is not more than 30%, then assetRegistry.refresh is called.

The problem here is how withdrawal is calculated. uint192 withdrawal = _safeWrap((rsrWithdrawal * FIX_ONE + totalRSR - 1) / totalRSR) totalRSR is all rsr at the moment of withdraw, so on next call totalRSR will be smaller than on previous(or even can be bigger if new stakers minted stRSR).

That means that this withdrawal calculation will not be able to get correct ratio. Example. 1.totalRSR is 100 and user withdraws 10, which means that withdrawal == 10%. this value is stored to leaked. 2.another user withdraws 10, which is 11% this time. leaked is 21 now, which is incorrect. It should be only 20%.

Tools Used

VsCode

Recommended Mitigation Steps

I don't see good solution here, because totalRSR is not fixed, it can be bigger or less than previous one, because of withdraws/deposits. However current calculation is wrong.

Assessed type

Error

[0xean]

This is needed to save gas for users.

Can you clarify why this is not a gas optimization?

[c4-sponsor]

tbrent marked the issue as disagree with severity

[c4-sponsor]

tbrent marked the issue as sponsor acknowledged

[c4-judge]

0xean changed the severity to QA (Quality Assurance)