sherlock-audit / 2023-12-arcadia-judging

19 stars 15 forks source link

0x52 - `LendingPool#flashAction` is broken when trying to refinance position across `LendingPools` due to improper access control #145

Open sherlock-admin2 opened 9 months ago

sherlock-admin2 commented 9 months ago

0x52

medium

LendingPool#flashAction is broken when trying to refinance position across LendingPools due to improper access control

Summary

When refinancing an account, LendingPool#flashAction is used to facilitate the transfer. However due to access restrictions on updateActionTimestampByCreditor, the call made from the new creditor will revert, blocking any account transfers. This completely breaks refinancing across lenders which is a core functionality of the protocol.

Vulnerability Detail

LendingPool.sol#L564-L579

IAccount(account).updateActionTimestampByCreditor();

asset.safeTransfer(actionTarget, amountBorrowed);

{
    uint256 accountVersion = IAccount(account).flashActionByCreditor(actionTarget, actionData);
    if (!isValidVersion[accountVersion]) revert LendingPoolErrors.InvalidVersion();
}

We see above that account#updateActionTimestampByCreditor is called before flashActionByCreditor.

AccountV1.sol#L671

function updateActionTimestampByCreditor() external onlyCreditor updateActionTimestamp { }

When we look at this function, it can only be called by the current creditor. When refinancing a position, this function is actually called by the pending creditor since the flashaction should originate from there. This will cause the call to revert, making it impossible to refinance across lendingPools.

Impact

Refinancing is impossible

Code Snippet

LendingPool.sol#L529-L586

Tool used

Manual Review

Recommendation

Account#updateActionTimestampByCreditor() should be callable by BOTH the current and pending creditor

sherlock-admin2 commented 9 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

sherlock-admin commented 9 months ago

The protocol team fixed this issue in PR/commit https://github.com/arcadia-finance/lending-v2/pull/133.

Thomas-Smets commented 9 months ago

Fix consists out of two PR's:

IAm0x52 commented 8 months ago

Fix looks good. Removes the updateActionTimestampByCreditor call and instead uses a callback to enforce nonreentrant and prevent ERC777s from reentering

sherlock-admin4 commented 8 months ago

The Lead Senior Watson signed off on the fix.