code-423n4 / 2021-12-yetifinance-findings

0 stars 0 forks source link

`_redeemCaller` should not obtain rights to future rewards for the `WJLP` they redeemed #291

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-yetifinance/blob/5f5bf61209b722ba568623d8446111b1ea5cb61c/packages/contracts/contracts/TroveManagerRedemptions.sol#L310-L312

if (whitelist.isWrapped(colls.tokens[i])) {
    IWAsset(colls.tokens[i]).updateReward(_borrower, _redeemCaller, tokenAmountToRedeem);
}

Per the comment:

Later the asset will be transferred directly out, so no new reward is needed to be kept track of.

However, the current implementation will still grant rights to future rewards to the _redeemCaller, which we believe is a mistake.

As a result, this can cause the _redeemCaller to be able to call WJLP.claimReward() and claim more rewards than expected.

Recommendation

Change to:

if (whitelist.isWrapped(colls.tokens[i])) {
    IWAsset(colls.tokens[i]).updateReward(_borrower, address(0), tokenAmountToRedeem);
}
kingyetifinance commented 2 years ago

@LilYeti : These are equivalent in the code, since before the evaluation of this function completes, sendCollateralsUnwrap is called on the redeemed collateral.

https://github.com/code-423n4/2021-12-yetifinance/blob/main/packages/contracts/contracts/TroveManagerRedemptions.sol#L247

Therefore the rewards for the redeemer are ended, before they would be able to gain any rewards. Also, the existing rewards do not transfer to the new owner anyway, only new rewards from that time on do. So, this would not be a problem anyway.

kingyetifinance commented 2 years ago

Essentially, They will receive the unwrapped version anyway

alcueca commented 2 years ago

Downgrading to a code clarity issue.