search

code-423n4 / 2021-11-unlock-findings

0 stars 0 forks source link

Use safeTransfer consistently instead of transfer #109

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

Jujic

Vulnerability details

Impact

It is good to add a require() statement that checks the return value of token transfers, or to use something like OpenZeppelin’s safeTransfer unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Proof of Concept

Reference this similar medium-severity finding from Consensys Diligence Audit of Fei Protocol. https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call https://github.com/unlock-protocol/unlock/blob/dda84f298e51ea37af514133e861052f21164b37/smart-contracts/contracts/Unlock.sol#L354-L355

Tools Used

Recommended Mitigation Steps

Recommend using safeTransfer or require() consistently.

0xleastwood commented 2 years ago

The affected areas of code do not operate on any ERC20 token. UDT is a ERC20 compliant token, so there is no concern for unexpected behaviour. Marking this as non-critical as it is best practices.