Re-entrerable code in NFTVault.repurchase

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L879

Vulnerability details

This attack requires stable to relinquish control during tranferFrom() which under the current StableCoin it does not. Although it’s not exploitable currently, it is a highly risky code pattern that should be avoided.

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L879

Proof Of Concept

This vulnerability would allow liquidator to steal all of the callers tokens. If an attacker was able to get control flow during the StableCoin.transferFrom function, he would be able to re-enter NFTVault.repurchase and cause the re exeution of stablecoin.transferFrom because all of the requirements would be satisfied as no variables were updated. Thus, the attacker would be able to cause as many executions of stablecoin.transferFrom as he wants.

    stablecoin.transferFrom(
        msg.sender,
        position.liquidator,
        debtAmount + penalty
    );

    positionOwner[_nftIndex] = address(0);
    delete positions[_nftIndex];
    positionIndexes.remove(_nftIndex);

Tools Used

VScode

Recommended Mitigation Steps

Add the nonReentrant modifier to NFTVault.repurchase to avoid re-entrance, similar to the other functions such as liquidate

Another option would be to follow checks-effects-interactions pattern, however this solution would not be consistant with the other reenterable functions in the contract which are using the nonReentrant guard.

[spaghettieth]

Attackers can't gain control of the execution flow during a call to stablecoin.transferFrom

[dmvt]

Code for the token in question is written by sponsor. This attack is not possible.