Secret Vault
First Flight #46
Beginner FriendlyWallet
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: medium
Valid

Lack of account authentication in `get_secret()` and `set_secret()`

enkidu

Root

The core functions in the contract is either lack of Account authentication or didn't implement any account authentication.

Impact

Anyone can also use the core functions in the contract.

Description

In the documentation, it is stated that only the Owner of the Vault can set and get the secret. This authentication is not properly implemented due to how Move handles Authentication.

public entry fun set_secret(caller:&signer,secret:vector<u8>){
@> No proper checks whether the caller is Owner
let secret_vault = Vault{secret: string::utf8(secret)};
move_to(caller,secret_vault);
event::emit(SetNewSecret {});
}
​
#[view]
@> This only check whether the address of caller is Owner, not who call the function
public fun get_secret (caller: address):String acquires Vault{
assert! (caller == @owner,NOT_OWNER);
let vault = borrow_global<Vault >(@owner);
vault.secret
}

Risk

Likelihood:

  • Any user can access both function since there are no proper authentication checks.

Impact:

  • Any user could call get_secret by passing the Owner address and retrieve Owner's Secret

  • Any user could just call set_secret even if they are not Owner. This however will only set the secret for the user specific vault and will not write into the Owner vault that will be retrieved by the get_secret function.

Proof of Concept

Add the following to the secret_vault.move

// Additional getter function to see whether the user successfully set
// a secret on their own Vault
#[view]
public fun get_secret_for_set_testing (caller: address):String acquires Vault{
let vault = borrow_global<Vault >(caller);
vault.secret
}
​
​
#[test(owner = @0xcc, user = @0xbad)]
fun anyone_can_set_and_read_secret(owner: &signer, user: &signer) acquires Vault {
use aptos_framework::account;
​
account::create_account_for_test(signer::address_of(owner));
account::create_account_for_test(signer::address_of(user));
​
// Owner sets a secret
let secret = b"i'm a secret";
set_secret(owner, secret);
// Anyone can access the secret by passing Owner address
let leaked = get_secret(signer::address_of(owner));
debug::print(&leaked);
​
// User sets a secret
let user_secret = b"this is user";
set_secret(user, user_secret);
// Use get_secret_for_testing to check whether the secret is set
let user_leaked = get_secret_for_set_testing(signer::address_of(user));
debug::print(&user_leaked);
// User can retrieve Owner secret by passing Owner address
let user_fetch_owner_secret = get_secret(signer::address_of(owner));
debug::print(&user_fetch_owner_secret);
}

Run the test and here is the result

$ aptos move test
​
INCLUDING DEPENDENCY AptosFramework
INCLUDING DEPENDENCY AptosStdlib
INCLUDING DEPENDENCY MoveStdlib
BUILDING aptos-secret-vault
Running Move unit tests
[debug] "i'm a secret"
[debug] "this is user"
[debug] "i'm a secret"
[ PASS ] 0x234::vault::anyone_can_set_and_read_secret
Test result: OK. Total tests: 1; passed: 1; failed: 0
{
"Result": "Success"
}

Recommended Mitigation

For set_secret function, it is recommended to add account authentication like below. As for the get_secret function, it is recommended to change the function visibility to no longer view, as authenticating the caller can only be done when the function is not a view type. This way, the caller of get_secret can be checked.

public entry fun set_secret(caller:&signer,secret:vector<u8>){
+ let addr = signer::address_of(caller);
+ assert!(addr == @owner, NOT_OWNER);
let secret_vault = Vault{secret: string::utf8(secret)};
move_to(caller,secret_vault);
event::emit(SetNewSecret {});
}
​
- #[view]
- public fun get_secret (caller: address):String acquires Vault{
+ public fun get_secret(caller: &signer): String acquires Vault {
+ let addr = signer::address_of(caller);
- assert! (caller == @owner,NOT_OWNER);
+ assert!(addr == @owner, NOT_OWNER);
let vault = borrow_global<Vault>(@owner);
vault.secret
}

To check for the mitigation, the test below can be run

// ✅ Test: User cannot set secret — expect NOT_OWNER abort
#[test(user = @0x123)]
#[expected_failure(abort_code = NOT_OWNER)]
fun test_user_cannot_set_secret(user: &signer) {
set_secret(user, b"fake secret");
}
​
// ✅ Test: User cannot get owner's secret — expect NOT_OWNER abort
#[test(owner = @0xcc, user = @0x123)]
#[expected_failure(abort_code = NOT_OWNER)]
fun test_user_cannot_fetch_owner_secret(owner: &signer, user: &signer) acquires Vault {
set_secret(owner, b"owner secret");
get_secret(user);
}

Test result:

$ aptos move test 1 ↵
INCLUDING DEPENDENCY AptosFramework
INCLUDING DEPENDENCY AptosStdlib
INCLUDING DEPENDENCY MoveStdlib
BUILDING aptos-secret-vault
Running Move unit tests
[ PASS ] 0x234::vault::test_user_cannot_fetch_owner_secret
[ PASS ] 0x234::vault::test_user_cannot_set_secret
Test result: OK. Total tests: 2; passed: 2; failed: 0
{
"Result": "Success"
}
Updates

Lead Judging Commences

bube Lead Judge 12 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Lack of signer check in `get_secret`

Anyone can call `set_secret` function

In Move for Aptos, the term "owner" refers to a signer, which is a verified account that owns a given resource, has permission to add resources and the ability to grant access or modify digital assets. Following this logic in this contest, the owner is the account that owns `Vault`. This means that anyone has right to call `set_secret` and then to own the `Vault` and to retrieve the secret from the `Vault` in `get_secret` function. Therefore, this group is invalid, because the expected behavior is anyone to call the `set_secret` function.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.