code-423n4 / 2022-06-nested-findings

0 stars 1 forks source link

QA Report #24

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

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.

Instances:

contracts/Withdrawer.sol:27:        weth.transferFrom(msg.sender, address(this), amount);
contracts/mocks/DummyRouter.sol:29:        IERC20(_inputToken).transferFrom(msg.sender, address(this), _amount);
contracts/mocks/DeflationaryMockERC20.sol:29:        super.transferFrom(sender, recipient, (amount * 5) / 10);
contracts/mocks/AugustusSwapper.sol:21:        proxy.transferFrom(_inputToken, msg.sender, address(this), _amount);

contracts/mocks/DummyRouter.sol:20:        token.transfer(msg.sender, msg.value * 10);
contracts/mocks/DummyRouter.sol:30:        _outputToken.transfer(msg.sender, _amount);
contracts/mocks/AugustusSwapper.sol:22:        IERC20(_outputToken).transfer(msg.sender, _amount);
contracts/mocks/WETHMock.sol:48:        payable(msg.sender).transfer(wad);

Reference:

This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol.

Recommended Mitigation Steps:

Consider using safeTransfer/safeTransferFrom or require() consistently.

Yashiru commented 2 years ago

Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom (Confirmed)

Quality assurance confirmed

JeeberC4 commented 2 years ago

Warden submitted multiple QA Reports. Will not be judged.