Skip to content

Commit

Permalink
Audit fixes (#167)
Browse files Browse the repository at this point in the history 
* Prevent evm transfers with extra value provided

* Fix signature malleability on Solana

* Fix overflow on Solana

* Remove unnecessary yocto when minting tokens

* Apply lint

* Remvoe `valueRequired`

---------

Co-authored-by: karim-en <[email protected]>
  • Loading branch information
@kiseln @karim-en
kiseln and karim-en authored Dec 30, 2024
1 parent 93f4c8c commit 796772c
Show file tree
Hide file tree
Showing 11 changed files with 219 additions and 63 deletions.
24 changes: 14 additions & 10 deletions evm/src/omni-bridge/contracts/OmniBridge.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ contract OmniBridge is
error InvalidSignature();
error NonceAlreadyUsed(uint64 nonce);
error InvalidFee();
error InvalidValue();
error FailedToSendEther();

function initialize(
Expand Down Expand Up @@ -235,7 +236,6 @@ contract OmniBridge is
}

uint256 extensionValue;

if (tokenAddress == address(0)) {
if (fee != 0) {
revert InvalidFee();
Expand All @@ -259,16 +259,20 @@ contract OmniBridge is
}

function initTransferExtension(
address sender,
address tokenAddress,
uint64 originNonce,
uint128 amount,
uint128 fee,
uint128 nativeFee,
string calldata recipient,
string calldata message,
address /*sender*/,
address /*tokenAddress*/,
uint64 /*originNonce*/,
uint128 /*amount*/,
uint128 /*fee*/,
uint128 /*nativeFee*/,
string calldata /*recipient*/,
string calldata /*message*/,
uint256 value
) internal virtual {}
) internal virtual {
if (value != 0) {
revert InvalidValue();
}
}

function pause(uint flags) external onlyRole(DEFAULT_ADMIN_ROLE) {
_pause(flags);
Expand Down
2 changes: 2 additions & 0 deletions evm/src/omni-bridge/contracts/OmniBridgeWormhole.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ interface IWormhole {
bytes memory payload,
uint8 consistencyLevel
) external payable returns (uint64 sequence);

function messageFee() external view returns (uint256);
}

enum MessageType {
Expand Down
6 changes: 5 additions & 1 deletion evm/src/omni-bridge/contracts/test/TestWormhole.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,8 @@ contract TestWormhole {
emit MessagePublished(nonce, payload, consistencyLevel);
return 0;
}
}

function messageFee() external pure returns (uint256) {
return 10000;
}
}
78 changes: 65 additions & 13 deletions evm/tests/BridgeToken.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ describe("BridgeToken", () => {
).to.be.revertedWithCustomError(OmniBridge, "AccessControlUnauthorizedAccount")
})

it("deposit token", async () => {
it("can fin transfer", async () => {
const { token } = await createToken(wrappedNearId)
const tokenProxyAddress = await OmniBridge.nearToEthToken(wrappedNearId)

Expand All @@ -123,7 +123,7 @@ describe("BridgeToken", () => {
)
})

it("can't deposit if the contract is paused", async () => {
it("can't fin transfer if the contract is paused", async () => {
await createToken(wrappedNearId)
const tokenProxyAddress = await OmniBridge.nearToEthToken(wrappedNearId)

Expand All @@ -136,7 +136,7 @@ describe("BridgeToken", () => {
await expect(OmniBridge.finTransfer(signature, payload)).to.be.revertedWith("Pausable: paused")
})

it("can't deposit twice with the same signature", async () => {
it("can't fin transfer twice with the same signature", async () => {
await createToken(wrappedNearId)
const tokenProxyAddress = await OmniBridge.nearToEthToken(wrappedNearId)

Expand All @@ -149,7 +149,7 @@ describe("BridgeToken", () => {
)
})

it("can't deposit with invalid amount", async () => {
it("can't fin transfer with invalid amount", async () => {
await createToken(wrappedNearId)
const tokenProxyAddress = await OmniBridge.nearToEthToken(wrappedNearId)

Expand All @@ -162,7 +162,7 @@ describe("BridgeToken", () => {
)
})

it("can't deposit with invalid nonce", async () => {
it("can't fin transfer with invalid nonce", async () => {
await createToken(wrappedNearId)
const tokenProxyAddress = await OmniBridge.nearToEthToken(wrappedNearId)

Expand All @@ -175,7 +175,7 @@ describe("BridgeToken", () => {
)
})

it("can't deposit with invalid token", async () => {
it("can't fin transfer with invalid token", async () => {
await createToken(wrappedNearId)
const wrappedNearTokenAddress = await OmniBridge.nearToEthToken(wrappedNearId)

Expand All @@ -189,7 +189,7 @@ describe("BridgeToken", () => {
)
})

it("can't deposit with invalid recipient", async () => {
it("can't fin transfer with invalid recipient", async () => {
await createToken(wrappedNearId)
const tokenProxyAddress = await OmniBridge.nearToEthToken(wrappedNearId)

Expand All @@ -202,7 +202,7 @@ describe("BridgeToken", () => {
)
})

it("can't deposit with invalid relayer", async () => {
it("can't fin transfer with invalid relayer", async () => {
await createToken(wrappedNearId)
const wrappedNearTokenAddress = await OmniBridge.nearToEthToken(wrappedNearId)

Expand All @@ -215,7 +215,7 @@ describe("BridgeToken", () => {
)
})

it("withdraw token", async () => {
it("can init transfer", async () => {
const { token } = await createToken(wrappedNearId)
const tokenProxyAddress = await token.getAddress()

Expand All @@ -242,15 +242,15 @@ describe("BridgeToken", () => {
expect((await token.balanceOf(user1.address)).toString()).to.be.equal("0")
})

it("cant withdraw token when paused", async () => {
it("can't init transfer token when paused", async () => {
await createToken(wrappedNearId)
const tokenProxyAddress = await OmniBridge.nearToEthToken(wrappedNearId)

const { signature, payload } = depositSignature(tokenProxyAddress, user1.address)
await OmniBridge.finTransfer(signature, payload)

const fee = 0
const nativeFee = 0
const nativeFee = 100
const message = ""
await expect(OmniBridge.pause(PauseMode.PausedInitTransfer))
.to.emit(OmniBridge, "Paused")
Expand All @@ -263,11 +263,63 @@ describe("BridgeToken", () => {
nativeFee,
"testrecipient.near",
message,
{
value: 100,
},
),
).to.be.revertedWith("Pausable: paused")
})

it("can deposit and withdraw after unpausing", async () => {
it("can't init transfer when value is too low", async () => {
await createToken(wrappedNearId)
const tokenProxyAddress = await OmniBridge.nearToEthToken(wrappedNearId)

const { signature, payload } = depositSignature(tokenProxyAddress, user1.address)
await OmniBridge.finTransfer(signature, payload)

const fee = 0
const nativeFee = 100
const message = ""

await expect(
OmniBridge.initTransfer(
tokenProxyAddress,
payload.amount,
fee,
nativeFee,
"testrecipient.near",
message,
),
).to.be.revertedWithCustomError(OmniBridge, "InvalidValue")
})

it("can't init transfer when value is too high", async () => {
await createToken(wrappedNearId)
const tokenProxyAddress = await OmniBridge.nearToEthToken(wrappedNearId)

const { signature, payload } = depositSignature(tokenProxyAddress, user1.address)
await OmniBridge.finTransfer(signature, payload)

const fee = 0
const nativeFee = 100
const message = ""

await expect(
OmniBridge.initTransfer(
tokenProxyAddress,
payload.amount,
fee,
nativeFee,
"testrecipient.near",
message,
{
value: 200,
},
),
).to.be.revertedWithCustomError(OmniBridge, "InvalidValue")
})

it("can fin and init transfer after unpausing", async () => {
const { token } = await createToken(wrappedNearId)
const tokenProxyAddress = await token.getAddress()

Expand Down Expand Up @@ -313,7 +365,7 @@ describe("BridgeToken", () => {
expect((await BridgeTokenV2Proxied.decimals()).toString()).to.equal("24")
})

it("user cant upgrade token contract", async () => {
it("user can't upgrade token contract", async () => {
await createToken(wrappedNearId)
const tokenProxyAddress = await OmniBridge.nearToEthToken(wrappedNearId)

Expand Down
25 changes: 23 additions & 2 deletions evm/tests/BridgeTokenWormhole.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ describe("BridgeTokenWormhole", () => {
.withArgs(0, anyValue, consistencyLevel)
})

it("deposit token", async () => {
it("fin transfer", async () => {
const { token } = await createToken(wrappedNearId)
const tokenProxyAddress = await token.getAddress()
const { signature, payload } = depositSignature(tokenProxyAddress, await user1.getAddress())
Expand All @@ -163,7 +163,7 @@ describe("BridgeTokenWormhole", () => {
)
})

it("withdraw token", async () => {
it("init transfer", async () => {
const { token } = await createToken(wrappedNearId)
const tokenProxyAddress = await token.getAddress()
const { signature, payload } = depositSignature(tokenProxyAddress, await user1.getAddress())
Expand Down Expand Up @@ -197,11 +197,32 @@ describe("BridgeTokenWormhole", () => {
nativeFee,
recipient,
message,
{
value: 10000,
},
),
)
.to.emit(TestWormhole, "MessagePublished")
.withArgs(2, expectedWormholeMessage, consistencyLevel)

expect((await token.balanceOf(await user1.getAddress())).toString()).to.be.equal("0")
})

it("can't init transfer without enough value", async () => {
const { token } = await createToken(wrappedNearId)
const tokenProxyAddress = await token.getAddress()
const { signature, payload } = depositSignature(tokenProxyAddress, await user1.getAddress())
await OmniBridgeWormhole.finTransfer(signature, payload)

await expect(
OmniBridgeWormhole.connect(user1).initTransfer(
tokenProxyAddress,
payload.amount,
0,
0,
"testrecipient.near",
"",
),
).to.be.revertedWithCustomError(OmniBridgeWormhole, "InvalidValue")
})
})
57 changes: 31 additions & 26 deletions near/omni-bridge/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -952,30 +952,35 @@ impl Contract {
Promise::new(recipient)
.transfer(NearToken::from_yoctonear(amount_to_transfer.0)),
)
} else {
let transfer_promise = ext_token::ext(token.clone()).with_attached_deposit(ONE_YOCTO);
if is_deployed_token {
transfer_promise
.with_static_gas(MINT_TOKEN_GAS.saturating_add(FT_TRANSFER_CALL_GAS))
.mint(
recipient,
amount_to_transfer,
(!transfer_message.msg.is_empty()).then(|| transfer_message.msg.clone()),
)
} else if transfer_message.msg.is_empty() {
transfer_promise
.with_static_gas(FT_TRANSFER_GAS)
.ft_transfer(recipient, amount_to_transfer, None)
} else if is_deployed_token {
let deposit = if transfer_message.msg.is_empty() {
NO_DEPOSIT
} else {
transfer_promise
.with_static_gas(FT_TRANSFER_CALL_GAS)
.ft_transfer_call(
recipient,
amount_to_transfer,
None,
transfer_message.msg.clone(),
)
}
ONE_YOCTO
};
ext_token::ext(token.clone())
.with_attached_deposit(deposit)
.with_static_gas(MINT_TOKEN_GAS.saturating_add(FT_TRANSFER_CALL_GAS))
.mint(
recipient,
amount_to_transfer,
(!transfer_message.msg.is_empty()).then(|| transfer_message.msg.clone()),
)
} else if transfer_message.msg.is_empty() {
ext_token::ext(token.clone())
.with_attached_deposit(ONE_YOCTO)
.with_static_gas(FT_TRANSFER_GAS)
.ft_transfer(recipient, amount_to_transfer, None)
} else {
ext_token::ext(token.clone())
.with_attached_deposit(ONE_YOCTO)
.with_static_gas(FT_TRANSFER_CALL_GAS)
.ft_transfer_call(
recipient,
amount_to_transfer,
None,
transfer_message.msg.clone(),
)
};

if transfer_message.fee.fee.0 > 0 {
Expand All @@ -988,15 +993,15 @@ impl Contract {
);
storage_deposit_action_index += 1;

let transfer_fee_promise = ext_token::ext(token).with_attached_deposit(ONE_YOCTO);
promise = promise.then(if is_deployed_token {
transfer_fee_promise.with_static_gas(MINT_TOKEN_GAS).mint(
ext_token::ext(token).with_static_gas(MINT_TOKEN_GAS).mint(
predecessor_account_id.clone(),
transfer_message.fee.fee,
None,
)
} else {
transfer_fee_promise
ext_token::ext(token)
.with_attached_deposit(ONE_YOCTO)
.with_static_gas(FT_TRANSFER_GAS)
.ft_transfer(
predecessor_account_id.clone(),
Expand Down
Loading

0 comments on commit 796772c

Please sign in to comment.