Skip to content

Commit

Permalink
Merge pull request #8 from VenusProtocol/fix/VEN-2240
Browse files Browse the repository at this point in the history 
[VEN-2240]: Certik audit fixes on token bridge
  • Loading branch information
@GitGuru7
GitGuru7 authored Dec 20, 2023
2 parents 3e8bae9 + 66e0995 commit b46ca30
Show file tree
Hide file tree
Showing 9 changed files with 257 additions and 54 deletions.
103 changes: 80 additions & 23 deletions contracts/Bridge/BaseXVSProxyOFT.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,12 @@ abstract contract BaseXVSProxyOFT is Pausable, ExponentialNoError, BaseOFTV2 {
*/
mapping(uint16 => uint256) public chainIdToLast24HourReceiveWindowStart;
/**
* @notice Address on which cap check and bound limit is not appicable.
* @notice Address on which cap check and bound limit is not applicable.
*/
mapping(address => bool) public whitelist;

/**
* @notice Emmited when address is added to whitelist.
* @notice Emitted when address is added to whitelist.
*/
event SetWhitelist(address indexed addr, bool isWhitelist);
/**
Expand Down Expand Up @@ -98,14 +98,26 @@ abstract contract BaseXVSProxyOFT is Pausable, ExponentialNoError, BaseOFTV2 {
*/
event InnerTokenAdded(address indexed innerToken);

/**
*@notice Emitted on sweep token success
*/
event SweepToken(address indexed token, address indexed to, uint256 sweepAmount);

/**
*@notice Error thrown when this contract balance is less than sweep amount
*/
error InsufficientBalance(uint256 sweepAmount, uint256 balance);

/**
* @param tokenAddress_ Address of the inner token.
* @param sharedDecimals_ No of shared decimals.
* @param sharedDecimals_ Number of shared decimals.
* @param lzEndpoint_ Address of the layer zero endpoint contract.
* @param oracle_ Address of the price oracle.
* @custom:error ZeroAddressNotAllowed is thrown when token contract address is zero.
* @custom:error ZeroAddressNotAllowed is thrown when lzEndpoint contract address is zero.
* @custom:error ZeroAddressNotAllowed is thrown when oracle contract address is zero.
* @custom:event Emits InnerTokenAdded with token address.
* @custom:event Emits OracleChanged with zero address and oracle address.
*/
constructor(
address tokenAddress_,
Expand All @@ -115,6 +127,7 @@ abstract contract BaseXVSProxyOFT is Pausable, ExponentialNoError, BaseOFTV2 {
) BaseOFTV2(sharedDecimals_, lzEndpoint_) {
ensureNonzeroAddress(tokenAddress_);
ensureNonzeroAddress(lzEndpoint_);
ensureNonzeroAddress(oracle_);

innerToken = IERC20(tokenAddress_);

Expand All @@ -125,8 +138,6 @@ abstract contract BaseXVSProxyOFT is Pausable, ExponentialNoError, BaseOFTV2 {
require(sharedDecimals_ <= decimals, "ProxyOFT: sharedDecimals must be <= decimals");
ld2sdRate = 10 ** (decimals - sharedDecimals_);

ensureNonzeroAddress(oracle_);

emit InnerTokenAdded(tokenAddress_);
emit OracleChanged(address(0), oracle_);

Expand Down Expand Up @@ -203,7 +214,7 @@ abstract contract BaseXVSProxyOFT is Pausable, ExponentialNoError, BaseOFTV2 {

/**
* @notice Sets the whitelist address to skip checks on transaction limit.
* @param user_ Adress to be add in whitelist.
* @param user_ Address to be add in whitelist.
* @param val_ Boolean to be set (true for user_ address is whitelisted).
* @custom:access Only owner.
* @custom:event Emits setWhitelist.
Expand All @@ -229,15 +240,54 @@ abstract contract BaseXVSProxyOFT is Pausable, ExponentialNoError, BaseOFTV2 {
_unpause();
}

/**
* @notice A public function to sweep accidental ERC-20 transfers to this contract. Tokens are sent to user
* @param token_ The address of the ERC-20 token to sweep
* @param to_ The address of the recipient
* @param amount_ The amount of tokens needs to transfer
* @custom:event Emits SweepToken event
* @custom:error Throw InsufficientBalance if amount_ is greater than the available balance of the token in the contract
* @custom:access Only Owner
*/
function sweepToken(IERC20 token_, address to_, uint256 amount_) external onlyOwner {
uint256 balance = token_.balanceOf(address(this));
if (amount_ > balance) {
revert InsufficientBalance(amount_, balance);
}

emit SweepToken(address(token_), to_, amount_);

token_.safeTransfer(to_, amount_);
}

/**
* @notice Remove trusted remote from storage.
* @param remoteChainId_ The chain's id corresponds to setting the trusted remote to empty.
* @custom:access Only owner.
* @custom:event Emits TrustedRemoteRemoved once chain id is removed from trusted remote.
*/
function removeTrustedRemote(uint16 remoteChainId_) external onlyOwner {
delete trustedRemoteLookup[remoteChainId_];
emit TrustedRemoteRemoved(remoteChainId_);
}

function retryMessage(
uint16 _srcChainId,
bytes calldata _srcAddress,
uint64 _nonce,
bytes calldata _payload
) public payable override {
bytes memory trustedRemote = trustedRemoteLookup[_srcChainId];
// it will still block the message pathway from (srcChainId, srcAddress). should not receive message from untrusted remote.
require(
_srcAddress.length == trustedRemote.length &&
trustedRemote.length > 0 &&
keccak256(_srcAddress) == keccak256(trustedRemote),
"LzApp: invalid source sending contract"
);
super.retryMessage(_srcChainId, _srcAddress, _nonce, _payload);
}

/**
* @notice Checks the eligibility of a sender to initiate a cross-chain token transfer.
* @dev This external view function assesses whether the specified sender is eligible to transfer the given amount
Expand Down Expand Up @@ -300,11 +350,18 @@ abstract contract BaseXVSProxyOFT is Pausable, ExponentialNoError, BaseOFTV2 {

/**
* @notice Return's the address of the inner token of this bridge.
* @return Address of the inner token of this bridge.
*/
function token() public view override returns (address) {
return address(innerToken);
}

/**
* @notice Checks if the sender is eligible to send tokens
* @param from_ Sender's address sending tokens
* @param dstChainId_ Chain id on which tokens should be sent
* @param amount_ Amount of tokens to be sent
*/
function _isEligibleToSend(address from_, uint16 dstChainId_, uint256 amount_) internal {
// Check if the sender's address is whitelisted
bool isWhiteListedUser = whitelist[from_];
Expand Down Expand Up @@ -343,6 +400,12 @@ abstract contract BaseXVSProxyOFT is Pausable, ExponentialNoError, BaseOFTV2 {
chainIdToLast24HourTransferred[dstChainId_] = transferredInWindow;
}

/**
* @notice Checks if receiver is able to receive tokens
* @param toAddress_ Receiver address
* @param srcChainId_ Source chain id from which token is send
* @param receivedAmount_ Amount of tokens received
*/
function _isEligibleToReceive(address toAddress_, uint16 srcChainId_, uint256 receivedAmount_) internal {
// Check if the recipient's address is whitelisted
bool isWhiteListedUser = whitelist[toAddress_];
Expand Down Expand Up @@ -382,6 +445,13 @@ abstract contract BaseXVSProxyOFT is Pausable, ExponentialNoError, BaseOFTV2 {
chainIdToLast24HourReceived[srcChainId_] = receivedInWindow;
}

/**
* @notice Transfer tokens from sender to receiver account.
* @param from_ Address from which token has to be transferred(Sender).
* @param to_ Address on which token will be tranferred(Receiver).
* @param amount_ Amount of token to be transferred.
* @return Actual balance difference.
*/
function _transferFrom(
address from_,
address to_,
Expand All @@ -396,23 +466,10 @@ abstract contract BaseXVSProxyOFT is Pausable, ExponentialNoError, BaseOFTV2 {
return innerToken.balanceOf(to_) - before;
}

function _nonblockingLzReceive(
uint16 _srcChainId,
bytes memory _srcAddress,
uint64 _nonce,
bytes memory _payload
) internal override {
bytes memory trustedRemote = trustedRemoteLookup[_srcChainId];
// if will still block the message pathway from (srcChainId, srcAddress). should not receive message from untrusted remote.
require(
_srcAddress.length == trustedRemote.length &&
trustedRemote.length > 0 &&
keccak256(_srcAddress) == keccak256(trustedRemote),
"LzApp: invalid source sending contract"
);
super._nonblockingLzReceive(_srcChainId, _srcAddress, _nonce, _payload);
}

/**
* @notice Returns Conversion rate factor from large decimals to shared decimals.
* @return Conversion rate factor.
*/
function _ld2sdRate() internal view override returns (uint256) {
return ld2sdRate;
}
Expand Down
33 changes: 24 additions & 9 deletions contracts/Bridge/XVSBridgeAdmin.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ contract XVSBridgeAdmin is AccessControlledV8 {
*/
mapping(bytes4 => string) public functionRegistry;

// Event emitted when function registry updated
/**
* @notice emitted when function registry updated
*/
event FunctionRegistryChanged(string signature, bool active);

/// @custom:oz-upgrades-unsafe-allow constructor
Expand All @@ -33,15 +35,14 @@ contract XVSBridgeAdmin is AccessControlledV8 {

/**
* @param accessControlManager_ Address of access control manager contract.
* @custom:error ZeroAddressNotAllowed is thrown when accessControlManager contract address is zero.
*/
function initialize(address accessControlManager_) external initializer {
ensureNonzeroAddress(accessControlManager_);
__AccessControlled_init(accessControlManager_);
}

/**
* @notice Invoked when called function does not exist in the contract
* @notice Invoked when called function does not exist in the contract.
* @return Response of low level call.
* @custom:access Controlled by AccessControlManager.
*/
fallback(bytes calldata data) external returns (bytes memory) {
Expand All @@ -54,8 +55,10 @@ contract XVSBridgeAdmin is AccessControlledV8 {
}

/**
* @notice Sets trusted remote on particular chain.
* @param remoteChainId_ Chain Id of the destination chain.
* @param remoteAddress_ Address of the destination bridge.
* @custom:access Controlled by AccessControlManager.
* @custom:error ZeroAddressNotAllowed is thrown when remoteAddress_ contract address is zero.
*/
function setTrustedRemoteAddress(uint16 remoteChainId_, bytes calldata remoteAddress_) external {
Expand All @@ -66,14 +69,16 @@ contract XVSBridgeAdmin is AccessControlledV8 {
}

/**
* @notice A registry of functions that are allowed to be executed from proposals
* @notice A setter for the registry of functions that are allowed to be executed from proposals.
* @param signatures_ Function signature to be added or removed.
* @param active_ bool value, should be true to add function.
* @custom:access Only owner.
* @custom:event Emits FunctionRegistryChanged if bool value of function changes.
*/
function upsertSignature(string[] calldata signatures_, bool[] calldata active_) external onlyOwner {
uint256 signatureLength = signatures_.length;
require(signatureLength == active_.length, "Input arrays must have the same length");
for (uint256 i; i < signatureLength; i++) {
for (uint256 i; i < signatureLength; ) {
bytes4 sigHash = bytes4(keccak256(bytes(signatures_[i])));
bytes memory signature = bytes(functionRegistry[sigHash]);
if (active_[i] && signature.length == 0) {
Expand All @@ -83,25 +88,28 @@ contract XVSBridgeAdmin is AccessControlledV8 {
delete functionRegistry[sigHash];
emit FunctionRegistryChanged(signatures_[i], false);
}
unchecked {
++i;
}
}
}

/**
* @notice This function transfer the ownership of the bridge from this contract to new owner.
* @notice This function transfers the ownership of the bridge from this contract to new owner.
* @param newOwner_ New owner of the XVS Bridge.
* @custom:access Controlled by AccessControlManager.
*/
function transferBridgeOwnership(address newOwner_) external {
_checkAccessAllowed("transferBridgeOwnership(address)");
ensureNonzeroAddress(newOwner_);
XVSBridge.transferOwnership(newOwner_);
}

/**
* @notice Returns bool = true if srcAddress_ is trustedRemote corresponds to chainId_.
* @notice Returns true if remote address is trustedRemote corresponds to chainId_.
* @param remoteChainId_ Chain Id of the destination chain.
* @param remoteAddress_ Address of the destination bridge.
* @custom:error ZeroAddressNotAllowed is thrown when remoteAddress_ contract address is zero.
* @return Bool indicating whether the remote chain is trusted or not.
*/
function isTrustedRemote(uint16 remoteChainId_, bytes calldata remoteAddress_) external returns (bool) {
require(remoteChainId_ != 0, "ChainId must not be zero");
Expand All @@ -116,11 +124,18 @@ contract XVSBridgeAdmin is AccessControlledV8 {

/**
* @dev Returns function name string associated with function signature.
* @param signature_ Four bytes of function signature.
* @return Function signature corresponding to its hash.
*/
function _getFunctionName(bytes4 signature_) internal view returns (string memory) {
return functionRegistry[signature_];
}

/**
* @notice Converts given bytes into address.
* @param b Bytes to be converted into address.
* @return Converted address of given bytes.
*/
function bytesToAddress(bytes calldata b) private pure returns (address) {
return address(uint160(bytes20(b)));
}
Expand Down
22 changes: 20 additions & 2 deletions contracts/Bridge/XVSProxyOFTDest.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { BaseXVSProxyOFT } from "./BaseXVSProxyOFT.sol";

contract XVSProxyOFTDest is BaseXVSProxyOFT {
/**
* @notice Emits when stored message dropped without successfull retrying.
* @notice Emits when stored message dropped without successful retrying.
*/
event DropFailedMessage(uint16 srcChainId, bytes indexed srcAddress, uint64 nonce);

Expand All @@ -25,10 +25,13 @@ contract XVSProxyOFTDest is BaseXVSProxyOFT {
address oracle_
) BaseXVSProxyOFT(tokenAddress_, sharedDecimals_, lzEndpoint_, oracle_) {}

/** @notice Clear failed messages from the storage.
/**
* @notice Clear failed messages from the storage.
* @param srcChainId_ Chain id of source
* @param srcAddress_ Address of source followed by current bridge address
* @param nonce_ Nonce_ of the transaction
* @custom:access Only owner
* @custom:event Emits DropFailedMessage on clearance of failed message.
*/
function dropFailedMessage(uint16 srcChainId_, bytes memory srcAddress_, uint64 nonce_) external onlyOwner {
failedMessages[srcChainId_][srcAddress_][nonce_] = bytes32(0);
Expand All @@ -37,11 +40,19 @@ contract XVSProxyOFTDest is BaseXVSProxyOFT {

/**
* @notice Returns the total circulating supply of the token on the destination chain i.e (total supply).
* @return total circulating supply of the token on the destination chain.
*/
function circulatingSupply() public view override returns (uint256) {
return innerToken.totalSupply();
}

/**
* @notice Debit tokens from the given address
* @param from_ Address from which tokens to be debited
* @param dstChainId_ Destination chain id
* @param amount_ Amount of tokens to be debited
* @return Actual amount debited
*/
function _debitFrom(
address from_,
uint16 dstChainId_,
Expand All @@ -54,6 +65,13 @@ contract XVSProxyOFTDest is BaseXVSProxyOFT {
return amount_;
}

/**
* @notice Credit tokens in the given account
* @param srcChainId_ Source chain id
* @param toAddress_ Address on which token will be credited
* @param amount_ Amount of tokens to be credited
* @return Actual amount credited
*/
function _creditTo(
uint16 srcChainId_,
address toAddress_,
Expand Down
Loading

0 comments on commit b46ca30

Please sign in to comment.