Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Immunefi reports #131

Merged
merged 10 commits into from
Mar 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 29 additions & 25 deletions src/ORMP.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ import "./security/ExcessivelySafeCall.sol";
contract ORMP is ReentrancyGuard, Channel {
using ExcessivelySafeCall for address;

event MessageAssigned(
bytes32 indexed msgHash, address indexed oracle, address indexed relayer, uint256 oracleFee, uint256 relayerFee
);

constructor(address dao) Channel(dao) {}

/// @dev Send a cross-chain message over the endpoint.
Expand All @@ -39,22 +43,21 @@ contract ORMP is ReentrancyGuard, Channel {
/// @param gasLimit Gas limit for destination user application used.
/// @param encoded The calldata which encoded by ABI Encoding.
/// @param refund Return extra fee to refund address.
/// @param params General extensibility for relayer to custom functionality.
function send(
uint256 toChainId,
address to,
uint256 gasLimit,
bytes calldata encoded,
address refund,
bytes calldata params
bytes calldata
) external payable sendNonReentrant returns (bytes32) {
// user application address.
address ua = msg.sender;
// send message by channel, return the hash of the message as id.
bytes32 msgHash = _send(ua, toChainId, to, gasLimit, encoded);

// handle fee
_handleFee(ua, refund, msgHash, toChainId, gasLimit, encoded, params);
_handleFee(ua, refund, msgHash, toChainId, gasLimit, encoded);

return msgHash;
}
Expand All @@ -65,21 +68,21 @@ contract ORMP is ReentrancyGuard, Channel {
bytes32 msgHash,
uint256 toChainId,
uint256 gasLimit,
bytes calldata encoded,
bytes calldata params
bytes calldata encoded
) internal {
// fetch user application's config.
UC memory uc = getAppConfig(ua);
// handle relayer fee
uint256 relayerFee = _handleRelayer(uc.relayer, msgHash, toChainId, ua, gasLimit, encoded, params);
uint256 relayerFee = _handleRelayer(uc.relayer, toChainId, ua, gasLimit, encoded);
// handle oracle fee
uint256 oracleFee = _handleOracle(uc.oracle, msgHash, toChainId, ua);
uint256 oracleFee = _handleOracle(uc.oracle, toChainId, ua);

emit MessageAssigned(msgHash, uc.oracle, uc.relayer, oracleFee, relayerFee);

// refund
if (msg.value > relayerFee + oracleFee) {
uint256 refundFee = msg.value - (relayerFee + oracleFee);
(bool success,) = refund.call{value: refundFee}("");
require(success, "!refund");
_sendValue(refund, refundFee);
}
}

Expand All @@ -88,35 +91,29 @@ contract ORMP is ReentrancyGuard, Channel {
// @param ua User application contract address which send the message.
/// @param gasLimit Gas limit for destination user application used.
/// @param encoded The calldata which encoded by ABI Encoding.
/// @param params General extensibility for relayer to custom functionality.
function fee(uint256 toChainId, address ua, uint256 gasLimit, bytes calldata encoded, bytes calldata params)
function fee(uint256 toChainId, address ua, uint256 gasLimit, bytes calldata encoded, bytes calldata)
external
view
returns (uint256)
{
UC memory uc = getAppConfig(ua);
uint256 relayerFee = IRelayer(uc.relayer).fee(toChainId, ua, gasLimit, encoded, params);
uint256 relayerFee = IRelayer(uc.relayer).fee(toChainId, ua, gasLimit, encoded);
uint256 oracleFee = IOracle(uc.oracle).fee(toChainId, ua);
return relayerFee + oracleFee;
}

function _handleRelayer(
address relayer,
bytes32 msgHash,
uint256 toChainId,
address ua,
uint256 gasLimit,
bytes calldata encoded,
bytes calldata params
) internal returns (uint256) {
uint256 relayerFee = IRelayer(relayer).fee(toChainId, ua, gasLimit, encoded, params);
IRelayer(relayer).assign{value: relayerFee}(msgHash, params);
function _handleRelayer(address relayer, uint256 toChainId, address ua, uint256 gasLimit, bytes calldata encoded)
internal
returns (uint256)
{
uint256 relayerFee = IRelayer(relayer).fee(toChainId, ua, gasLimit, encoded);
_sendValue(relayer, relayerFee);
return relayerFee;
}

function _handleOracle(address oracle, bytes32 msgHash, uint256 toChainId, address ua) internal returns (uint256) {
function _handleOracle(address oracle, uint256 toChainId, address ua) internal returns (uint256) {
uint256 oracleFee = IOracle(oracle).fee(toChainId, ua);
IOracle(oracle).assign{value: oracleFee}(msgHash);
_sendValue(oracle, oracleFee);
return oracleFee;
}

Expand Down Expand Up @@ -147,4 +144,11 @@ contract ORMP is ReentrancyGuard, Channel {
abi.encodePacked(message.encoded, msgHash, message.fromChainId, message.from)
);
}

/// @dev Replacement for Solidity's `transfer`: sends `amount` wei to
/// `recipient`, forwarding all available gas and reverting on errors.
function _sendValue(address recipient, uint256 amount) internal {
(bool success,) = recipient.call{value: amount}("");
require(success, "!send");
}
}
12 changes: 9 additions & 3 deletions src/UserConfig.sol
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ contract UserConfig {
/// @param oracle Oracle which the user application choose.
/// @param relayer Relayer which the user application choose.
event AppConfigUpdated(address indexed ua, address oracle, address relayer);
/// @dev Notifies an observer that the setter is changed.
/// @param oldSetter Old setter address.
/// @param newSetter New setter address.
event SetterChanged(address indexed oldSetter, address indexed newSetter);

modifier onlySetter() {
require(msg.sender == setter, "!auth");
Expand All @@ -52,9 +56,11 @@ contract UserConfig {

/// @dev Change setter.
/// @notice Only current setter could call.
/// @param setter_ New setter.
function changeSetter(address setter_) external onlySetter {
setter = setter_;
/// @param newSetter New setter.
function changeSetter(address newSetter) external onlySetter {
address oldSetter = setter;
setter = newSetter;
emit SetterChanged(oldSetter, newSetter);
}

/// @dev Set default user config for all user application.
Expand Down
6 changes: 0 additions & 6 deletions src/eco/ORMPOracle.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ pragma solidity 0.8.17;
import "../Verifier.sol";

contract ORMPOracle is Verifier {
event Assigned(bytes32 indexed msgHash, uint256 fee);
event SetFee(uint256 indexed chainId, uint256 fee);
event SetApproved(address operator, bool approve);
event Withdrawal(address indexed to, uint256 amt);
Expand Down Expand Up @@ -96,11 +95,6 @@ contract ORMPOracle is Verifier {
return f;
}

function assign(bytes32 msgHash) external payable {
require(msg.sender == PROTOCOL, "!auth");
emit Assigned(msgHash, msg.value);
}

function merkleRoot(uint256 chainId, uint256 blockNumber) public view override returns (bytes32) {
return rootOf[chainId][blockNumber];
}
Expand Down
6 changes: 0 additions & 6 deletions src/eco/Oracle.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import "../Verifier.sol";
import "../interfaces/IFeedOracle.sol";

contract Oracle is Verifier {
event Assigned(bytes32 indexed msgHash, uint256 fee);
event SetFee(uint256 indexed chainId, uint256 fee);
event SetApproved(address operator, bool approve);

Expand Down Expand Up @@ -79,11 +78,6 @@ contract Oracle is Verifier {
return feeOf[toChainId];
}

function assign(bytes32 msgHash) external payable {
require(msg.sender == PROTOCOL, "!auth");
emit Assigned(msgHash, msg.value);
}

function merkleRoot(uint256 chainId, uint256 /*blockNumber*/ ) public view override returns (bytes32) {
return IFeedOracle(SUBAPI).messageRootOf(chainId);
}
Expand Down
25 changes: 10 additions & 15 deletions src/eco/Relayer.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ pragma solidity 0.8.17;
import "../interfaces/IORMP.sol";

contract Relayer {
event Assigned(bytes32 indexed msgHash, uint256 fee, bytes params, bytes32[32] proof);
event SetDstPrice(uint256 indexed chainId, uint128 dstPriceRatio, uint128 dstGasPriceInWei);
event SetDstConfig(uint256 indexed chainId, uint64 baseGas, uint64 gasPerByte);
event SetApproved(address operator, bool approve);
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);

struct DstPrice {
uint128 dstPriceRatio; // dstPrice / localPrice * 10^10
Expand Down Expand Up @@ -69,8 +69,10 @@ contract Relayer {
return approvedOf[operator];
}

function changeOwner(address owner_) external onlyOwner {
owner = owner_;
function changeOwner(address newOwner) external onlyOwner {
address oldOwner = owner;
owner = newOwner;
emit OwnershipTransferred(oldOwner, newOwner);
}

function setApproved(address operator, bool approve) public onlyOwner {
Expand All @@ -89,13 +91,11 @@ contract Relayer {
}

// extraGas = gasLimit
function fee(
uint256 toChainId,
address, /*ua*/
uint256 gasLimit,
bytes calldata encoded,
bytes calldata /*params*/
) public view returns (uint256) {
function fee(uint256 toChainId, address, /*ua*/ uint256 gasLimit, bytes calldata encoded)
public
view
returns (uint256)
{
uint256 size = encoded.length;
uint256 extraGas = gasLimit;
DstPrice memory p = priceOf[toChainId];
Expand All @@ -111,11 +111,6 @@ contract Relayer {
return sourceToken + payloadToken;
}

function assign(bytes32 msgHash, bytes calldata params) external payable {
require(msg.sender == PROTOCOL, "!ormp");
emit Assigned(msgHash, msg.value, params, IORMP(PROTOCOL).prove());
}

function relay(Message calldata message, bytes calldata proof) external onlyApproved {
IORMP(PROTOCOL).recv(message, proof);
}
Expand Down
2 changes: 1 addition & 1 deletion src/interfaces/IORMP.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ interface IORMP {
/// @param gasLimit Gas limit for destination user application used.
/// @param encoded The calldata which encoded by ABI Encoding.
/// @param refund Return extra fee to refund address.
/// @param params General extensibility for relayer to custom functionality.
/// @return Return the hash of the message as message id.
/// @param params General extensibility for relayer to custom functionality.
function send(
uint256 toChainId,
address to,
Expand Down
4 changes: 0 additions & 4 deletions src/interfaces/IOracle.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,4 @@ interface IOracle is IVerifier {
/// @param ua The user application which send the message.
/// @return Oracle price in source native gas.
function fee(uint256 toChainId, address ua) external view returns (uint256);

/// @notice Assign the relay message root task to oracle maintainer.
/// @param msgHash Hash of the message.
function assign(bytes32 msgHash) external payable;
}
8 changes: 1 addition & 7 deletions src/interfaces/IRelayer.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,9 @@ interface IRelayer {
/// @param ua The user application which send the message.
/// @param gasLimit Gas limit for destination user application used.
/// @param encoded The calldata which encoded by ABI Encoding.
/// @param params General extensibility for relayer to custom functionality.
/// @return Relayer price in source native gas.
function fee(uint256 toChainId, address ua, uint256 gasLimit, bytes calldata encoded, bytes calldata params)
function fee(uint256 toChainId, address ua, uint256 gasLimit, bytes calldata encoded)
external
view
returns (uint256);

/// @notice Assign the relay message task to relayer maintainer.
/// @param msgHash Hash of the message.
/// @param params General extensibility for relayer to custom functionality.
function assign(bytes32 msgHash, bytes calldata params) external payable;
}
7 changes: 3 additions & 4 deletions test/ORMP.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ contract ORMPTest is Test, Verifier {
Proof proof;
address immutable self = address(this);

receive() external payable {}

function setUp() public {
vm.chainId(1);
ormp = new ORMP(self);
Expand Down Expand Up @@ -108,10 +110,7 @@ contract ORMPTest is Test, Verifier {
return 2;
}

function assign(bytes32) external payable {}
function assign(bytes32, bytes calldata) external payable {}

function fee(uint256, address, uint256, bytes calldata, bytes calldata) external pure returns (uint256) {
function fee(uint256, address, uint256, bytes calldata) external pure returns (uint256) {
return 1;
}

Expand Down
4 changes: 2 additions & 2 deletions test/bench/ORMP.b.sol
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ contract ORMPBenchmarkTest is Test {

function perform_send(uint256 fromChainId, uint256 toChainId, bytes calldata encoded) public {
vm.createSelectFork(fromChainId.toChainName());
uint256 f = ormp.fee(toChainId, self, 0, encoded, abi.encode(uint256(0)));
ormp.send{value: f}(toChainId, self, 0, encoded, self, abi.encode(uint256(0)));
uint256 f = ormp.fee(toChainId, self, 0, encoded, "");
ormp.send{value: f}(toChainId, self, 0, encoded, self, "");
}
}
5 changes: 0 additions & 5 deletions test/eco/Oracle.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,6 @@ contract OracleTest is Test {
oracle.setFee(1, 1);
}

function test_assign() public {
oracle.setFee(1, 1);
oracle.assign{value: 1}(bytes32(0));
}

function test_merkleRoot() public {
bytes32 r = oracle.merkleRoot(1, 1);
assertEq(r, bytes32(uint256(1)));
Expand Down
10 changes: 1 addition & 9 deletions test/eco/Relayer.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -78,18 +78,10 @@ contract RelayerTest is Test {
function test_setPrice() public {
relayer.setDstPrice(1, 10 ** 10, 1);
relayer.setDstConfig(1, 1, 1);
uint256 f = relayer.fee(1, self, 1, hex"00", abi.encode(uint256(1)));
uint256 f = relayer.fee(1, self, 1, hex"00");
assertEq(f, 3);
}

function test_assign() public {
relayer.setDstPrice(1, 10 ** 10, 1);
relayer.setDstConfig(1, 1, 1);
uint256 v = relayer.fee(1, address(1), 1, hex"00", abi.encode(uint256(1)));
relayer.assign{value: v}(bytes32(0), abi.encode(uint256(1)));
assertEq(v, 3);
}

function test_relay() public {
Message memory message = Message({
channel: address(0xc),
Expand Down
Loading