code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

Checks-Effects-Interactions (CEI) pattern not followed in calls to safeTransfer/safeTransferFrom, after which contract state is updated. While liquidityAsset/collateralAsset may be trusted tokens for now, this may become an issue in future if scope is expanded to include user-supplied (i.e. untrusted) ERC20 tokens as assets. In such cases, the safeTransfer/safeTransferFrom implementations may be malicious and cause issues via reentrancy.

Proof of Concept

https://github.com/maple-labs/maple-core/blob/355141befa89c7623150a83b7d56a5f5820819e9/contracts/Loan.sol#L203 https://github.com/maple-labs/maple-core/blob/355141befa89c7623150a83b7d56a5f5820819e9/contracts/Loan.sol#L286 https://github.com/maple-labs/maple-core/blob/355141befa89c7623150a83b7d56a5f5820819e9/contracts/Loan.sol#L318 https://github.com/maple-labs/maple-core/blob/355141befa89c7623150a83b7d56a5f5820819e9/contracts/Loan.sol#L365

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider CEI patterns or use ReentrancyGuards from OpenZeppelin.

lucas-manuel commented 3 years ago

All assets used across the protocol will be evaluated and whitelisted, these checks will not be necessary.

Arachnid commented 3 years ago

If arbitrary tokens are allowed, many other attacks than reentrancy are possible - such as tokens that misreport balances or allow admins to confiscate tokens. Allowing untrusted tokens would be a large enough change that I think a reassessment of the whole platform would be required.