search

salvorio / salvor-contracts

1 stars 0 forks source link

[SLG-02M] Inexistent Conformance to Checks-Effects-Interactions Pattern #10

Closed HKskn closed 3 months ago

HKskn commented 8 months ago

SLG-02M: Inexistent Conformance to Checks-Effects-Interactions Pattern

Type Severity Location
Language Specific SalvorLending.sol:L403, L405, L420, L422

Description:

The referenced code statements do not adhere to the Checks-Effects-Interactions (CEI) pattern, permitting a re-entrancy to exploit an interim corrupted system state.

Impact:

While the codebase is presently not susceptible to a re-entrancy attack as guards are imposed throughout all sensitive functions of it, the CEI pattern is still advisable to apply so as to prevent future misbehaviours in the code.

Example:

/**
* @notice Repays the loan for a specific NFT and returns the NFT to the borrower. This function is internal.
* @param nftContractAddress The address of the NFT contract.
* @param _tokenId The ID of the token (NFT) for which the loan is being repaid.
*/
function repay(address nftContractAddress, uint256 _tokenId) internal {
    address borrower = items[nftContractAddress][_tokenId].borrower;
    address lender = items[nftContractAddress][_tokenId].lender;
    require(borrower == msg.sender, "msg.sender is not borrower");

    uint256 payment = _calculateRepayment(items[nftContractAddress][_tokenId].amount, items[nftContractAddress][_tokenId].rate);

    emit Repay(nftContractAddress, _tokenId, payment);

    IAssetManager(assetManager).transferFrom(borrower, lender, payment);

    IAssetManager(assetManager).nftTransferFrom(address(this), borrower, nftContractAddress, _tokenId);

    delete items[nftContractAddress][_tokenId];
}

Recommendation:

We advise them to be re-ordered, ensuring that the items loan entry that corresponds to the transferred NFT is cleared prior to it being released to the borrower / lender.

HKskn commented 8 months ago

Fixed