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

0 stars 1 forks source link

QA Report #197

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Unspecific compiler version pragma

Information : L003 - Unspecific Compiler Version Pragma Consensys Audit of 1inch Solidity docs

Instances include :

VaultTracker/FixedPointMathLib.sol:2:pragma solidity >=0.8.0;
VaultTracker/LibCompound.sol:2:pragma solidity >=0.8.4;
Marketplace/FixedPointMathLib.sol:2:pragma solidity >=0.8.0;
Marketplace/LibCompound.sol:2:pragma solidity >=0.8.4;
Creator/FixedPointMathLib.sol:2:pragma solidity >=0.8.0;
Creator/LibCompound.sol:2:pragma solidity >=0.8.4;
Tokens/FixedPointMathLib.sol:2:pragma solidity >=0.8.0;
Tokens/LibCompound.sol:2:pragma solidity >=0.8.4;
Marketplace/Erc20.sol:4:pragma solidity ^0.8.0;
Creator/Erc20.sol:4:pragma solidity ^0.8.0;
Creator/IRedeemer.sol:2:pragma solidity ^0.8.0;
Creator/ZcToken.sol:2:pragma solidity ^0.8.4;
Creator/IERC5095.sol:2:pragma solidity ^0.8.0;
Tokens/Erc20.sol:4:pragma solidity ^0.8.0;
Tokens/IRedeemer.sol:2:pragma solidity ^0.8.0;
Tokens/ZcToken.sol:2:pragma solidity ^0.8.4;
Tokens/IERC5095.sol:2:pragma solidity ^0.8.0;

Recommendation

It is recommended to use a concrete compiler version, for example :

VaultTracker/FixedPointMathLib.sol:2:pragma solidity 0.8.0;

Unsafe ERC20 Operation(s)

Information : L001 - Unsafe ERC20 Operation(s)

Instances include :

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);

Recommendation

It is recommended to always use OpenZeppelin's SafeERC20 library, for example :

Swivel/Swivel.sol:359:    Safe.safeTransfer(uToken, o.maker, a - premiumFilled);

Use two-step transfer pattern for access controls

Contracts that implement access control, e.g. admin, should consider implementing a two-step transfer pattern. Otherwise, it is possible that the role mistakenly transfers ownership to the wrong address, resulting in losing the role.

Instances include :

https://github.com/code-423n4/2022-07-swivel/blob/main/Marketplace/MarketPlace.sol#L53-L56 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L428-L432 https://github.com/code-423n4/2022-07-swivel/blob/main/Creator/Creator.sol#L47-L50

Recommendation

It is recommended separate the ownership transfer into 2 functions, first to set a pending owner, and second to approve the pending admin set using the first function and actually implementing the change of the ownership.


robrobbins commented 2 years ago

we use solmate safe lib, so this comment is incorrect

rest are dupe or wontfix