search

code-423n4 / 2022-07-swivel-findings

0 stars 1 forks source link

The transferFrom() method is used instead of safeTransferFrom() #33

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L125 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L128 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L163 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L167 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L199 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L202 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L224 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L293 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L298 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L324 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L328 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L580

Vulnerability details

Details & Impact The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:

1)OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible 2)Given that any NFT can be used for the call option, there are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()

Recommended Mitigation Steps Call the safeTransferFrom() method instead of transferFrom()

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

JTraversa commented 2 years ago

Safe.transferFrom is calling the SafeTransferLib library with uToken.

bghughes commented 2 years ago

Safe.transferFrom is calling the SafeTransferLib library with uToken.

I agree with this point. Warden, the sponsor is using their own safe transfer library that differs from OZ's. Also note that the sponsor said that libraries, including Safe.sol were out of contest scope. Given the combination of these two factors, I believe this to be an invalid issue.

@JTraversa, kindly you could consider switching over to OZ's implementation for a more maintained and widely used ERC-20 safe transfer.