code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L359

Vulnerability details

Unchecked Return Value for IERC20.transfer & IERC20.transferFrom call

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:

File: Swivel.sol- Link

Swivel/Swivel.sol:359:    Safe.transfer(uToken, o.maker, a - premiumFilled);
Swivel/Swivel.sol:363:    Safe.transfer(uToken, msg.sender, premiumFilled - fee);
Swivel/Swivel.sol:395:    Safe.transfer(uToken, msg.sender, principalFilled - a - fee);
Swivel/Swivel.sol:396:    Safe.transfer(uToken, o.maker, a);
Swivel/Swivel.sol:467:    Safe.transfer(token, admin, token.balanceOf(address(this)));
Swivel/Swivel.sol:609:    Safe.transfer(IErc20(u), msg.sender, a);
Swivel/Swivel.sol:624:    Safe.transfer(IErc20(u), t, a);
Swivel/Swivel.sol:642:    Safe.transfer(IErc20(u), msg.sender, redeemed);
Swivel/Swivel.sol:661:    Safe.transfer(IErc20(u), msg.sender, redeemed);

Swivel/Swivel.sol:125:    Safe.transferFrom(uToken, msg.sender, o.maker, a);
Swivel/Swivel.sol:128:    Safe.transferFrom(uToken, o.maker, address(this), principalFilled);
Swivel/Swivel.sol:163:    Safe.transferFrom(uToken, o.maker, msg.sender, premiumFilled);
Swivel/Swivel.sol:167:    Safe.transferFrom(uToken, msg.sender, address(this), (a + fee));
Swivel/Swivel.sol:199:    Safe.transferFrom(uToken, msg.sender, o.maker, a - premiumFilled);
Swivel/Swivel.sol:202:    Safe.transferFrom(uToken, msg.sender, address(this), fee);
Swivel/Swivel.sol:224:    Safe.transferFrom(IErc20(o.underlying), msg.sender, o.maker, a);
Swivel/Swivel.sol:293:    Safe.transferFrom(uToken, o.maker, msg.sender, principalFilled - a);
Swivel/Swivel.sol:298:    Safe.transferFrom(uToken, msg.sender, address(this), fee);
Swivel/Swivel.sol:324:    Safe.transferFrom(uToken, o.maker, msg.sender, premiumFilled);
Swivel/Swivel.sol:328:    Safe.transferFrom(uToken, msg.sender, address(this), fee);
Swivel/Swivel.sol:580:    Safe.transferFrom(uToken, msg.sender, address(this), a);

Reference:

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

Recommended Mitigation Steps:

Consider using safeTransfer/safeTransferFrom or require() consistently.

JTraversa commented 2 years ago

We are using safetransfer.

bghughes commented 2 years ago