Skip to content

Conversation

cwsnt
Copy link
Contributor

@cwsnt cwsnt commented Feb 23, 2022

Refactor rbtcWrapperProxy to support non-wrbtc for providing liquidity

@tjcloa tjcloa self-requested a review March 3, 2022 10:36
Comment on lines 489 to 498
function withdraw(address token, address payable to, uint256 amount) external ownerOnly {
require(amount > 0, "non-zero withdraw amount expected");
require(to != address(0), "receiver address invalid");

if (token == address(0)) {
require(amount <= address(this).balance, "withdraw amount cannot exceed balance");
to.transfer(amount);
} else {
require(IERC20Token(token).transfer(to, amount), "Failed to transfer the token");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CD6 - Don't use assert, tx.origin, address.transfer(), address.send()
https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

import "../openzeppelin/SafeERC20.sol"; // copy from the protocol or from OZ - install proper version
using SafeERC20 for IERC20; // in the contract

address to; - to allow transfer to a contract address too (e.g. multisig)

Suggested change
function withdraw(address token, address payable to, uint256 amount) external ownerOnly {
require(amount > 0, "non-zero withdraw amount expected");
require(to != address(0), "receiver address invalid");
if (token == address(0)) {
require(amount <= address(this).balance, "withdraw amount cannot exceed balance");
to.transfer(amount);
} else {
require(IERC20Token(token).transfer(to, amount), "Failed to transfer the token");
}
function withdraw(
address token,
address to,
uint256 amount
) external ownerOnly {
require(amount > 0, "non-zero withdraw amount expected");
require(to != address(0), "receiver address invalid");
if (token == address(0)) {
(bool success, ) = to.call.value(amount)("");
require(success, "transfer failed");
} else {
IERC20(token).safeTransfer(token, to, amount);
}
}

Copy link
Contributor

@tjcloa tjcloa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
pls resolve all conversations (comments)

@Alitasovryn
Copy link

Tested and reviewed the testRBTCWrapperProxy.js file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants