Inefficient publisher deletion algorithm results in excessive costs
Description
The del_publisher instruction allows a caller to remove a publisher from a price account. To do this, the instruction first loops through the publishers on the price account's comp_ array. After identifying the index of comp_ with the publisher account, an inner loop runs which shifts all of the accounts down.
static uint64_t del_publisher( SolParameters *prm, SolAccountInfo *ka )
{
...
// try to remove publisher
for(uint32_t i=0; i != sptr->num_; ++i ) {
pc_price_comp_t *iptr = &sptr->comp_[i];
if ( pc_pub_key_equal( &iptr->pub_, &cptr->pub_ ) ) {
for( unsigned j=i+1; j != sptr->num_; ++j ) {
pc_price_comp_t *jptr = &sptr->comp_[j];
iptr[0] = jptr[0];
iptr = jptr;
}
--sptr->num_;
sol_memset( iptr, 0, sizeof( pc_price_comp_t ) );
// update size of account
sptr->size_ = sizeof( pc_price_t ) - sizeof( sptr->comp_ ) +
sptr->num_ * sizeof( pc_price_comp_t );
return SUCCESS;
}
}
}This is an inefficient way to remove the publisher account from the price account.
Impact
This can result in excessive fees for removing a publisher than would otherwise be necessary. It also increases code complexity, which may leads to bugs in the future.
Recommendations
A more efficient solution would be to replace the publisher account with the last publisher account and then clear out the final publisher entry.
A reference implementation is supplied.
for(uint32_t i = 0; i != sptr->num_; ++i ) {
pc_price_comp_t *iptr = &sptr->comp_[i];
// identify the targeted publisher entry
if ( pc_pub_key_equal( &iptr->pub_, &cptr->pub_ ) ) {
// select the last publisher entry
pc_price_comp_t *substitute_ptr = &sptr->comp_[sptr->num_ - 1];
// swap the current publisher entry with the last one - it's okay if this is the same entry
iptr[0] = substitute_ptr[0];
// clear out the last publisher
sol_memset(substitute_ptr, 0, sizeof( pc_price_comp_t ));
// reduce the number of publishers by one
--sptr->num_;
// recalculate size
sptr->size_ = sizeof( pc_price_t ) - sizeof( sptr->comp_ )
+ sptr->num_ * sizeof( pc_price_comp_t );
return SUCCESS;
}
}Remediation
The finding has been acknowledged by Pyth Data Association. Their official response is reproduced below:
Pyth Data Association acknowledges the finding, but doesn't believe it has security implications. However, we may deploy a bug fix to address it.