search

code-423n4 / 2021-10-ambire-findings

0 stars 0 forks source link

Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom #20

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

defsec

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/safeTransferFrom 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.

While most places use a require or safeTransfer/safeTransferFrom, there are three missing cases in the withdrawal of staking token and rescue of arbitrary tokens sent to the Manager contract.

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

Proof of Concept

  1. Navigate to https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/wallet/Zapper.sol#L228 contract.
  2. Transfer function is used instead of safe transfer.

Tools Used

Manual Code Review

Recommended Mitigation Steps

Consider using safeTransfer/safeTransferFrom or require() consistently.

Ivshti commented 2 years ago

duplicate of #9 but also a nonissue

GalloDaSballo commented 2 years ago

Duplicate of #35